diff mbox series

selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha

Message ID ddf594c81787dba708fc392cb03027470dee64fb.1725124064.git.christophe.leroy@csgroup.eu
State New
Headers show
Series selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha | expand

Commit Message

Christophe Leroy Aug. 31, 2024, 5:11 p.m. UTC
$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

Comments

kernel test robot Sept. 1, 2024, 1:27 p.m. UTC | #1
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
Jason A. Donenfeld Sept. 1, 2024, 6:02 p.m. UTC | #2
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.
Jason A. Donenfeld Sept. 2, 2024, 1:20 a.m. UTC | #3
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)..
Christophe Leroy Sept. 2, 2024, 12:22 p.m. UTC | #4
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
Mark Brown Sept. 2, 2024, 12:37 p.m. UTC | #5
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.
LEROY Christophe Sept. 2, 2024, 12:39 p.m. UTC | #6
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
Jason A. Donenfeld Sept. 2, 2024, 12:43 p.m. UTC | #7
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...
Christophe Leroy Sept. 2, 2024, 1:23 p.m. UTC | #8
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
Jason A. Donenfeld Sept. 2, 2024, 1:57 p.m. UTC | #9
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.
Christophe Leroy Sept. 2, 2024, 2:18 p.m. UTC | #10
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.
Christophe Leroy Sept. 2, 2024, 2:23 p.m. UTC | #11
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.
Christophe Leroy Nov. 7, 2024, 8:40 a.m. UTC | #12
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
diff mbox series

Patch

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) {