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
$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) {
--
2.44.0Hi 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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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. I was surprised today to discover that you finaly pushed something more or less similar. Did you change your mind ? Christophe
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. > > 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...
© 2016 - 2026 Red Hat, Inc.