| Message ID | ddf594c81787dba708fc392cb03027470dee64fb.1725124064.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha | expand |
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 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-vDSO-Do-not-rely-on-ARCH-for-vdso_test_getrandom-vdso_test_chacha/20240901-011346
base: https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git master
patch link: https://lore.kernel.org/r/ddf594c81787dba708fc392cb03027470dee64fb.1725124064.git.christophe.leroy%40csgroup.eu
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@intel.com/reproduce)
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
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)..
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 à 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...
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.
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) {
$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