| Message ID | 20240827132018.88854-1-xry111@xry111.site |
|---|---|
| Headers | show |
| Series | LoongArch: Implement getrandom() in vDSO | expand |
On Tue, Aug 27, 2024 at 09:20:16PM +0800, Xi Ruoyao wrote: > Building test_vdso_getrandom currently leads to following issue: > > In file included from /home/xry111/git-repos/linux/tools/include/linux/compiler_types.h:36, > from /home/xry111/git-repos/linux/include/uapi/linux/stddef.h:5, > from /home/xry111/git-repos/linux/include/uapi/linux/posix_types.h:5, > from /usr/include/asm/sigcontext.h:12, > from /usr/include/bits/sigcontext.h:30, > from /usr/include/signal.h:301, > from vdso_test_getrandom.c:14: > /home/xry111/git-repos/linux/tools/include/linux/compiler-gcc.h:3:2: error: #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > 3 | #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > | ^~~~~ > > It's because the compiler_types.h inclusion in > include/uapi/linux/stddef.h is expected to be removed by the > header_install.sh script, as compiler_types.h shouldn't be used from the > user space. Hmm. If I run this on my current 6.10-based system, I get: $ make CC vdso_standalone_test_x86 CC vdso_test_getrandom vdso_test_getrandom.c:43:41: error: field ‘params’ has incomplete type 43 | struct vgetrandom_opaque_params params; | ^~~~~~ Because KHDR_INCLUDES is /usr/include instead. Christophe, any suggestions on this one? And any idea why loongarch is hitting it, but not x86 or ppc? Jason
On Tue, Aug 27, 2024 at 02:07:59PM +0000, LEROY Christophe wrote: > Le 27/08/2024 à 15:58, Jason A. Donenfeld a écrit : > > On Tue, Aug 27, 2024 at 09:20:16PM +0800, Xi Ruoyao wrote: > >> Building test_vdso_getrandom currently leads to following issue: > >> > >> In file included from /home/xry111/git-repos/linux/tools/include/linux/compiler_types.h:36, > >> from /home/xry111/git-repos/linux/include/uapi/linux/stddef.h:5, > >> from /home/xry111/git-repos/linux/include/uapi/linux/posix_types.h:5, > >> from /usr/include/asm/sigcontext.h:12, > >> from /usr/include/bits/sigcontext.h:30, > >> from /usr/include/signal.h:301, > >> from vdso_test_getrandom.c:14: > >> /home/xry111/git-repos/linux/tools/include/linux/compiler-gcc.h:3:2: error: #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > >> 3 | #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > >> | ^~~~~ > >> > >> It's because the compiler_types.h inclusion in > >> include/uapi/linux/stddef.h is expected to be removed by the > >> header_install.sh script, as compiler_types.h shouldn't be used from the > >> user space. > > > > Hmm. If I run this on my current 6.10-based system, I get: > > > > $ make > > CC vdso_standalone_test_x86 > > CC vdso_test_getrandom > > vdso_test_getrandom.c:43:41: error: field ‘params’ has incomplete type > > 43 | struct vgetrandom_opaque_params params; > > | ^~~~~~ > > > > Because KHDR_INCLUDES is /usr/include instead. > > > > Christophe, any suggestions on this one? And any idea why loongarch is > > hitting it, but not x86 or ppc? > > > > > Can you 'make clean' then provide the output of 'make V=1' ? With*out* this patch, the output is: gcc -std=gnu99 -D_GNU_SOURCE= -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/include -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../include/uapi vdso_test_getrandom.c parse_vdso.c -o /home/zx2c4/Projects/random-linux/tools/testing/selftests/vDSO/vdso_test_getrandom *With* this patch, the output is: gcc -std=gnu99 -D_GNU_SOURCE= -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/include -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include vdso_test_getrandom.c parse_vdso.c -o /home/zx2c4/Projects/random-linux/tools/testing/selftests/vDSO/vdso_test_getrandom vdso_test_getrandom.c:43:41: error: field ‘params’ has incomplete type 43 | struct vgetrandom_opaque_params params; | ^~~~~~ $ ls /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include headers_check.pl Makefile Since I don't build in there, this directory is empty. Jason
On Tue, Aug 27, 2024 at 10:26:53PM +0800, Xi Ruoyao wrote: > > And then use addi.w here with the integer literals instead? > > LoongArch addi.w can only handle 12-bit signed immediate values (such a > limitation is very common in RISC machines). On my processor I can > avoid using a register to materialize the constant with addu16i.d + > addu12i.w + addi.w. But there would be 3 instructions, and addu12i.w is > a part of the Loongson Binary Translation extension which is not > available on some processors. Also LBT isn't intended for general use, > so most LBT instructions have a lower throughput than the basic > instructions. Very interesting, thanks for the explanation. Jason
On Tue, Aug 27, 2024 at 04:50:59PM +0200, Christophe Leroy wrote: > > > Le 27/08/2024 à 16:41, Xi Ruoyao a écrit : > > On Tue, 2024-08-27 at 16:15 +0200, Jason A. Donenfeld wrote: > > > > /* snip */ > > > >> gcc -std=gnu99 -D_GNU_SOURCE= -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/include -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include vdso_test_getrandom.c parse_vdso.c -o /home/zx2c4/Projects/random-linux/tools/testing/selftests/vDSO/vdso_test_getrandom > >> vdso_test_getrandom.c:43:41: error: field ‘params’ has incomplete type > >> 43 | struct vgetrandom_opaque_params params; > >> | ^~~~~~ > >> > >> $ ls /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include > >> headers_check.pl Makefile > >> > >> Since I don't build in there, this directory is empty. > > > > In the toplevel Makefile: > > > > kselftest-%: headers FORCE > > $(Q)$(MAKE) -C $(srctree)/tools/testing/selftests $* > > > > So running "make kselftest-all" to build the self tests should have > > already caused make to build the "headers" target, which puts the > > headers into usr/include. > > > > I don't think it's supported to build self tests w/o invoking the > > toplevel Makefile: many other self tests use KHDR_INCLUDES as well, so > > generally building with something like "make -C tools/testing/selftests" > > just won't work. > > > > My usr/include/ is also empty (only Makefile and headers_check.pl) and > building directly in tools/testing/selftests/vDSO works for me. > > The command is: > > ppc-linux-gcc -std=gnu99 -D_GNU_SOURCE= -isystem > /home/chleroy/linux-powerpc/tools/testing/selftests/../../../tools/include > -isystem > /home/chleroy/linux-powerpc/tools/testing/selftests/../../../include/uapi > vdso_test_getrandom.c parse_vdso.c -o > /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_getrandom > > I believe I get the needed headers through : -isystem > /home/chleroy/linux-powerpc/tools/testing/selftests/../../../include/uapi The effect of this patch is to replace include/uapi with usr/include, so it will break for you too. What I'm wondering is why yours and mine work like that, while Ruoyao's breaks. He makes a good argument as to why this patch is the "right way", even if it breaks our workflow... > > Christophe > > PS: By the way, did you see the -DBULID_VDSO for the chacha test ? Don't > know the impact though .... Yes and https://lore.kernel.org/all/20240827145454.3317093-1-Jason@zx2c4.com/
On Tue, 2024-08-27 at 21:20 +0800, Xi Ruoyao wrote: > diff --git a/arch/loongarch/vdso/vgetrandom.c b/arch/loongarch/vdso/vgetrandom.c > new file mode 100644 > index 000000000000..b9142a5b5d77 > --- /dev/null > +++ b/arch/loongarch/vdso/vgetrandom.c > @@ -0,0 +1,12 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 Xi Ruoyao <xry111@xry111.site>. All Rights Reserved. > + */ > +#include <linux/types.h> > + > +ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags, > + void *opaque_state, size_t opaque_len) Self note: I got a -Wmissing-prototype warning here and it needs to be fixed in v4. I had > +{ > + return __cvdso_getrandom(buffer, len, flags, opaque_state, > + opaque_len); > +}
Le 27/08/2024 à 17:00, Jason A. Donenfeld a écrit : > On Tue, Aug 27, 2024 at 04:50:59PM +0200, Christophe Leroy wrote: >> >> >> Le 27/08/2024 à 16:41, Xi Ruoyao a écrit : >>> On Tue, 2024-08-27 at 16:15 +0200, Jason A. Donenfeld wrote: >>> >>> /* snip */ >>> >>>> gcc -std=gnu99 -D_GNU_SOURCE= -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/include -isystem /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include vdso_test_getrandom.c parse_vdso.c -o /home/zx2c4/Projects/random-linux/tools/testing/selftests/vDSO/vdso_test_getrandom >>>> vdso_test_getrandom.c:43:41: error: field ‘params’ has incomplete type >>>> 43 | struct vgetrandom_opaque_params params; >>>> | ^~~~~~ >>>> >>>> $ ls /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../usr/include >>>> headers_check.pl Makefile >>>> >>>> Since I don't build in there, this directory is empty. >>> >>> In the toplevel Makefile: >>> >>> kselftest-%: headers FORCE >>> $(Q)$(MAKE) -C $(srctree)/tools/testing/selftests $* >>> >>> So running "make kselftest-all" to build the self tests should have >>> already caused make to build the "headers" target, which puts the >>> headers into usr/include. >>> >>> I don't think it's supported to build self tests w/o invoking the >>> toplevel Makefile: many other self tests use KHDR_INCLUDES as well, so >>> generally building with something like "make -C tools/testing/selftests" >>> just won't work. >>> >> >> My usr/include/ is also empty (only Makefile and headers_check.pl) and >> building directly in tools/testing/selftests/vDSO works for me. >> >> The command is: >> >> ppc-linux-gcc -std=gnu99 -D_GNU_SOURCE= -isystem >> /home/chleroy/linux-powerpc/tools/testing/selftests/../../../tools/include >> -isystem >> /home/chleroy/linux-powerpc/tools/testing/selftests/../../../include/uapi >> vdso_test_getrandom.c parse_vdso.c -o >> /home/chleroy/linux-powerpc/tools/testing/selftests/vDSO/vdso_test_getrandom >> >> I believe I get the needed headers through : -isystem >> /home/chleroy/linux-powerpc/tools/testing/selftests/../../../include/uapi > > The effect of this patch is to replace include/uapi with usr/include, so > it will break for you too. > > What I'm wondering is why yours and mine work like that, while Ruoyao's > breaks. He makes a good argument as to why this patch is the "right > way", even if it breaks our workflow... Ah yes he is probably right. Then I'll have to first do: make CROSS_COMPILE=powerpc64-linux- ARCH=powerpc headers Then everything should be fine. Christophe > >> >> Christophe >> >> PS: By the way, did you see the -DBULID_VDSO for the chacha test ? Don't >> know the impact though .... > > Yes and https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20240827145454.3317093-1-Jason%40zx2c4.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C627846cc0a0e429e45c508dcc6a90690%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638603676502990226%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=H7PPFv8QGHsb9xh3J%2FzyeFrpvDu2uSKqx4ZwrPNwC2s%3D&reserved=0
On Tue, 2024-08-27 at 17:10 +0200, Jason A. Donenfeld wrote: > On Tue, Aug 27, 2024 at 11:05:14PM +0800, Xi Ruoyao wrote: > > On Tue, 2024-08-27 at 17:00 +0200, Jason A. Donenfeld wrote: > > > The effect of this patch is to replace include/uapi with usr/include, so > > > it will break for you too. > > > > > > What I'm wondering is why yours and mine work like that, while Ruoyao's > > > breaks. He makes a good argument as to why this patch is the "right > > > way", even if it breaks our workflow... > > > > Because arch/loongarch/include/uapi/asm/sigcontext.h includes > > <linux/posix_types.h>, but the files for x86 and ppc do not. > > > > I cannot see how this inclusion is useful anyway, so maybe I can just > > remove the inclusion and paper over the real issue for now? > > The kselftest people might disagree with papering it over and may prefer > your patch, but your solution does sound better to me... Oops the papering over does not really work because the compiler picks up the sigcontext.h already installed to /usr/include/asm/sigcontext.h. So, if we want to use the "updated" version of sigcontext.h, we still have to add KHDR_INCLUDES to pick up kernel-tree/usr/include/asm/sigcontext.h instead. Or, I can add $(KHDR_INCLUDES) but also keep -isystem $(top_srcdir)/include/uapi, so "make -C tools/testing/selftests TARGETS=vDSO" will still (happens to) work on x86 and ppc (without headers in kernel-tree/usr). If you agree I'll use this approach in v5.
On Tue, Aug 27, 2024 at 05:29:56PM +0200, Jason A. Donenfeld wrote: > On Tue, Aug 27, 2024 at 5:29 PM Xi Ruoyao <xry111@xry111.site> wrote: > > Or, I can add $(KHDR_INCLUDES) but also keep > > -isystem $(top_srcdir)/include/uapi, so "make -C tools/testing/selftests > > TARGETS=vDSO" will still (happens to) work on x86 and ppc (without > > headers in kernel-tree/usr). > > Oh, the porquenolosdos solution. That sounds good to me. Does the below work for you?
On Tue, Aug 27, 2024 at 09:20:14PM +0800, Xi Ruoyao wrote: > +extern void __arch_chacha20_blocks_nostack(u8 *dst_bytes, const u32 *key, > + u32 *counter, size_t nblocks); As of the latest master, the forward declaration here isn't necessary.
For the rationale to implement getrandom() in vDSO see [1]. The vDSO getrandom() needs a stack-less ChaCha20 implementation, so we need to add architecture-specific code and wire it up with the generic code. The implementation is tested with the kernel vDSO selftests, which need to be adapted as well. The selftest changes are also included in the series. The vdso_test_getrandom bench-single result: vdso: 25000000 times in 0.501461533 seconds libc: 25000000 times in 6.975149458 seconds syscall: 25000000 times in 6.985865529 seconds The vdso_test_getrandom bench-multi result: vdso: 25000000 x 256 times in 28.688809414 seconds libc: 25000000 x 256 times in 356.863400242 seconds syscall: 25000000 x 256 times in 338.562183570 seconds [1]:https://lore.kernel.org/all/20240712014009.281406-1-Jason@zx2c4.com/ Cc: linux-crypto@vger.kernel.org Cc: loongarch@lists.linux.dev Cc: Jinyang He <hejinyang@loongson.cn> Cc: Tiezhu Yang <yangtiezhu@loongson.cn> Cc: Arnd Bergmann <arnd@arndb.de> [v3]->v4: - Remove LSX implementation, which isn't much faster than the generic implementaion. - Rebase onto crng/random.git: - Define __arch_get_k_vdso_rng_data instead of using inline asm to provide the _vdso_rng_data symbol in a magic way. - Remove memset.S. - Use c-getrandom-y to easily include the generic C code. - The benchmark results seem better than v3, maybe related to the TLS refactoring in random.git. - Add patches for selftests. [v2]->v3: - Add a generic LoongArch implementation for which LSX isn't needed. v1->v2: - Properly send the series to the list. [v3]:https://lore.kernel.org/all/20240816110717.10249-1-xry111@xry111.site/ [v2]:https://lore.kernel.org/all/20240815133357.35829-1-xry111@xry111.site/ Xi Ruoyao (4): LoongArch: vDSO: Wire up getrandom() vDSO implementation selftests/vDSO: Add --cflags for pkg-config command querying libsodium selftests/vDSO: Use KHDR_INCLUDES to locate UAPI headers for vdso_test_getrandom selftests/vDSO: Enable vdso getrandom tests for LoongArch arch/loongarch/Kconfig | 1 + arch/loongarch/include/asm/vdso/getrandom.h | 47 ++++ arch/loongarch/include/asm/vdso/vdso.h | 8 + arch/loongarch/include/asm/vdso/vsyscall.h | 10 + arch/loongarch/kernel/asm-offsets.c | 10 + arch/loongarch/kernel/vdso.c | 2 + arch/loongarch/vdso/Makefile | 6 + arch/loongarch/vdso/vdso.lds.S | 1 + arch/loongarch/vdso/vgetrandom-chacha.S | 239 ++++++++++++++++++++ arch/loongarch/vdso/vgetrandom.c | 12 + tools/arch/loongarch/vdso | 1 + tools/testing/selftests/vDSO/Makefile | 8 +- 12 files changed, 341 insertions(+), 4 deletions(-) create mode 100644 arch/loongarch/include/asm/vdso/getrandom.h create mode 100644 arch/loongarch/vdso/vgetrandom-chacha.S create mode 100644 arch/loongarch/vdso/vgetrandom.c create mode 120000 tools/arch/loongarch/vdso base-commit: c64dcc01ebf2b7d5a7cb56b5c6a4b6adc2273774