[PATCH] selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha
$ARCH is not always enough to know whether getrandom vDSO is supported or not. For instance on x86 we want it for x86_64 but not i386.
On the other hand, we already have detailed architecture selection in vdso_config.h, the only difference is that it cannot be used for Makefile. But most selftests are built regardless of whether a functionality is supported or not. The return value KSFT_SKIP is there for than: it tells the test is skipped because it is not supported.
Make the implementation more flexible by setting a VDSO_GETRANDOM macro in vdso_config.h. That macro contains the path to the file that defines __arch_chacha20_blocks_nostack(). It avoids the symbolic link to vdso directory and will allow architectures to have several implementations of __arch_chacha20_blocks_nostack() if needed.
Then restore the original behaviour which was dedicated to vdso_standalone_test_x86 and build getrandom and chacha tests all the time just like other vDSO selftests and return SKIP when the functionality to be tested is not implemented.
This has the advantage of doing architecture specific selection at only one place.
Also change vdso_test_getrandom to return SKIP instead of FAIL when vDSO function is not found, just like vdso_test_getcpu or vdso_test_gettimeofday.
Signed-off-by: Christophe Leroy christophe.leroy@csgroup.eu --- Based on latest random tree (0dfed8092247)
tools/arch/x86/vdso | 1 - tools/testing/selftests/vDSO/Makefile | 10 ++++------ tools/testing/selftests/vDSO/vdso_config.h | 3 +++ tools/testing/selftests/vDSO/vdso_test_chacha-asm.S | 7 +++++++ tools/testing/selftests/vDSO/vdso_test_chacha.c | 11 +++++++++++ tools/testing/selftests/vDSO/vdso_test_getrandom.c | 2 +- 6 files changed, 26 insertions(+), 8 deletions(-) delete mode 120000 tools/arch/x86/vdso create mode 100644 tools/testing/selftests/vDSO/vdso_test_chacha-asm.S
diff --git a/tools/arch/x86/vdso b/tools/arch/x86/vdso deleted file mode 120000 index 7eb962fd3454..000000000000 --- a/tools/arch/x86/vdso +++ /dev/null @@ -1 +0,0 @@ -../../../arch/x86/entry/vdso/ \ No newline at end of file diff --git a/tools/testing/selftests/vDSO/Makefile b/tools/testing/selftests/vDSO/Makefile index 5ead6b1f0478..cfb7c281b22c 100644 --- a/tools/testing/selftests/vDSO/Makefile +++ b/tools/testing/selftests/vDSO/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -ARCH ?= $(shell uname -m | sed -e s/i.86/x86/) -SRCARCH := $(subst x86_64,x86,$(ARCH)) +uname_M := $(shell uname -m 2>/dev/null || echo not) +ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
TEST_GEN_PROGS := vdso_test_gettimeofday TEST_GEN_PROGS += vdso_test_getcpu @@ -10,10 +10,8 @@ ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64)) TEST_GEN_PROGS += vdso_standalone_test_x86 endif TEST_GEN_PROGS += vdso_test_correctness -ifeq ($(ARCH),$(filter $(ARCH),x86_64)) TEST_GEN_PROGS += vdso_test_getrandom TEST_GEN_PROGS += vdso_test_chacha -endif
CFLAGS := -std=gnu99
@@ -38,8 +36,8 @@ $(OUTPUT)/vdso_test_getrandom: CFLAGS += -isystem $(top_srcdir)/tools/include \ $(KHDR_INCLUDES) \ -isystem $(top_srcdir)/include/uapi
-$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S +$(OUTPUT)/vdso_test_chacha: vdso_test_chacha-asm.S $(OUTPUT)/vdso_test_chacha: CFLAGS += -idirafter $(top_srcdir)/tools/include \ - -idirafter $(top_srcdir)/arch/$(SRCARCH)/include \ + -idirafter $(top_srcdir)/arch/$(ARCH)/include \ -idirafter $(top_srcdir)/include \ -D__ASSEMBLY__ -Wa,--noexecstack diff --git a/tools/testing/selftests/vDSO/vdso_config.h b/tools/testing/selftests/vDSO/vdso_config.h index 740ce8c98d2e..693920471160 100644 --- a/tools/testing/selftests/vDSO/vdso_config.h +++ b/tools/testing/selftests/vDSO/vdso_config.h @@ -47,6 +47,7 @@ #elif defined(__x86_64__) #define VDSO_VERSION 0 #define VDSO_NAMES 1 +#define VDSO_GETRANDOM "../../../../arch/x86/entry/vdso/vgetrandom-chacha.S" #elif defined(__riscv__) || defined(__riscv) #define VDSO_VERSION 5 #define VDSO_NAMES 1 @@ -58,6 +59,7 @@ #define VDSO_NAMES 1 #endif
+#ifndef __ASSEMBLY__ static const char *versions[7] = { "LINUX_2.6", "LINUX_2.6.15", @@ -88,5 +90,6 @@ static const char *names[2][7] = { "__vdso_getrandom", }, }; +#endif
#endif /* __VDSO_CONFIG_H__ */ diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S new file mode 100644 index 000000000000..8e704165f6f2 --- /dev/null +++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S @@ -0,0 +1,7 @@ +#include "vdso_config.h" + +#ifdef VDSO_GETRANDOM + +#include VDSO_GETRANDOM + +#endif diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha.c b/tools/testing/selftests/vDSO/vdso_test_chacha.c index 3a5a08d857cf..9d18d49a82f8 100644 --- a/tools/testing/selftests/vDSO/vdso_test_chacha.c +++ b/tools/testing/selftests/vDSO/vdso_test_chacha.c @@ -8,6 +8,8 @@ #include <string.h> #include <stdint.h> #include <stdbool.h> +#include <linux/kconfig.h> +#include "vdso_config.h" #include "../kselftest.h"
static uint32_t rol32(uint32_t word, unsigned int shift) @@ -57,6 +59,10 @@ typedef uint32_t u32; typedef uint64_t u64; #include <vdso/getrandom.h>
+#ifdef VDSO_GETRANDOM +#define HAVE_VDSO_GETRANDOM 1 +#endif + int main(int argc, char *argv[]) { enum { TRIALS = 1000, BLOCKS = 128, BLOCK_SIZE = 64 }; @@ -68,6 +74,11 @@ int main(int argc, char *argv[]) ksft_print_header(); ksft_set_plan(1);
+ if (!__is_defined(HAVE_VDSO_GETRANDOM)) { + printf("__arch_chacha20_blocks_nostack() not implemented\n"); + return KSFT_SKIP; + } + for (unsigned int trial = 0; trial < TRIALS; ++trial) { if (getrandom(key, sizeof(key), 0) != sizeof(key)) { printf("getrandom() failed!\n"); diff --git a/tools/testing/selftests/vDSO/vdso_test_getrandom.c b/tools/testing/selftests/vDSO/vdso_test_getrandom.c index 8866b65a4605..47ee94b32617 100644 --- a/tools/testing/selftests/vDSO/vdso_test_getrandom.c +++ b/tools/testing/selftests/vDSO/vdso_test_getrandom.c @@ -115,7 +115,7 @@ static void vgetrandom_init(void) vgrnd.fn = (__typeof__(vgrnd.fn))vdso_sym(version, name); if (!vgrnd.fn) { printf("%s is missing!\n", name); - exit(KSFT_FAIL); + exit(KSFT_SKIP); } ret = VDSO_CALL(vgrnd.fn, 5, NULL, 0, 0, &vgrnd.params, ~0UL); if (ret == -ENOSYS) {
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
What about, instead, something like below, replacing the other commit?
Jason
From ccc53425c98f4f5c2a1edaf775089efb56bd106e Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Jason@zx2c4.com Date: Sun, 1 Sep 2024 15:05:01 +0200 Subject: [PATCH] selftests: vDSO: fix cross build for getrandom and chacha tests
Unlike the check for the standalone x86 test, the check for building the vDSO getrandom and chacaha tests looks at the architecture for the host rather than the architecture for the target when deciding if they should be built. Since the chacha test includes some assembler code this means that cross building with x86 as either the target or host is broken.
There's also some additional complications, where ARCH can legitimately be either x86_64 or x86, but the source code we need to compile lives in a directory path containing arch/x86. The standard SRCARCH variable handles that. And actually, all these variables and proper substitutions are already described in tools/scripts/Makefile.arch, so just include that to handle it.
Similarly, ARCH=x86 can actually describe ARCH=x86_64, just with CONFIG_64BIT, so we can't rely on ARCH for selecting non-32-bit tests. For that, check against $(ARCH)$(CONFIG_X86_32). This won't help for people manually running this inside the vDSO selftest directory (which isn't really supported anyway and has problems on various archs), but it should work for builds of the kselftests, where the CONFIG_* variables are defined. On x86_64 machines, $(ARCH)$(CONFIG_X86_32) will evaluate to x86. On arm64 machines, it will evaluate to arm64. On 32-bit x86 machines, it will evaluate to x86y, which won't match the filter list.
Reported-by: Mark Brown broonie@kernel.org Reported-by: Christophe Leroy christophe.leroy@csgroup.eu Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com --- tools/testing/selftests/vDSO/Makefile | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/vDSO/Makefile b/tools/testing/selftests/vDSO/Makefile index e21e78aae24d..01a5805062b3 100644 --- a/tools/testing/selftests/vDSO/Makefile +++ b/tools/testing/selftests/vDSO/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -uname_M := $(shell uname -m 2>/dev/null || echo not) -ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/) +include ../../../scripts/Makefile.arch
TEST_GEN_PROGS := vdso_test_gettimeofday TEST_GEN_PROGS += vdso_test_getcpu @@ -10,7 +9,7 @@ ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64)) TEST_GEN_PROGS += vdso_standalone_test_x86 endif TEST_GEN_PROGS += vdso_test_correctness -ifeq ($(uname_M),x86_64) +ifeq ($(ARCH)$(CONFIG_X86_32),$(filter $(ARCH)$(CONFIG_X86_32),x86)) TEST_GEN_PROGS += vdso_test_getrandom TEST_GEN_PROGS += vdso_test_chacha endif @@ -38,8 +37,8 @@ $(OUTPUT)/vdso_test_getrandom: CFLAGS += -isystem $(top_srcdir)/tools/include \ $(KHDR_INCLUDES) \ -isystem $(top_srcdir)/include/uapi
-$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/tools/arch/$(ARCH)/vdso/vgetrandom-chacha.S +$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S $(OUTPUT)/vdso_test_chacha: CFLAGS += -idirafter $(top_srcdir)/tools/include \ - -idirafter $(top_srcdir)/arch/$(ARCH)/include \ + -idirafter $(top_srcdir)/arch/$(SRCARCH)/include \ -idirafter $(top_srcdir)/include \ -D__ASSEMBLY__ -Wa,--noexecstack -- 2.46.0
Hi Jason,
Le 01/09/2024 à 15:22, Jason A. Donenfeld a écrit :
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
What about, instead, something like below, replacing the other commit?
I need to look at it in more details and perfom a test, but after first look I can't figure out how it would work.
When I build selftests,
to build 32 bits selftests I do:
make ARCH=powerpc CROSS_COMPILE=ppc-linux-
to build a 64 bits BE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-
to build a 64 bits LE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-
I addition, in case someone does the build on a native platform directly,
On 32 bits, uname -m returns 'ppc' On 64 bits, uname -m returns 'ppc64' On 64 bits little endian, uname -m returns 'ppc64le'
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Christophe
Jason
From ccc53425c98f4f5c2a1edaf775089efb56bd106e Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Jason@zx2c4.com Date: Sun, 1 Sep 2024 15:05:01 +0200 Subject: [PATCH] selftests: vDSO: fix cross build for getrandom and chacha tests
Unlike the check for the standalone x86 test, the check for building the vDSO getrandom and chacaha tests looks at the architecture for the host rather than the architecture for the target when deciding if they should be built. Since the chacha test includes some assembler code this means that cross building with x86 as either the target or host is broken.
There's also some additional complications, where ARCH can legitimately be either x86_64 or x86, but the source code we need to compile lives in a directory path containing arch/x86. The standard SRCARCH variable handles that. And actually, all these variables and proper substitutions are already described in tools/scripts/Makefile.arch, so just include that to handle it.
Similarly, ARCH=x86 can actually describe ARCH=x86_64, just with CONFIG_64BIT, so we can't rely on ARCH for selecting non-32-bit tests. For that, check against $(ARCH)$(CONFIG_X86_32). This won't help for people manually running this inside the vDSO selftest directory (which isn't really supported anyway and has problems on various archs), but it should work for builds of the kselftests, where the CONFIG_* variables are defined. On x86_64 machines, $(ARCH)$(CONFIG_X86_32) will evaluate to x86. On arm64 machines, it will evaluate to arm64. On 32-bit x86 machines, it will evaluate to x86y, which won't match the filter list.
Reported-by: Mark Brown broonie@kernel.org Reported-by: Christophe Leroy christophe.leroy@csgroup.eu Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
tools/testing/selftests/vDSO/Makefile | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/vDSO/Makefile b/tools/testing/selftests/vDSO/Makefile index e21e78aae24d..01a5805062b3 100644 --- a/tools/testing/selftests/vDSO/Makefile +++ b/tools/testing/selftests/vDSO/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -uname_M := $(shell uname -m 2>/dev/null || echo not) -ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/) +include ../../../scripts/Makefile.arch
TEST_GEN_PROGS := vdso_test_gettimeofday TEST_GEN_PROGS += vdso_test_getcpu @@ -10,7 +9,7 @@ ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64)) TEST_GEN_PROGS += vdso_standalone_test_x86 endif TEST_GEN_PROGS += vdso_test_correctness -ifeq ($(uname_M),x86_64) +ifeq ($(ARCH)$(CONFIG_X86_32),$(filter $(ARCH)$(CONFIG_X86_32),x86)) TEST_GEN_PROGS += vdso_test_getrandom TEST_GEN_PROGS += vdso_test_chacha endif @@ -38,8 +37,8 @@ $(OUTPUT)/vdso_test_getrandom: CFLAGS += -isystem $(top_srcdir)/tools/include \ $(KHDR_INCLUDES) \ -isystem $(top_srcdir)/include/uapi
-$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/tools/arch/$(ARCH)/vdso/vgetrandom-chacha.S +$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S $(OUTPUT)/vdso_test_chacha: CFLAGS += -idirafter $(top_srcdir)/tools/include \
-idirafter $(top_srcdir)/arch/$(ARCH)/include \
-idirafter $(top_srcdir)/arch/$(SRCARCH)/include \ -idirafter $(top_srcdir)/include \ -D__ASSEMBLY__ -Wa,--noexecstack-- 2.46.0
On Sun, Sep 01, 2024 at 08:00:30PM +0200, Christophe Leroy wrote:
Hi Jason,
Le 01/09/2024 à 15:22, Jason A. Donenfeld a écrit :
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
What about, instead, something like below, replacing the other commit?
I need to look at it in more details and perfom a test, but after first look I can't figure out how it would work.
When I build selftests,
to build 32 bits selftests I do:
make ARCH=powerpc CROSS_COMPILE=ppc-linux-
to build a 64 bits BE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-
to build a 64 bits LE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-
I addition, in case someone does the build on a native platform directly,
On 32 bits, uname -m returns 'ppc' On 64 bits, uname -m returns 'ppc64' On 64 bits little endian, uname -m returns 'ppc64le'
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Huh? That's not what tools/scripts/Makefile.arch does.
Le 01/09/2024 à 20:02, Jason A. Donenfeld a écrit :
On Sun, Sep 01, 2024 at 08:00:30PM +0200, Christophe Leroy wrote:
Hi Jason,
Le 01/09/2024 à 15:22, Jason A. Donenfeld a écrit :
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
What about, instead, something like below, replacing the other commit?
I need to look at it in more details and perfom a test, but after first look I can't figure out how it would work.
When I build selftests,
to build 32 bits selftests I do:
make ARCH=powerpc CROSS_COMPILE=ppc-linux-
to build a 64 bits BE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-
to build a 64 bits LE selftests I do:
make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-
I addition, in case someone does the build on a native platform directly,
On 32 bits, uname -m returns 'ppc' On 64 bits, uname -m returns 'ppc64' On 64 bits little endian, uname -m returns 'ppc64le'
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Huh? That's not what tools/scripts/Makefile.arch does.
Hum ... yes sorry I looked at it too quickly and mixed things up with the other patch.
Nevertheless, if I understand well what tools/scripts/Makefile.arch does on an x86_64 for instance:
uname -m returns x86_64 HOSTARCH = x86 (sed -e s/x86_64/x86) ARCH = x86 SRCARCH = x86
If you build with make ARCH=x86_64, SRCARCH = x86
So I still can't see how you can use that to know if it is a x86_64 or not.
I don't see either what could be the result for powerpc.
By the way, in your patch I don't think you can use CONFIG_X86_32, CONFIG symbols are not known when building selftests.
Christophe
On Sun, Sep 01, 2024 at 08:43:10PM +0200, Christophe Leroy wrote:
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Huh? That's not what tools/scripts/Makefile.arch does.
Hum ... yes sorry I looked at it too quickly and mixed things up with the other patch.
Nevertheless, if I understand well what tools/scripts/Makefile.arch does on an x86_64 for instance:
uname -m returns x86_64 HOSTARCH = x86 (sed -e s/x86_64/x86) ARCH = x86 SRCARCH = x86
If you build with make ARCH=x86_64, SRCARCH = x86
So I still can't see how you can use that to know if it is a x86_64 or not.
By the use of CONFIG_X86_32, which is also used elsewhere in that samme makefile for something else (so I assume it's wired up in the context where it counts, and if not, that's a bug that affects both spots and should be fixed)..
Le 02/09/2024 à 03:20, Jason A. Donenfeld a écrit :
On Sun, Sep 01, 2024 at 08:43:10PM +0200, Christophe Leroy wrote:
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Huh? That's not what tools/scripts/Makefile.arch does.
Hum ... yes sorry I looked at it too quickly and mixed things up with the other patch.
Nevertheless, if I understand well what tools/scripts/Makefile.arch does on an x86_64 for instance:
uname -m returns x86_64 HOSTARCH = x86 (sed -e s/x86_64/x86) ARCH = x86 SRCARCH = x86
If you build with make ARCH=x86_64, SRCARCH = x86
So I still can't see how you can use that to know if it is a x86_64 or not.
By the use of CONFIG_X86_32, which is also used elsewhere in that samme makefile for something else (so I assume it's wired up in the context where it counts, and if not, that's a bug that affects both spots and should be fixed)..
I looks like it is a left-over from the time vDSO selftests were in Documentation/vDSO and were likely built with kernel config.
CONFIG_X86_32 was brought into tools/testing/selftests/vDSO by commit f9b6b0ef6034 ("selftests: move vDSO tests from Documentation/vDSO") and was meant to pass -lgcc_s when building vdso_standalone_test_x86 for i386, but obviously it doesn't work:
$ make ARCH=i386 V=1 gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_gettimeofday.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_gettimeofday gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_getcpu.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_getcpu gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_abi.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_abi gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_clock_getres.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_clock_getres gcc -std=gnu99 -O2 -D_GNU_SOURCE= -ldl vdso_test_correctness.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_correctness
In another place in selftests (tools/testing/selftests/ipc/), they manually add it:
ifeq ($(ARCH),i386) ARCH := x86 CFLAGS := -DCONFIG_X86_32 -D__i386__ endif ifeq ($(ARCH),x86_64) ARCH := x86 CFLAGS := -DCONFIG_X86_64 -D__x86_64__ endif
So I think this is a confirmation that CONFIG_X86_32 doesn't exist in selftests.
Christophe
On Mon, Sep 02, 2024 at 12:39:17PM +0000, LEROY Christophe wrote:
Le 02/09/2024 à 03:20, Jason A. Donenfeld a écrit :
On Sun, Sep 01, 2024 at 08:43:10PM +0200, Christophe Leroy wrote:
How would this fit in the logic where IIUC you just remove '_64' from 'x86_64' to get 'x86'
Huh? That's not what tools/scripts/Makefile.arch does.
Hum ... yes sorry I looked at it too quickly and mixed things up with the other patch.
Nevertheless, if I understand well what tools/scripts/Makefile.arch does on an x86_64 for instance:
uname -m returns x86_64 HOSTARCH = x86 (sed -e s/x86_64/x86) ARCH = x86 SRCARCH = x86
If you build with make ARCH=x86_64, SRCARCH = x86
So I still can't see how you can use that to know if it is a x86_64 or not.
By the use of CONFIG_X86_32, which is also used elsewhere in that samme makefile for something else (so I assume it's wired up in the context where it counts, and if not, that's a bug that affects both spots and should be fixed)..
I looks like it is a left-over from the time vDSO selftests were in Documentation/vDSO and were likely built with kernel config.
CONFIG_X86_32 was brought into tools/testing/selftests/vDSO by commit f9b6b0ef6034 ("selftests: move vDSO tests from Documentation/vDSO") and was meant to pass -lgcc_s when building vdso_standalone_test_x86 for i386, but obviously it doesn't work:
$ make ARCH=i386 V=1 gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_gettimeofday.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_gettimeofday gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_getcpu.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_getcpu gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_abi.c parse_vdso.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_abi gcc -std=gnu99 -O2 -D_GNU_SOURCE= vdso_test_clock_getres.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_clock_getres gcc -std=gnu99 -O2 -D_GNU_SOURCE= -ldl vdso_test_correctness.c -o /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_correctness
In another place in selftests (tools/testing/selftests/ipc/), they manually add it:
ifeq ($(ARCH),i386) ARCH := x86 CFLAGS := -DCONFIG_X86_32 -D__i386__ endif ifeq ($(ARCH),x86_64) ARCH := x86 CFLAGS := -DCONFIG_X86_64 -D__x86_64__ endif
So I think this is a confirmation that CONFIG_X86_32 doesn't exist in selftests.
Interesting... Seems like both sites, then, should be fixed somehow...
Hi Jason,
Le 01/09/2024 à 15:22, Jason A. Donenfeld a écrit :
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
If we take the exemple of getcpu:
Only powerpc and s390 implement __kernel_getcpu. Until recently, it was implemented on ppc64 but not on ppc32
Only loongarch, riscv and x86 implement __vdso_getcpu.
Nevertheless, vdso_test_getcpu is always builts, regardless of the architecture.
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
Christophe
On Mon, Sep 02, 2024 at 02:22:38PM +0200, Christophe Leroy wrote:
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
The problem is that the test incorporates assembler code so it simply won't build for architectures without explicit porting, the issue isn't if the target kernel supports it but rather that the test won't compile in the first place.
Le 02/09/2024 à 14:37, Mark Brown a écrit :
On Mon, Sep 02, 2024 at 02:22:38PM +0200, Christophe Leroy wrote:
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
The problem is that the test incorporates assembler code so it simply won't build for architectures without explicit porting, the issue isn't if the target kernel supports it but rather that the test won't compile in the first place.
Yes indeed and that was the purpose of my patch, have a macro in vdso_config.h to tell where the assembler code is:
diff --git a/tools/testing/selftests/vDSO/vdso_config.h b/tools/testing/selftests/vDSO/vdso_config.h index 740ce8c98d2e..693920471160 100644 --- a/tools/testing/selftests/vDSO/vdso_config.h +++ b/tools/testing/selftests/vDSO/vdso_config.h @@ -47,6 +47,7 @@ #elif defined(__x86_64__) #define VDSO_VERSION 0 #define VDSO_NAMES 1 +#define VDSO_GETRANDOM "../../../../arch/x86/entry/vdso/vgetrandom-chacha.S" #elif defined(__riscv__) || defined(__riscv) #define VDSO_VERSION 5 #define VDSO_NAMES 1
And then:
diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S new file mode 100644 index 000000000000..8e704165f6f2 --- /dev/null +++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S @@ -0,0 +1,7 @@ +#include "vdso_config.h" + +#ifdef VDSO_GETRANDOM + +#include VDSO_GETRANDOM + +#endif
I thought it was a lot easier to handle if through necessary #ifdefs in vdso_config.h that implementing an additional logic in Makefiles.
Christophe
On Mon, Sep 02, 2024 at 03:23:47PM +0200, Christophe Leroy wrote:
Le 02/09/2024 à 14:37, Mark Brown a écrit :
On Mon, Sep 02, 2024 at 02:22:38PM +0200, Christophe Leroy wrote:
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
The problem is that the test incorporates assembler code so it simply won't build for architectures without explicit porting, the issue isn't if the target kernel supports it but rather that the test won't compile in the first place.
Yes indeed and that was the purpose of my patch, have a macro in vdso_config.h to tell where the assembler code is:
diff --git a/tools/testing/selftests/vDSO/vdso_config.h b/tools/testing/selftests/vDSO/vdso_config.h index 740ce8c98d2e..693920471160 100644 --- a/tools/testing/selftests/vDSO/vdso_config.h +++ b/tools/testing/selftests/vDSO/vdso_config.h @@ -47,6 +47,7 @@ #elif defined(__x86_64__) #define VDSO_VERSION 0 #define VDSO_NAMES 1 +#define VDSO_GETRANDOM "../../../../arch/x86/entry/vdso/vgetrandom-chacha.S" #elif defined(__riscv__) || defined(__riscv) #define VDSO_VERSION 5 #define VDSO_NAMES 1
And then:
diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S new file mode 100644 index 000000000000..8e704165f6f2 --- /dev/null +++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S @@ -0,0 +1,7 @@ +#include "vdso_config.h"
+#ifdef VDSO_GETRANDOM
+#include VDSO_GETRANDOM
+#endif
I thought it was a lot easier to handle if through necessary #ifdefs in vdso_config.h that implementing an additional logic in Makefiles.
Yet it still tripped up the test robot, right?
In general I'm not crazy about this approach.
Le 02/09/2024 à 15:57, Jason A. Donenfeld a écrit :
On Mon, Sep 02, 2024 at 03:23:47PM +0200, Christophe Leroy wrote:
Le 02/09/2024 à 14:37, Mark Brown a écrit :
On Mon, Sep 02, 2024 at 02:22:38PM +0200, Christophe Leroy wrote:
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
The problem is that the test incorporates assembler code so it simply won't build for architectures without explicit porting, the issue isn't if the target kernel supports it but rather that the test won't compile in the first place.
Yes indeed and that was the purpose of my patch, have a macro in vdso_config.h to tell where the assembler code is:
diff --git a/tools/testing/selftests/vDSO/vdso_config.h b/tools/testing/selftests/vDSO/vdso_config.h index 740ce8c98d2e..693920471160 100644 --- a/tools/testing/selftests/vDSO/vdso_config.h +++ b/tools/testing/selftests/vDSO/vdso_config.h @@ -47,6 +47,7 @@ #elif defined(__x86_64__) #define VDSO_VERSION 0 #define VDSO_NAMES 1 +#define VDSO_GETRANDOM "../../../../arch/x86/entry/vdso/vgetrandom-chacha.S" #elif defined(__riscv__) || defined(__riscv) #define VDSO_VERSION 5 #define VDSO_NAMES 1
And then:
diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S new file mode 100644 index 000000000000..8e704165f6f2 --- /dev/null +++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S @@ -0,0 +1,7 @@ +#include "vdso_config.h"
+#ifdef VDSO_GETRANDOM
+#include VDSO_GETRANDOM
+#endif
I thought it was a lot easier to handle if through necessary #ifdefs in vdso_config.h that implementing an additional logic in Makefiles.
Yet it still tripped up the test robot, right?
Yes I need to look at that.
In general I'm not crazy about this approach.
I have the feeling I get things done easier with that approach. But if you feel better playing up with the makefile, I incline.
Le 02/09/2024 à 16:18, Christophe Leroy a écrit :
Le 02/09/2024 à 15:57, Jason A. Donenfeld a écrit :
On Mon, Sep 02, 2024 at 03:23:47PM +0200, Christophe Leroy wrote:
Le 02/09/2024 à 14:37, Mark Brown a écrit :
On Mon, Sep 02, 2024 at 02:22:38PM +0200, Christophe Leroy wrote:
When vdso_test_getcpu doesn't find the vDSO entry point, it prints an error text and returns KSFT_SKIP
I thought it would be more correct to have the same behaviour on vdso_test_getrandom instead of trying to build it only when the underlying kernel supports it.
The problem is that the test incorporates assembler code so it simply won't build for architectures without explicit porting, the issue isn't if the target kernel supports it but rather that the test won't compile in the first place.
Yes indeed and that was the purpose of my patch, have a macro in vdso_config.h to tell where the assembler code is:
diff --git a/tools/testing/selftests/vDSO/vdso_config.h b/tools/testing/selftests/vDSO/vdso_config.h index 740ce8c98d2e..693920471160 100644 --- a/tools/testing/selftests/vDSO/vdso_config.h +++ b/tools/testing/selftests/vDSO/vdso_config.h @@ -47,6 +47,7 @@ #elif defined(__x86_64__) #define VDSO_VERSION 0 #define VDSO_NAMES 1 +#define VDSO_GETRANDOM "../../../../arch/x86/entry/vdso/vgetrandom-chacha.S" #elif defined(__riscv__) || defined(__riscv) #define VDSO_VERSION 5 #define VDSO_NAMES 1
And then:
diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S new file mode 100644 index 000000000000..8e704165f6f2 --- /dev/null +++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S @@ -0,0 +1,7 @@ +#include "vdso_config.h"
+#ifdef VDSO_GETRANDOM
+#include VDSO_GETRANDOM
+#endif
I thought it was a lot easier to handle if through necessary #ifdefs in vdso_config.h that implementing an additional logic in Makefiles.
Yet it still tripped up the test robot, right?
Yes I need to look at that.
In general I'm not crazy about this approach.
I have the feeling I get things done easier with that approach. But if you feel better playing up with the makefile, I incline.
Also I thing that one day or another someone will want to implement it a more performant way on power10 which is one of the latest powerpc CPU, something similar to arch/powerpc/crypto/chacha-p10le-8x.S
When that happens, we will need a way to tell vdso_test_chacha to build another vgetrandom-chacha.S and I feel that doing it in the Makefile will become really tricky.
Hi Jason,
Le 01/09/2024 à 15:22, Jason A. Donenfeld a écrit :
Hi Christophe,
Hmm, I'm not so sure I like this very much. I think it's important for these tests to fail when an arch tries to hook up the function to the vDSO, but it's still not exported for some reason. This also regresses the ARCH=x86_64 vs ARCH=x86 thing, which SRCARCH fixes.
I was surprised today to discover that you finaly pushed something more or less similar. Did you change your mind ?
Christophe
Hi Christophe,
kernel test robot noticed the following build errors:
[auto build test ERROR on crng-random/master] [cannot apply to shuah-kselftest/next shuah-kselftest/fixes linus/master v6.11-rc6 next-20240830] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christophe-Leroy/selftests-vD... base: https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git master patch link: https://lore.kernel.org/r/ddf594c81787dba708fc392cb03027470dee64fb.172512406... patch subject: [PATCH] selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha :::::: branch date: 20 hours ago :::::: commit date: 20 hours ago compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409012034.hcMbZCrE-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/r/202409012034.hcMbZCrE-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from vdso_test_chacha-asm.S:5:
./../../../../arch/x86/entry/vdso/vgetrandom-chacha.S:7:10: fatal error: 'asm/frame.h' file not found
7 | #include <asm/frame.h> | ^~~~~~~~~~~~~ 1 error generated.
vim +7 tools/testing/selftests/vDSO/./../../../../arch/x86/entry/vdso/vgetrandom-chacha.S
33385150ac456f6 Jason A. Donenfeld 2022-11-18 @7 #include <asm/frame.h> 33385150ac456f6 Jason A. Donenfeld 2022-11-18 8
linux-kselftest-mirror@lists.linaro.org
-
Christophe Leroy -
Jason A. Donenfeld -
kernel test robot -
LEROY Christophe -
Mark Brown