All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
@ 2021-11-17  7:07 Li Wang
  2021-11-17  7:07 ` [LTP] [PATCH v5 2/3] testcase: make use of .supported_archs Li Wang
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Li Wang @ 2021-11-17  7:07 UTC (permalink / raw)
  To: ltp

Testcases for specific arch should be limited on that only being supported
platform to run, we now involve a .supported_archs to achieve this feature
in LTP library. All you need to run a test on the expected arch is to set
the '.supported_archs' array in the 'struct tst_test' to choose the required
arch list. e.g.

    .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}

This helps move the TCONF info from code to tst_test metadata as well.

And, we also export a struct tst_arch to save the system architecture
for using in the whole test cases.

    extern const struct tst_arch {
             char name[16];
             enum tst_arch_type type;
    } tst_arch;

Signed-off-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
 doc/c-test-api.txt | 36 +++++++++++++++++
 include/tst_arch.h | 39 +++++++++++++++++++
 include/tst_test.h |  7 ++++
 lib/tst_arch.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/tst_test.c     |  3 ++
 5 files changed, 181 insertions(+)
 create mode 100644 include/tst_arch.h
 create mode 100644 lib/tst_arch.c

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 3127018a4..64d0630ce 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -2261,6 +2261,42 @@ struct tst_test test = {
 };
 -------------------------------------------------------------------------------
 
+1.39 Testing on the specific architecture
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Testcases for specific arch should be limited on that only being supported
+platform to run, we now involve a .supported_archs to achieve this feature
+in LTP library. All you need to run a test on the expected arch is to set
+the '.supported_archs' array in the 'struct tst_test' to choose the required
+arch list. e.g.
+
+    .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
+
+This helps move the TCONF info from code to tst_test metadata as well.
+
+And, we also export a struct tst_arch to save the system architecture for
+using in the whole test cases.
+
+    extern const struct tst_arch {
+             char name[16];
+             enum tst_arch_type type;
+    } tst_arch;
+
+[source,c]
+-------------------------------------------------------------------------------
+#include "tst_test.h"
+
+static struct tst_test test = {
+       ...
+       .setup = setup,
+       .supported_archs = (const char *const []) {
+                 "x86_64",
+                 "ppc64",
+                 "s390x",
+                 NULL
+       },
+};
+-------------------------------------------------------------------------------
+
 2. Common problems
 ------------------
 
diff --git a/include/tst_arch.h b/include/tst_arch.h
new file mode 100644
index 000000000..4a9473c6f
--- /dev/null
+++ b/include/tst_arch.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (c) 2021 Li Wang <liwang@redhat.com>
+ */
+
+#ifndef TST_ARCH_H__
+#define TST_ARCH_H__
+
+enum tst_arch_type {
+	TST_UNKNOWN,
+	TST_X86,
+	TST_X86_64,
+	TST_IA64,
+	TST_PPC,
+	TST_PPC64,
+	TST_S390,
+	TST_S390X,
+	TST_ARM,
+	TST_AARCH64,
+	TST_SPARC,
+};
+
+/*
+ * This tst_arch is to save the system architecture for
+ * using in the whole testcase.
+ */
+extern const struct tst_arch {
+        char name[16];
+        enum tst_arch_type type;
+} tst_arch;
+
+/*
+ * Check if test platform is in the given arch list. If yes return 1,
+ * otherwise return 0.
+ *
+ * @archlist A NULL terminated array of architectures to support.
+ */
+int tst_is_on_arch(const char *const *archlist);
+
+#endif /* TST_ARCH_H__ */
diff --git a/include/tst_test.h b/include/tst_test.h
index 602ce3090..c06a4729b 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -43,6 +43,7 @@
 #include "tst_fips.h"
 #include "tst_taint.h"
 #include "tst_memutils.h"
+#include "tst_arch.h"
 
 /*
  * Reports testcase result.
@@ -139,6 +140,12 @@ struct tst_test {
 
 	const char *min_kver;
 
+	/*
+	 * The supported_archs is a NULL terminated list of archs the test
+	 * does support.
+	 */
+	const char *const *supported_archs;
+
 	/* If set the test is compiled out */
 	const char *tconf_msg;
 
diff --git a/lib/tst_arch.c b/lib/tst_arch.c
new file mode 100644
index 000000000..f19802a03
--- /dev/null
+++ b/lib/tst_arch.c
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (c) 2021 Li Wang <liwang@redhat.com>
+ */
+
+#include <string.h>
+#include <stdlib.h>
+
+#define TST_NO_DEFAULT_MAIN
+#include "tst_arch.h"
+#include "tst_test.h"
+
+const struct tst_arch tst_arch = {
+#if defined(__x86_64__)
+        .name = "x86_64",
+        .type = TST_X86_64,
+#elif defined(__i386__) || defined(__i586__) || defined(__i686__)
+        .name = "x86",
+        .type = TST_X86,
+#elif defined(__ia64__)
+        .name = "ia64",
+        .type = TST_IA64,
+#elif defined(__powerpc64__) || defined(__ppc64__)
+        .name = "ppc64",
+        .type = TST_PPC64,
+#elif defined(__powerpc__) || defined(__ppc__)
+        .name = "ppc",
+        .type = TST_PPC,
+#elif defined(__s390x__)
+        .name = "s390x",
+        .type = TST_S390X,
+#elif defined(__s390__)
+        .name = "s390",
+        .type = TST_S390,
+#elif defined(__aarch64__)
+        .name = "aarch64",
+        .type = TST_AARCH64,
+#elif defined(__arm__)
+        .name = "arm",
+        .type = TST_ARM,
+#elif defined(__sparc__)
+        .name = "sparc",
+        .type = TST_SPARC,
+#else
+        .name = "unknown",
+        .type = TST_UNKNOWN,
+#endif
+};
+
+static const char *const arch_type_list[] = {
+	"i386",
+	"i586",
+	"i686",
+	"x86_64",
+	"ia64",
+	"ppc",
+	"ppc64",
+	"s390",
+	"s390x",
+	"arm",
+	"aarch64",
+	"sparc",
+	NULL
+};
+
+static int is_valid_arch_name(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; arch_type_list[i]; i++) {
+		if (!strcmp(arch_type_list[i], name))
+			return 1;
+	}
+
+	return 0;
+}
+
+int tst_is_on_arch(const char *const *archlist)
+{
+	unsigned int i;
+
+	if (!archlist)
+		return 1;
+
+	for (i = 0; archlist[i]; i++) {
+		if (!is_valid_arch_name(archlist[i]))
+			tst_brk(TBROK, "%s is invalid arch, please reset!",
+					archlist[i]);
+	}
+
+	for (i = 0; archlist[i]; i++) {
+		if (!strcmp(tst_arch.name, archlist[i]))
+			return 1;
+	}
+
+	return 0;
+}
diff --git a/lib/tst_test.c b/lib/tst_test.c
index d7f9e0f97..a79275722 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -976,6 +976,9 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->min_kver)
 		check_kver();
 
+	if (tst_test->supported_archs && !tst_is_on_arch(tst_test->supported_archs))
+		tst_brk(TCONF, "This arch '%s' is not supported for test!", tst_arch.name);
+
 	if (tst_test->skip_in_lockdown && tst_lockdown_enabled())
 		tst_brk(TCONF, "Kernel is locked down, skipping test");
 
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [LTP] [PATCH v5 2/3] testcase: make use of .supported_archs
  2021-11-17  7:07 [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Li Wang
@ 2021-11-17  7:07 ` Li Wang
  2021-11-17  7:07 ` [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch Li Wang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 18+ messages in thread
From: Li Wang @ 2021-11-17  7:07 UTC (permalink / raw)
  To: ltp

In some places, we have to keep ifdefs to make the compiler work
with unportable code.

Signed-off-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
 configure.ac                                  |  1 +
 testcases/cve/meltdown.c                      | 16 ++++++-----
 testcases/kernel/syscalls/ptrace/ptrace07.c   | 11 ++++----
 testcases/kernel/syscalls/ptrace/ptrace08.c   | 22 ++++++++-------
 testcases/kernel/syscalls/ptrace/ptrace09.c   | 11 +++++---
 testcases/kernel/syscalls/ptrace/ptrace10.c   | 12 +++++----
 .../syscalls/set_mempolicy/set_mempolicy05.c  | 27 +++++++++----------
 7 files changed, 54 insertions(+), 46 deletions(-)

diff --git a/configure.ac b/configure.ac
index 859aa9857..9af32c859 100644
--- a/configure.ac
+++ b/configure.ac
@@ -42,6 +42,7 @@ AC_CHECK_DECLS([SEM_STAT_ANY],,,[#include <sys/sem.h>])
 
 AC_CHECK_HEADERS_ONCE([ \
     asm/ldt.h \
+    emmintrin.h \
     ifaddrs.h \
     keyutils.h \
     linux/can.h \
diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
index 5a984aba3..2577c1a80 100644
--- a/testcases/cve/meltdown.c
+++ b/testcases/cve/meltdown.c
@@ -6,8 +6,6 @@
 #include "config.h"
 #include "tst_test.h"
 
-#if defined(__x86_64__) || defined(__i386__)
-
 #include <stdio.h>
 #include <string.h>
 #include <signal.h>
@@ -17,6 +15,7 @@
 #include <ctype.h>
 #include <sys/utsname.h>
 
+#ifdef HAVE_EMMINTRIN_H
 #include <emmintrin.h>
 
 #include "tst_tsc.h"
@@ -378,14 +377,17 @@ static struct tst_test test = {
 	.test_all = run,
 	.cleanup = cleanup,
 	.min_kver = "2.6.32",
+	.supported_archs = (const char *const []) {
+		"i386",
+		"x86_64",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"CVE", "2017-5754"},
 		{}
 	}
 };
 
-#else /* #if defined(__x86_64__) || defined(__i386__) */
-
-TST_TEST_TCONF("not x86_64 or i386");
-
-#endif /* #else #if defined(__x86_64__) || defined(__i386__) */
+#else /* HAVE_EMMINTRIN_H */
+	TST_TEST_TCONF("<emmintrin.h> is not supported");
+#endif
diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c b/testcases/kernel/syscalls/ptrace/ptrace07.c
index 9e3f7511d..da62cadb0 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace07.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
@@ -48,13 +48,13 @@
 # define NT_X86_XSTATE 0x202
 #endif
 
-#ifdef __x86_64__
 static void check_regs_loop(uint32_t initval)
 {
 	const unsigned long num_iters = 1000000000;
 	uint32_t xmm0[4] = { initval, initval, initval, initval };
 	int status = 1;
 
+#ifdef __x86_64__
 	asm volatile("   movdqu %0, %%xmm0\n"
 		     "   mov %0, %%rbx\n"
 		     "1: dec %2\n"
@@ -68,6 +68,7 @@ static void check_regs_loop(uint32_t initval)
 		     "3:\n"
 		     : "+m" (xmm0), "+r" (status)
 		     : "r" (num_iters) : "rax", "rbx", "xmm0");
+#endif
 
 	if (status) {
 		tst_res(TFAIL,
@@ -178,6 +179,10 @@ static struct tst_test test = {
 	.test_all = do_test,
 	.forks_child = 1,
 	.needs_checkpoints = 1,
+	.supported_archs = (const char *const []) {
+		"x86_64",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "814fb7bb7db5"},
 		{"CVE", "2017-15537"},
@@ -185,7 +190,3 @@ static struct tst_test test = {
 	}
 
 };
-
-#else /* !__x86_64__ */
-	TST_TEST_TCONF("this test is only supported on x86_64");
-#endif /* __x86_64__ */
diff --git a/testcases/kernel/syscalls/ptrace/ptrace08.c b/testcases/kernel/syscalls/ptrace/ptrace08.c
index 170cae64c..20df39a9b 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace08.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace08.c
@@ -43,18 +43,16 @@
 #include "tst_test.h"
 #include "tst_safe_stdio.h"
 
-#if defined(__i386__) || defined(__x86_64__)
-
 static pid_t child_pid;
 
-#if defined(__x86_64__)
-# define KERN_ADDR_MIN 0xffff800000000000
-# define KERN_ADDR_MAX 0xffffffffffffffff
-# define KERN_ADDR_BITS 64
-#elif defined(__i386__)
+#if defined(__i386__)
 # define KERN_ADDR_MIN 0xc0000000
 # define KERN_ADDR_MAX 0xffffffff
 # define KERN_ADDR_BITS 32
+#else
+# define KERN_ADDR_MIN 0xffff800000000000
+# define KERN_ADDR_MAX 0xffffffffffffffff
+# define KERN_ADDR_BITS 64
 #endif
 
 static int deffered_check;
@@ -98,6 +96,7 @@ static void ptrace_try_kern_addr(unsigned long kern_addr)
 	if (SAFE_WAITPID(child_pid, &status, WUNTRACED) != child_pid)
 		tst_brk(TBROK, "Received event from unexpected PID");
 
+#if defined(__i386__) || defined(__x86_64__)
 	SAFE_PTRACE(PTRACE_ATTACH, child_pid, NULL, NULL);
 	SAFE_PTRACE(PTRACE_POKEUSER, child_pid,
 		(void *)offsetof(struct user, u_debugreg[0]), (void *)1);
@@ -127,6 +126,7 @@ static void ptrace_try_kern_addr(unsigned long kern_addr)
 
 	addr = ptrace(PTRACE_PEEKUSER, child_pid,
 	              (void*)offsetof(struct user, u_debugreg[0]), NULL);
+#endif
 
 	if (!deffered_check && addr == kern_addr)
 		tst_res(TFAIL, "Was able to set breakpoint on kernel addr");
@@ -161,6 +161,11 @@ static struct tst_test test = {
 	 * have to skip the test.
 	 */
 	.skip_in_compat = 1,
+	.supported_archs = (const char *const []) {
+		"i386",
+		"x86_64",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "f67b15037a7a"},
 		{"CVE", "2018-1000199"},
@@ -168,6 +173,3 @@ static struct tst_test test = {
 		{}
 	}
 };
-#else
-TST_TEST_TCONF("This test is only supported on x86 systems");
-#endif
diff --git a/testcases/kernel/syscalls/ptrace/ptrace09.c b/testcases/kernel/syscalls/ptrace/ptrace09.c
index 85875ce65..8ccdfcc4b 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace09.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace09.c
@@ -22,7 +22,6 @@
 #include <signal.h>
 #include "tst_test.h"
 
-#if defined(__i386__) || defined(__x86_64__)
 static short watchpoint;
 static pid_t child_pid;
 
@@ -46,6 +45,7 @@ static void run(void)
 {
 	int status;
 
+#if defined(__i386__) || defined(__x86_64__)
 	child_pid = SAFE_FORK();
 
 	if (!child_pid) {
@@ -60,6 +60,7 @@ static void run(void)
 	SAFE_PTRACE(PTRACE_POKEUSER, child_pid,
 		(void *)offsetof(struct user, u_debugreg[7]), (void *)0x30001);
 	SAFE_PTRACE(PTRACE_CONT, child_pid, NULL, NULL);
+#endif
 
 	while (1) {
 		if (SAFE_WAITPID(child_pid, &status, 0) != child_pid)
@@ -92,12 +93,14 @@ static struct tst_test test = {
 	.test_all = run,
 	.cleanup = cleanup,
 	.forks_child = 1,
+	.supported_archs = (const char *const []) {
+		"i386",
+		"x86_64",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "d8ba61ba58c8"},
 		{"CVE", "2018-8897"},
 		{}
 	}
 };
-#else
-TST_TEST_TCONF("This test is only supported on x86 systems");
-#endif
diff --git a/testcases/kernel/syscalls/ptrace/ptrace10.c b/testcases/kernel/syscalls/ptrace/ptrace10.c
index b5d6b9f8f..3bd8ca1a9 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace10.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace10.c
@@ -22,8 +22,6 @@
 #include <signal.h>
 #include "tst_test.h"
 
-#if defined(__i386__) || defined(__x86_64__)
-
 static pid_t child_pid;
 
 static void child_main(void)
@@ -45,6 +43,7 @@ static void run(void)
 	if (SAFE_WAITPID(child_pid, &status, WUNTRACED) != child_pid)
 		tst_brk(TBROK, "Received event from unexpected PID");
 
+#if defined(__i386__) || defined(__x86_64__)
 	SAFE_PTRACE(PTRACE_ATTACH, child_pid, NULL, NULL);
 	SAFE_PTRACE(PTRACE_POKEUSER, child_pid,
 		(void *)offsetof(struct user, u_debugreg[0]), (void *)1);
@@ -53,6 +52,7 @@ static void run(void)
 
 	addr = ptrace(PTRACE_PEEKUSER, child_pid,
 	              (void*)offsetof(struct user, u_debugreg[0]), NULL);
+#endif
 
 	if (addr == 2)
 		tst_res(TPASS, "The rd0 was set on second PTRACE_POKEUSR");
@@ -76,11 +76,13 @@ static struct tst_test test = {
 	.test_all = run,
 	.cleanup = cleanup,
 	.forks_child = 1,
+	.supported_archs = (const char *const []) {
+		"i386",
+		"x86_64",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "bd14406b78e6"},
 		{}
 	}
 };
-#else
-TST_TEST_TCONF("This test is only supported on x86 systems");
-#endif
diff --git a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy05.c b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy05.c
index 86f6a95dc..c65cb1e18 100644
--- a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy05.c
+++ b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy05.c
@@ -37,18 +37,10 @@
 #include "config.h"
 #include "tst_test.h"
 
-#if defined(__i386__) || defined(__powerpc__)
-
 #include <string.h>
 
 static unsigned int i;
 static int sys_ret;
-#ifdef __i386__
-static const int sys_num = 276;
-static const int mode;
-static const int node_mask_ptr = UINT_MAX;
-static const int node_mask_sz = UINT_MAX;
-#endif
 static volatile char *stack_ptr;
 
 static void run(void)
@@ -58,6 +50,11 @@ static void run(void)
 	register long mode __asm__("r3");
 	register long node_mask_ptr __asm__("r4");
 	register long node_mask_sz __asm__("r5");
+#else
+	const int sys_num = 276;
+	const int mode;
+	const int node_mask_ptr = UINT_MAX;
+	const int node_mask_sz = UINT_MAX;
 #endif
 	char stack_pattern[0x400];
 
@@ -78,7 +75,8 @@ static void run(void)
 		:
 		"memory", "cr0", "r6", "r7", "r8", "r9", "r10", "r11", "r12");
 	sys_ret = mode;
-#else /* __i386__ */
+#endif
+#ifdef __i386__
 	asm volatile (
 		"add $0x400, %%esp\n\t"
 		"int $0x80\n\t"
@@ -114,15 +112,14 @@ static void run(void)
 
 static struct tst_test test = {
 	.test_all = run,
+	.supported_archs = (const char *const []) {
+		"i386",
+		"ppc",
+		NULL
+	},
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "cf01fb9985e8"},
 		{"CVE", "CVE-2017-7616"},
 		{}
 	}
 };
-
-#else /* #if defined(__x86_64__) || defined(__powerpc__) */
-
-TST_TEST_TCONF("not i386 or powerpc");
-
-#endif /* #else #if defined(__x86_64__) || defined(__powerpc__) */
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch
  2021-11-17  7:07 [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Li Wang
  2021-11-17  7:07 ` [LTP] [PATCH v5 2/3] testcase: make use of .supported_archs Li Wang
@ 2021-11-17  7:07 ` Li Wang
  2021-11-17 10:33   ` Richard Palethorpe
  2021-11-17  9:58 ` [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Richard Palethorpe
  2021-11-17 13:59 ` Joerg Vehlow
  3 siblings, 1 reply; 18+ messages in thread
From: Li Wang @ 2021-11-17  7:07 UTC (permalink / raw)
  To: ltp

Signed-off-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/mem/tunable/max_map_count.c | 46 ++++++++++----------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/testcases/kernel/mem/tunable/max_map_count.c b/testcases/kernel/mem/tunable/max_map_count.c
index 4f0ad0037..a4c3dbf8e 100644
--- a/testcases/kernel/mem/tunable/max_map_count.c
+++ b/testcases/kernel/mem/tunable/max_map_count.c
@@ -55,7 +55,6 @@
 
 static long old_max_map_count = -1;
 static long old_overcommit = -1;
-static struct utsname un;
 
 static void setup(void)
 {
@@ -66,9 +65,6 @@ static void setup(void)
 	old_max_map_count = get_sys_tune("max_map_count");
 	old_overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 0, 1);
-
-	if (uname(&un) != 0)
-		tst_brk(TBROK | TERRNO, "uname error");
 }
 
 static void cleanup(void)
@@ -91,24 +87,30 @@ static bool filter_map(const char *line)
 	if (ret != 1)
 		return false;
 
-#if defined(__x86_64__) || defined(__x86__)
-	/* On x86, there's an old compat vsyscall page */
-	if (!strcmp(buf, "[vsyscall]"))
-		return true;
-#elif defined(__ia64__)
-	/* On ia64, the vdso is not a proper mapping */
-	if (!strcmp(buf, "[vdso]"))
-		return true;
-#elif defined(__arm__)
-	/* Skip it when run it in aarch64 */
-	if ((!strcmp(un.machine, "aarch64"))
-	|| (!strcmp(un.machine, "aarch64_be")))
-		return false;
-
-	/* Older arm kernels didn't label their vdso maps */
-	if (!strncmp(line, "ffff0000-ffff1000", 17))
-		return true;
-#endif
+	switch (tst_arch.type) {
+	case TST_X86:
+	case TST_X86_64:
+		/* On x86, there's an old compat vsyscall page */
+		if (!strcmp(buf, "[vsyscall]"))
+			return true;
+		break;
+	case TST_IA64:
+		/* On ia64, the vdso is not a proper mapping */
+		if (!strcmp(buf, "[vdso]"))
+			return true;
+		break;
+	case TST_ARM:
+		/* Skip it when run it in aarch64 */
+		if (tst_kernel_bits() == 64)
+			return false;
+
+		/* Older arm kernels didn't label their vdso maps */
+		if (!strncmp(line, "ffff0000-ffff1000", 17))
+			return true;
+		break;
+	default:
+		break;
+	};
 
 	return false;
 }
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17  7:07 [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Li Wang
  2021-11-17  7:07 ` [LTP] [PATCH v5 2/3] testcase: make use of .supported_archs Li Wang
  2021-11-17  7:07 ` [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch Li Wang
@ 2021-11-17  9:58 ` Richard Palethorpe
  2021-11-17 13:51   ` Li Wang
  2021-11-17 13:59 ` Joerg Vehlow
  3 siblings, 1 reply; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-17  9:58 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Testcases for specific arch should be limited on that only being supported
> platform to run, we now involve a .supported_archs to achieve this feature
> in LTP library. All you need to run a test on the expected arch is to set
> the '.supported_archs' array in the 'struct tst_test' to choose the required
> arch list. e.g.
>
>     .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
>
> This helps move the TCONF info from code to tst_test metadata as well.
>
> And, we also export a struct tst_arch to save the system architecture
> for using in the whole test cases.
>
>     extern const struct tst_arch {
>              char name[16];
>              enum tst_arch_type type;
>     } tst_arch;
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  doc/c-test-api.txt | 36 +++++++++++++++++
>  include/tst_arch.h | 39 +++++++++++++++++++
>  include/tst_test.h |  7 ++++
>  lib/tst_arch.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
>  lib/tst_test.c     |  3 ++
>  5 files changed, 181 insertions(+)
>  create mode 100644 include/tst_arch.h
>  create mode 100644 lib/tst_arch.c
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 3127018a4..64d0630ce 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -2261,6 +2261,42 @@ struct tst_test test = {
>  };
>  -------------------------------------------------------------------------------
>  
> +1.39 Testing on the specific architecture
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Testcases for specific arch should be limited on that only being supported
> +platform to run, we now involve a .supported_archs to achieve this feature
> +in LTP library. All you need to run a test on the expected arch is to set
> +the '.supported_archs' array in the 'struct tst_test' to choose the required
> +arch list. e.g.
> +
> +    .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
> +
> +This helps move the TCONF info from code to tst_test metadata as well.
> +
> +And, we also export a struct tst_arch to save the system architecture for
> +using in the whole test cases.
> +
> +    extern const struct tst_arch {
> +             char name[16];
> +             enum tst_arch_type type;
> +    } tst_arch;
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +#include "tst_test.h"
> +
> +static struct tst_test test = {
> +       ...
> +       .setup = setup,
> +       .supported_archs = (const char *const []) {
> +                 "x86_64",
> +                 "ppc64",
> +                 "s390x",
> +                 NULL
> +       },
> +};
> +-------------------------------------------------------------------------------
> +
>  2. Common problems
>  ------------------
>  
> diff --git a/include/tst_arch.h b/include/tst_arch.h
> new file mode 100644
> index 000000000..4a9473c6f
> --- /dev/null
> +++ b/include/tst_arch.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (c) 2021 Li Wang <liwang@redhat.com>
> + */
> +
> +#ifndef TST_ARCH_H__
> +#define TST_ARCH_H__
> +
> +enum tst_arch_type {
> +	TST_UNKNOWN,
> +	TST_X86,
> +	TST_X86_64,
> +	TST_IA64,
> +	TST_PPC,
> +	TST_PPC64,
> +	TST_S390,
> +	TST_S390X,
> +	TST_ARM,
> +	TST_AARCH64,
> +	TST_SPARC,
> +};
> +
> +/*
> + * This tst_arch is to save the system architecture for
> + * using in the whole testcase.
> + */
> +extern const struct tst_arch {
> +        char name[16];
> +        enum tst_arch_type type;
> +} tst_arch;
> +
> +/*
> + * Check if test platform is in the given arch list. If yes return 1,
> + * otherwise return 0.
> + *
> + * @archlist A NULL terminated array of architectures to support.
> + */
> +int tst_is_on_arch(const char *const *archlist);
> +
> +#endif /* TST_ARCH_H__ */
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 602ce3090..c06a4729b 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -43,6 +43,7 @@
>  #include "tst_fips.h"
>  #include "tst_taint.h"
>  #include "tst_memutils.h"
> +#include "tst_arch.h"
>  
>  /*
>   * Reports testcase result.
> @@ -139,6 +140,12 @@ struct tst_test {
>  
>  	const char *min_kver;
>  
> +	/*
> +	 * The supported_archs is a NULL terminated list of archs the test
> +	 * does support.
> +	 */
> +	const char *const *supported_archs;
> +
>  	/* If set the test is compiled out */
>  	const char *tconf_msg;
>  
> diff --git a/lib/tst_arch.c b/lib/tst_arch.c
> new file mode 100644
> index 000000000..f19802a03
> --- /dev/null
> +++ b/lib/tst_arch.c
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (c) 2021 Li Wang <liwang@redhat.com>
> + */
> +
> +#include <string.h>
> +#include <stdlib.h>
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_arch.h"
> +#include "tst_test.h"
> +
> +const struct tst_arch tst_arch = {
> +#if defined(__x86_64__)
> +        .name = "x86_64",

Writing out these string constants multiple times is error
prone. Perhaps arch_type_list can be indexed with enum tst_arch_type and
then name can be ".name = arch_type_list[TST_X86]"?

> +        .type = TST_X86_64,
> +#elif defined(__i386__) || defined(__i586__) || defined(__i686__)
> +        .name = "x86",
> +        .type = TST_X86,
> +#elif defined(__ia64__)
> +        .name = "ia64",
> +        .type = TST_IA64,
> +#elif defined(__powerpc64__) || defined(__ppc64__)
> +        .name = "ppc64",
> +        .type = TST_PPC64,
> +#elif defined(__powerpc__) || defined(__ppc__)
> +        .name = "ppc",
> +        .type = TST_PPC,
> +#elif defined(__s390x__)
> +        .name = "s390x",
> +        .type = TST_S390X,
> +#elif defined(__s390__)
> +        .name = "s390",
> +        .type = TST_S390,
> +#elif defined(__aarch64__)
> +        .name = "aarch64",
> +        .type = TST_AARCH64,
> +#elif defined(__arm__)
> +        .name = "arm",
> +        .type = TST_ARM,
> +#elif defined(__sparc__)
> +        .name = "sparc",
> +        .type = TST_SPARC,
> +#else
> +        .name = "unknown",
> +        .type = TST_UNKNOWN,
> +#endif
> +};
> +
> +static const char *const arch_type_list[] = {
> +	"i386",
> +	"i586",
> +	"i686",

These are not valid arch names AFAICT. There is no mapping from these to
x86 in the tst_arch table above.

Perhaps we could replace them all with x86?

e.g [TST_X86] = "x86",
    [TST_X86_64] = "x86_64",
    etc...

> +	"x86_64",
> +	"ia64",
> +	"ppc",
> +	"ppc64",
> +	"s390",
> +	"s390x",
> +	"arm",
> +	"aarch64",
> +	"sparc",
> +	NULL
> +};
> +

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch
  2021-11-17  7:07 ` [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch Li Wang
@ 2021-11-17 10:33   ` Richard Palethorpe
  2021-11-17 13:28     ` Li Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-17 10:33 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Signed-off-by: Li Wang <liwang@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/mem/tunable/max_map_count.c | 46 ++++++++++----------
>  1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/testcases/kernel/mem/tunable/max_map_count.c b/testcases/kernel/mem/tunable/max_map_count.c
> index 4f0ad0037..a4c3dbf8e 100644
> --- a/testcases/kernel/mem/tunable/max_map_count.c
> +++ b/testcases/kernel/mem/tunable/max_map_count.c
> @@ -55,7 +55,6 @@
>  
>  static long old_max_map_count = -1;
>  static long old_overcommit = -1;
> -static struct utsname un;
>  
>  static void setup(void)
>  {
> @@ -66,9 +65,6 @@ static void setup(void)
>  	old_max_map_count = get_sys_tune("max_map_count");
>  	old_overcommit = get_sys_tune("overcommit_memory");
>  	set_sys_tune("overcommit_memory", 0, 1);
> -
> -	if (uname(&un) != 0)
> -		tst_brk(TBROK | TERRNO, "uname error");
>  }
>  
>  static void cleanup(void)
> @@ -91,24 +87,30 @@ static bool filter_map(const char *line)
>  	if (ret != 1)
>  		return false;
>  
> -#if defined(__x86_64__) || defined(__x86__)
> -	/* On x86, there's an old compat vsyscall page */
> -	if (!strcmp(buf, "[vsyscall]"))
> -		return true;
> -#elif defined(__ia64__)
> -	/* On ia64, the vdso is not a proper mapping */
> -	if (!strcmp(buf, "[vdso]"))
> -		return true;
> -#elif defined(__arm__)
> -	/* Skip it when run it in aarch64 */
> -	if ((!strcmp(un.machine, "aarch64"))
> -	|| (!strcmp(un.machine, "aarch64_be")))
> -		return false;
> -
> -	/* Older arm kernels didn't label their vdso maps */
> -	if (!strncmp(line, "ffff0000-ffff1000", 17))
> -		return true;
> -#endif
> +	switch (tst_arch.type) {
> +	case TST_X86:
> +	case TST_X86_64:
> +		/* On x86, there's an old compat vsyscall page */
> +		if (!strcmp(buf, "[vsyscall]"))
> +			return true;
> +		break;
> +	case TST_IA64:
> +		/* On ia64, the vdso is not a proper mapping */
> +		if (!strcmp(buf, "[vdso]"))
> +			return true;
> +		break;
> +	case TST_ARM:
> +		/* Skip it when run it in aarch64 */

This should not be possible. If TST_ARM is set then how can we be on
aarch64? We also have TST_AARCH64.

> +		if (tst_kernel_bits() == 64)
> +			return false;
> +
> +		/* Older arm kernels didn't label their vdso maps */
> +		if (!strncmp(line, "ffff0000-ffff1000", 17))
> +			return true;
> +		break;
> +	default:
> +		break;
> +	};
>  
>  	return false;
>  }
> -- 
> 2.31.1


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch
  2021-11-17 10:33   ` Richard Palethorpe
@ 2021-11-17 13:28     ` Li Wang
  2021-11-17 13:58       ` Richard Palethorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Li Wang @ 2021-11-17 13:28 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 632 bytes --]

Hi Richard,



> > +     case TST_IA64:
> > +             /* On ia64, the vdso is not a proper mapping */
> > +             if (!strcmp(buf, "[vdso]"))
> > +                     return true;
> > +             break;
> > +     case TST_ARM:
> > +             /* Skip it when run it in aarch64 */
>
> This should not be possible. If TST_ARM is set then how can we be on
> aarch64? We also have TST_AARCH64.
>

Not exactly, I was thinking like this before, but as Cyril point that there
is
a possible 32bit ARM binary runs on 64bit aarch64 kernel.

https://lists.linux.it/pipermail/ltp/2021-November/025925.html


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1430 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17  9:58 ` [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Richard Palethorpe
@ 2021-11-17 13:51   ` Li Wang
  2021-11-17 14:01     ` Richard Palethorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Li Wang @ 2021-11-17 13:51 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 1104 bytes --]

Hi Richard,


> > +const struct tst_arch tst_arch = {
> > +#if defined(__x86_64__)
> > +        .name = "x86_64",
>
> Writing out these string constants multiple times is error
> prone. Perhaps arch_type_list can be indexed with enum tst_arch_type and
> then name can be ".name = arch_type_list[TST_X86]"?
>

Right, that will more flexible but you know, all arches we have are just
those, and we write them only once in the LTP test library.

I slightly wanted to keep string constants to make it more simple/readable.


> > +static const char *const arch_type_list[] = {
> > +     "i386",
> > +     "i586",
> > +     "i686",
>
> These are not valid arch names AFAICT. There is no mapping from these to
> x86 in the tst_arch table above.
>
> Perhaps we could replace them all with x86?
>

Yes, that is also the unsure part I was concerned about in patch v4.
Because x86 is also an invalid arch (it is just a conventional name),
if we use it in the arch_type_list we have to recognize it as a valid arch
name in test case as well. I'm not sure that will be accepted by other
people.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2336 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch
  2021-11-17 13:28     ` Li Wang
@ 2021-11-17 13:58       ` Richard Palethorpe
  0 siblings, 0 replies; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-17 13:58 UTC (permalink / raw)
  To: Li Wang; +Cc: LTP List

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
>  
>  > +     case TST_IA64:
>  > +             /* On ia64, the vdso is not a proper mapping */
>  > +             if (!strcmp(buf, "[vdso]"))
>  > +                     return true;
>  > +             break;
>  > +     case TST_ARM:
>  > +             /* Skip it when run it in aarch64 */
>
>  This should not be possible. If TST_ARM is set then how can we be on
>  aarch64? We also have TST_AARCH64.
>
> Not exactly, I was thinking like this before, but as Cyril point that there is
> a possible 32bit ARM binary runs on 64bit aarch64 kernel.
>
> https://lists.linux.it/pipermail/ltp/2021-November/025925.html

Thanks

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17  7:07 [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Li Wang
                   ` (2 preceding siblings ...)
  2021-11-17  9:58 ` [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Richard Palethorpe
@ 2021-11-17 13:59 ` Joerg Vehlow
  2021-11-17 14:25   ` Richard Palethorpe
  2021-11-18 12:07   ` Cyril Hrubis
  3 siblings, 2 replies; 18+ messages in thread
From: Joerg Vehlow @ 2021-11-17 13:59 UTC (permalink / raw)
  To: Li Wang, ltp

Hi,

On 11/17/2021 8:07 AM, Li Wang wrote:
> Testcases for specific arch should be limited on that only being supported
> platform to run, we now involve a .supported_archs to achieve this feature
> in LTP library. All you need to run a test on the expected arch is to set
> the '.supported_archs' array in the 'struct tst_test' to choose the required
> arch list. e.g.
>
>      .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
>
> This helps move the TCONF info from code to tst_test metadata as well.

while I do like this, I wonder if it wouldn't be better to do this using 
kernel config. IIRC there are config switches
for all architectures. Further more this would allow adding more complex 
conditions in the future.

E.g: I am pretty sure, that there are some syscalls, that have existed 
"forever" in x86_64, but where only added
in a specific version for aarch64. By making the arch a separate option, 
there is no way, to model this.
If it was done in the kernel config check, it could be possible to add 
version and arch checks like
(CONFIG_AARCH64 && CONFIG_VERSION > 5.3) || CONFIG_X86_64

While this probably does not produce a very good error message, it is 
more versatile.

Sorry for this late questioning the whole approach.

Joerg

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17 13:51   ` Li Wang
@ 2021-11-17 14:01     ` Richard Palethorpe
  2021-11-18 11:42       ` Cyril Hrubis
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-17 14:01 UTC (permalink / raw)
  To: Li Wang; +Cc: LTP List

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>  
>  > +const struct tst_arch tst_arch = {
>  > +#if defined(__x86_64__)
>  > +        .name = "x86_64",
>
>  Writing out these string constants multiple times is error
>  prone. Perhaps arch_type_list can be indexed with enum tst_arch_type and
>  then name can be ".name = arch_type_list[TST_X86]"?
>
> Right, that will more flexible but you know, all arches we have are just
> those, and we write them only once in the LTP test library.
>
> I slightly wanted to keep string constants to make it more
> simple/readable.

It's a minor thing, but IMO the extra complication is worth it to
eliminate typos.

Although the preprocessor could still hide errors on some arch until
someone tries to compile it.

>  
>  > +static const char *const arch_type_list[] = {
>  > +     "i386",
>  > +     "i586",
>  > +     "i686",
>
>  These are not valid arch names AFAICT. There is no mapping from these to
>  x86 in the tst_arch table above.
>
>  Perhaps we could replace them all with x86?
>
> Yes, that is also the unsure part I was concerned about in patch v4. 
> Because x86 is also an invalid arch (it is just a conventional name),
> if we use it in the arch_type_list we have to recognize it as a valid arch
> name in test case as well. I'm not sure that will be accepted by other
> people.

The folder in the kernel tree is arch/x86.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17 13:59 ` Joerg Vehlow
@ 2021-11-17 14:25   ` Richard Palethorpe
  2021-11-18  5:50     ` Li Wang
  2021-11-18 12:07   ` Cyril Hrubis
  1 sibling, 1 reply; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-17 14:25 UTC (permalink / raw)
  To: Joerg Vehlow; +Cc: ltp

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi,
>
> On 11/17/2021 8:07 AM, Li Wang wrote:
>> Testcases for specific arch should be limited on that only being supported
>> platform to run, we now involve a .supported_archs to achieve this feature
>> in LTP library. All you need to run a test on the expected arch is to set
>> the '.supported_archs' array in the 'struct tst_test' to choose the required
>> arch list. e.g.
>>
>>      .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
>>
>> This helps move the TCONF info from code to tst_test metadata as well.
>
> while I do like this, I wonder if it wouldn't be better to do this
> using kernel config. IIRC there are config switches
> for all architectures. Further more this would allow adding more
> complex conditions in the future.
>
> E.g: I am pretty sure, that there are some syscalls, that have existed
> "forever" in x86_64, but where only added
> in a specific version for aarch64. By making the arch a separate
> option, there is no way, to model this.
> If it was done in the kernel config check, it could be possible to add
> version and arch checks like
> (CONFIG_AARCH64 && CONFIG_VERSION > 5.3) || CONFIG_X86_64
>
> While this probably does not produce a very good error message, it is
> more versatile.
>
> Sorry for this late questioning the whole approach.

It should never be too late IMO. We should also consider whether there
are existing tst_test flags which have been made redundant by kconfig.

I suspect the main issue would be meta-data. As an arbitrarily
complicated kconfig expression may confuse test harnesses.

It might be better for us to tackle that issue and use kconfig though.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17 14:25   ` Richard Palethorpe
@ 2021-11-18  5:50     ` Li Wang
  0 siblings, 0 replies; 18+ messages in thread
From: Li Wang @ 2021-11-18  5:50 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 3072 bytes --]

Hi Joerg, Richard,

On Wed, Nov 17, 2021 at 10:37 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello Joerg,
>
> Joerg Vehlow <lkml@jv-coder.de> writes:
>
> > Hi,
> >
> > On 11/17/2021 8:07 AM, Li Wang wrote:
> >> Testcases for specific arch should be limited on that only being
> supported
> >> platform to run, we now involve a .supported_archs to achieve this
> feature
> >> in LTP library. All you need to run a test on the expected arch is to
> set
> >> the '.supported_archs' array in the 'struct tst_test' to choose the
> required
> >> arch list. e.g.
> >>
> >>      .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
> >>
> >> This helps move the TCONF info from code to tst_test metadata as well.
> >
> > while I do like this, I wonder if it wouldn't be better to do this
> > using kernel config. IIRC there are config switches
> > for all architectures. Further more this would allow adding more
> > complex conditions in the future.
> >
> > E.g: I am pretty sure, that there are some syscalls, that have existed
> > "forever" in x86_64, but where only added
> > in a specific version for aarch64. By making the arch a separate
> > option, there is no way, to model this.
>

Umm, we shouldn't set .supported_archs to make it a separate option
unless we have an explicit requirement on that. In other words, without
that .supported_archs setting, it will support all arches by default and we
can do anything by the exported tst_arch structure and enum tst_arch_type.



> > If it was done in the kernel config check, it could be possible to add
> > version and arch checks like
> > (CONFIG_AARCH64 && CONFIG_VERSION > 5.3) || CONFIG_X86_64
>

We definitely can achieve this in the current version as well.

    switch (tst_arch.type)
    case TST_AARCH64:
            if ((tst_kvercmp(5, 3, 0)) < 0)
                    break;
    case TST_X86_64:
            ltp_syscall(__NR_special_syscall, ...)
    break;
        ...



> >
> > While this probably does not produce a very good error message, it is
> > more versatile.
> >
> > Sorry for this late questioning the whole approach.
>
> It should never be too late IMO. We should also consider whether there
> are existing tst_test flags which have been made redundant by kconfig.
>

After checking the config file again, yes, I agree that we can achieve the
same thing just with existing kconfig functions to parse it.

E.g. If required x86_64:

    .needs_kconfigs = (const char *[]) {
        "CONFIG_X86_64=y",
        NULL
    },

and s390x will be like:

    .needs_kconfigs = (const char *[]) {
        "CONFIG_64BIT=y",
        "CONFIG_S390=y"
        NULL
    },

...


>
> I suspect the main issue would be meta-data. As an arbitrarily
> complicated kconfig expression may confuse test harnesses.
>

Right, so I would like to keep the .supported_archs and tst_arch structure
no change, even though goes with parsing config file in the library.



>
> It might be better for us to tackle that issue and use kconfig though.
>
> --
> Thank you,
> Richard.
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 6394 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17 14:01     ` Richard Palethorpe
@ 2021-11-18 11:42       ` Cyril Hrubis
  0 siblings, 0 replies; 18+ messages in thread
From: Cyril Hrubis @ 2021-11-18 11:42 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: LTP List

Hi!
> >  These are not valid arch names AFAICT. There is no mapping from these to
> >  x86 in the tst_arch table above.
> >
> >  Perhaps we could replace them all with x86?
> >
> > Yes, that is also the unsure part I was concerned about in patch v4. 
> > Because x86 is also an invalid arch (it is just a conventional name),
> > if we use it in the arch_type_list we have to recognize it as a valid arch
> > name in test case as well. I'm not sure that will be accepted by other
> > people.
> 
> The folder in the kernel tree is arch/x86.

The x86 is basically a wildcard where the x could be substitued with any
of {3,4,5,6,7}. Also I think that 786 was last 32bit intel arch ever
released.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-17 13:59 ` Joerg Vehlow
  2021-11-17 14:25   ` Richard Palethorpe
@ 2021-11-18 12:07   ` Cyril Hrubis
  2021-11-22  2:21     ` Li Wang
  1 sibling, 1 reply; 18+ messages in thread
From: Cyril Hrubis @ 2021-11-18 12:07 UTC (permalink / raw)
  To: Joerg Vehlow; +Cc: ltp

Hi!
> > Testcases for specific arch should be limited on that only being supported
> > platform to run, we now involve a .supported_archs to achieve this feature
> > in LTP library. All you need to run a test on the expected arch is to set
> > the '.supported_archs' array in the 'struct tst_test' to choose the required
> > arch list. e.g.
> >
> >      .supported_archs = (const char *const []){"x86_64", "ppc64", NULL}
> >
> > This helps move the TCONF info from code to tst_test metadata as well.
> 
> while I do like this, I wonder if it wouldn't be better to do this using 
> kernel config. IIRC there are config switches
> for all architectures. Further more this would allow adding more complex 
> conditions in the future.
> 
> E.g: I am pretty sure, that there are some syscalls, that have existed 
> "forever" in x86_64, but where only added
> in a specific version for aarch64. By making the arch a separate option, 
> there is no way, to model this.
> If it was done in the kernel config check, it could be possible to add 
> version and arch checks like
> (CONFIG_AARCH64 && CONFIG_VERSION > 5.3) || CONFIG_X86_64
> 
> While this probably does not produce a very good error message, it is 
> more versatile.
> 
> Sorry for this late questioning the whole approach.

Not at all, this is a good point.

The main problem is that the kernel architecture does not need to match
the binary architecture which is what this patchset tries to cover. That
means that 32bit binary on 64bit kernel would not match what we are
supposed to match. Even more the config variables are confusing, on
x86_64 with compat layer enabled we get:

CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y

That makes any reasoning quite messy.

What would make much more sense would be injecting LTP specific
variables to the parsed variables before evaluation. So for instance we
would insert BINARY_ARCH variable which would cover this exact case and
the check would look like:

"(BINARY_ARCH == "aarch64" && CONFIG_VERSION > 5.3) || CONFIG_X86_64"

However I would still like to have a simple list of supported
architectures in the test structure as well, since that is much easier
to read and reason about and it covers 99% of the cases. Nothing stops
us for adding the more complex checks in the case that we see the need
later on.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-18 12:07   ` Cyril Hrubis
@ 2021-11-22  2:21     ` Li Wang
  2021-11-22 13:27       ` Richard Palethorpe
  0 siblings, 1 reply; 18+ messages in thread
From: Li Wang @ 2021-11-22  2:21 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 181 bytes --]

Hi All,

By now, should I push this patch v5, or is any else change needed?
(From the above discussion I feel v5 is satisfied almost all of our
requirements.)

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 566 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-22  2:21     ` Li Wang
@ 2021-11-22 13:27       ` Richard Palethorpe
  2021-11-22 14:21         ` Cyril Hrubis
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Palethorpe @ 2021-11-22 13:27 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi All,
>
> By now, should I push this patch v5, or is any else change needed?

I still don't understand how tst_arch.name is mapped to i386, i586 or
i686?

It appears that we will always get TCONF on any of these architectures
because tst_arch.name will be set to x86 and the required arch will be
i386, i586 or i686.

> (From the above discussion I feel v5 is satisfied almost all of our
> requirements.)

Otherwise, yes, I agree.

>
> -- 
> Regards,
> Li Wang


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-22 13:27       ` Richard Palethorpe
@ 2021-11-22 14:21         ` Cyril Hrubis
  2021-11-23  4:13           ` Li Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Cyril Hrubis @ 2021-11-22 14:21 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: ltp

Hi!
> I still don't understand how tst_arch.name is mapped to i386, i586 or
> i686?
> 
> It appears that we will always get TCONF on any of these architectures
> because tst_arch.name will be set to x86 and the required arch will be
> i386, i586 or i686.

Can we please have single name for 32bit intel i.e. "x86" please?

> > (From the above discussion I feel v5 is satisfied almost all of our
> > requirements.)
> 
> Otherwise, yes, I agree.

Same here, the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure
  2021-11-22 14:21         ` Cyril Hrubis
@ 2021-11-23  4:13           ` Li Wang
  0 siblings, 0 replies; 18+ messages in thread
From: Li Wang @ 2021-11-23  4:13 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 664 bytes --]

Hi All,

On Mon, Nov 22, 2021 at 10:20 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > I still don't understand how tst_arch.name is mapped to i386, i586 or
> > i686?
> >
> > It appears that we will always get TCONF on any of these architectures
> > because tst_arch.name will be set to x86 and the required arch will be
> > i386, i586 or i686.
>

You are right, I neglected that point, anyway, let's switch to x86 in all.


>
> Can we please have single name for 32bit intel i.e. "x86" please?
>

Ok, sure.


Same here, the rest looks good.
>

Thank you all for the reviews.

I made changes by using the single name "x86" and pushed it.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 1991 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-11-23  4:13 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17  7:07 [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Li Wang
2021-11-17  7:07 ` [LTP] [PATCH v5 2/3] testcase: make use of .supported_archs Li Wang
2021-11-17  7:07 ` [LTP] [PATCH v5 3/3] max_map_count: replace ifdefs by tst_arch Li Wang
2021-11-17 10:33   ` Richard Palethorpe
2021-11-17 13:28     ` Li Wang
2021-11-17 13:58       ` Richard Palethorpe
2021-11-17  9:58 ` [LTP] [PATCH v5 1/3] lib: adding .supported_archs field in tst_test structure Richard Palethorpe
2021-11-17 13:51   ` Li Wang
2021-11-17 14:01     ` Richard Palethorpe
2021-11-18 11:42       ` Cyril Hrubis
2021-11-17 13:59 ` Joerg Vehlow
2021-11-17 14:25   ` Richard Palethorpe
2021-11-18  5:50     ` Li Wang
2021-11-18 12:07   ` Cyril Hrubis
2021-11-22  2:21     ` Li Wang
2021-11-22 13:27       ` Richard Palethorpe
2021-11-22 14:21         ` Cyril Hrubis
2021-11-23  4:13           ` Li Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.