| Message ID | 20240913130544.2398678-1-hca@linux.ibm.com |
|---|---|
| Headers | show |
| Series | s390/vdso: getrandom() vdso implementation | expand |
On Fri, Sep 13, 2024 at 03:05:43PM +0200, Heiko Carstens wrote: > Provide the s390 specific vdso getrandom() architecture backend. > > _vdso_rng_data required data is placed within the _vdso_data vvar page, by > using a hardcoded offset larger than vdso_data. > > As required the chacha20 implementation does not write to the stack. > > The implementation follows more or less the arm64 implementations and > makes use of vector instructions. It has a fallback to the getrandom() > system call for machines where the vector facility is not > installed. > The check if the vector facility is installed, as well as an > optimization for machines with the vector-enhancements facility 2, > is implemented with alternatives, avoiding runtime checks. > > Note that __kernel_getrandom() is implemented without the vdso user wrapper > which would setup a stack frame for odd cases (aka very old glibc variants) > where the caller has not done that. All callers of __kernel_getrandom() are > required to setup a stack frame, like the C ABI requires it. > > The vdso testcases vdso_test_getrandom and vdso_test_chacha pass. I'd be curious to see the results of ./vdso_test_getrandom bench-single and such. Jason
On Fri, Sep 13, 2024 at 03:52:39PM +0200, Jason A. Donenfeld wrote: > Hey Heiko, > > On Fri, Sep 13, 2024 at 03:05:36PM +0200, Heiko Carstens wrote: > > Hi Jason, > > > > quite late but finally the s390 vdso getrandom implementation which applies > > on top of your random git tree. > > > > As a prerequisite this requires some changes to s390 core code to allow > > alternatives in vdso code. It is fine when all of this gets routed via your > > tree. > > On first glance, this series looks perfect. I can't comment too much on > the s390 parts, but first pass of the crypto/vdso/api parts looks spot > on. Nice going. > > Were you thinking you'd like me to take these via the random.git tree > for 6.12 next week, or were you thinking of delaying it a release and > taking it into the arch tree for 6.13? If you did want it to be in 6.12, assuming this series continues to look good, I think we'd still want it to be in -next for at least a week, so maybe that'd take the form of me sending an additional late pull during the merge window for this. Either way, I'll defer to your judgement here, as most of these changes are fiddly s390 things more than anything else. Jason
On Fri, Sep 13, 2024 at 03:53:43PM +0200, Jason A. Donenfeld wrote: > On Fri, Sep 13, 2024 at 03:05:43PM +0200, Heiko Carstens wrote: > > The vdso testcases vdso_test_getrandom and vdso_test_chacha pass. > > I'd be curious to see the results of ./vdso_test_getrandom bench-single > and such. It looks like this with two layers of hypervisors in between, but that shouldn't matter too much for this type of workload: $ ./vdso_test_getrandom bench-single vdso: 25000000 times in 0.493703559 seconds libc: 25000000 times in 6.371764073 seconds syscall: 25000000 times in 6.584025337 seconds
On Fri, Sep 13, 2024 at 04:29:24PM +0200, Heiko Carstens wrote: > Hi Jason, > > > > On first glance, this series looks perfect. I can't comment too much on > > > the s390 parts, but first pass of the crypto/vdso/api parts looks spot > > > on. Nice going. > > > > > > Were you thinking you'd like me to take these via the random.git tree > > > for 6.12 next week, or were you thinking of delaying it a release and > > > taking it into the arch tree for 6.13? > > > > If you did want it to be in 6.12, assuming this series continues to look > > good, I think we'd still want it to be in -next for at least a week, so > > maybe that'd take the form of me sending an additional late pull during > > the merge window for this. Either way, I'll defer to your judgement > > here, as most of these changes are fiddly s390 things more than anything > > else. > > This series is intended to go into 6.12. I don't see a reason to delay > this for a full release cycle. If something breaks we'll fix it, as usual. > > So a late pull request would be perfectly fine. Alternatively we can > take this via s390 also for a second pull request; whatever you prefer > and is less work for you. Okay, great. I'll queue it up then in random.git. Jason
On Fri, Sep 13, 2024 at 05:13:28PM +0200, Jason A. Donenfeld wrote: > On Fri, Sep 13, 2024 at 03:05:43PM +0200, Heiko Carstens wrote: > > The vdso testcases vdso_test_getrandom and vdso_test_chacha pass. > > I'm trying to cross compile this but I'm getting: > > CC vdso_test_chacha > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S: Assembler messages: > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S:147: Error: Unrecognized opcode: `alsih' > > Any idea what's up? Looks like I needed `-march=arch9`. I can potentially rebuild my toolchains to do this by default, though, if that's a normal thing to have and this is just my toolchain being crappy. Or, if it's not a normal thing to have, do we need to add it to the selftests Makefile?
On Fri, Sep 13, 2024 at 05:22:09PM +0200, Jason A. Donenfeld wrote: > On Fri, Sep 13, 2024 at 05:13:28PM +0200, Jason A. Donenfeld wrote: > > On Fri, Sep 13, 2024 at 03:05:43PM +0200, Heiko Carstens wrote: > > > The vdso testcases vdso_test_getrandom and vdso_test_chacha pass. > > > > I'm trying to cross compile this but I'm getting: > > > > CC vdso_test_chacha > > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S: Assembler messages: > > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S:147: Error: Unrecognized opcode: `alsih' > > > > Any idea what's up? > > Looks like I needed `-march=arch9`. I can potentially rebuild my > toolchains to do this by default, though, if that's a normal thing to > have and this is just my toolchain being crappy. Or, if it's not a > normal thing to have, do we need to add it to the selftests Makefile? That needs to be fixed differently, since the kernel build would also fail when building for z10. Could you squash the below fix into this patch, please? That way the compiler will still generate the correct code even if compiled with a lower march flag. There is already a guard in place (test_facility()), which prevents that this code will be executed if the machine does not know the instruction. So for the kernel itself including the vdso code, everything is correct now. But similar checks are missing within vdso_test_chacha.c. I'll provide something for that, so that the test case will be skipped if the required instructions are missing, but not today. diff --git a/arch/s390/kernel/vdso64/vgetrandom-chacha.S b/arch/s390/kernel/vdso64/vgetrandom-chacha.S index ecd44cf0eaba..d802b0a96f41 100644 --- a/arch/s390/kernel/vdso64/vgetrandom-chacha.S +++ b/arch/s390/kernel/vdso64/vgetrandom-chacha.S @@ -144,7 +144,8 @@ SYM_FUNC_START(__arch_chacha20_blocks_nostack) .Lstoredone: /* ++COPY3.COUNTER */ - alsih %r3,1 + /* alsih %r3,1 */ + .insn rilu,0xcc0a00000000,%r3,1 alcr %r3,%r1 VLVGG COPY3,%r3,0
On Fri, Sep 13, 2024 at 09:23:20PM +0200, Jason A. Donenfeld wrote: > > > > CC vdso_test_chacha > > > > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S: Assembler messages: > > > > /home/zx2c4/Projects/random-linux/tools/testing/selftests/../../../tools/arch/s390/vdso/vgetrandom-chacha.S:147: Error: Unrecognized opcode: `alsih' > > > > > > > > Any idea what's up? > > > > > > Looks like I needed `-march=arch9`. I can potentially rebuild my > > > toolchains to do this by default, though, if that's a normal thing to > > > have and this is just my toolchain being crappy. Or, if it's not a > > > normal thing to have, do we need to add it to the selftests Makefile? > > > > That needs to be fixed differently, since the kernel build would also > > fail when building for z10. Could you squash the below fix into this > > patch, please? > > Done. > > > So for the kernel itself including the vdso code, everything is > > correct now. But similar checks are missing within vdso_test_chacha.c. > > I'll provide something for that, so that the test case will be skipped > > if the required instructions are missing, but not today. > > Okay. I would assume no rush there, because it's unlikely there are > those machines part of kselftest fleets anyway? There was another surprise waiting for me: the ALTERNATIVE macro within the tools header file is defined in a way that it omits everything. So I was just lucky that the s390 chacha assembler code worked, since even without the alternatives the code is working, but executes code for newer CPU generations, which it shouldn't. So below is a diff which fixes both: - Add an s390 specific ALTERNATIVE macro that emits code that is supposed to work on older CPU generations, instead of no code - Add a hwcap check to make sure that all CPU capabilities required to run the assembler code are present It probably makes sense to squash this also into "s390/vdso: Wire up getrandom() vdso implementation". Please feel free to change the code in whatever way you like. If you prefer separate patches, I will provide them. diff --git a/tools/include/asm/alternative.h b/tools/include/asm/alternative.h index 7ce02a223732..68dc894c0892 100644 --- a/tools/include/asm/alternative.h +++ b/tools/include/asm/alternative.h @@ -2,8 +2,18 @@ #ifndef _TOOLS_ASM_ALTERNATIVE_ASM_H #define _TOOLS_ASM_ALTERNATIVE_ASM_H +#if defined(__s390x__) +#ifdef __ASSEMBLY__ +.macro ALTERNATIVE oldinstr, newinstr, feature + \oldinstr +.endm +#endif +#else + /* Just disable it so we can build arch/x86/lib/memcpy_64.S for perf bench: */ #define ALTERNATIVE # #endif + +#endif diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha.c b/tools/testing/selftests/vDSO/vdso_test_chacha.c index e81d72c9882e..f1eace68a63b 100644 --- a/tools/testing/selftests/vDSO/vdso_test_chacha.c +++ b/tools/testing/selftests/vDSO/vdso_test_chacha.c @@ -5,11 +5,34 @@ #include <tools/le_byteshift.h> #include <sys/random.h> +#include <sys/auxv.h> #include <string.h> #include <stdint.h> #include <stdbool.h> #include "../kselftest.h" +#if defined(__s390x__) + +#ifndef HWCAP_S390_VX +#define HWCAP_S390_VX 2048 +#endif + +static bool cpu_has_capabilities(void) +{ + if (getauxval(AT_HWCAP) & HWCAP_S390_VX) + return true; + return false; +} + +#else + +static bool cpu_has_capabilities(void) +{ + return true; +} + +#endif + static uint32_t rol32(uint32_t word, unsigned int shift) { return (word << (shift & 31)) | (word >> ((-shift) & 31)); @@ -67,6 +90,8 @@ int main(int argc, char *argv[]) uint8_t output1[BLOCK_SIZE * BLOCKS], output2[BLOCK_SIZE * BLOCKS]; ksft_print_header(); + if (!cpu_has_capabilities()) + ksft_exit_skip("Required CPU capabilities missing\n"); ksft_set_plan(1); for (unsigned int trial = 0; trial < TRIALS; ++trial) {
Hi Heiko, On Sat, Sep 14, 2024 at 07:42:46PM +0200, Heiko Carstens wrote: > Please feel free to change the code in whatever way you like. > If you prefer separate patches, I will provide them. Just wanted to make sure you saw https://lore.kernel.org/all/20240914231241.3647749-1-Jason@zx2c4.com/ Jason
Hi Jason, > On Sat, Sep 14, 2024 at 07:42:46PM +0200, Heiko Carstens wrote: > > Please feel free to change the code in whatever way you like. > > If you prefer separate patches, I will provide them. > > Just wanted to make sure you saw https://lore.kernel.org/all/20240914231241.3647749-1-Jason@zx2c4.com/ Yes, looks good to me. I just gave it also a quick test. FWIW, I think the tags for the commit message should be Co-developed-by: Heiko Carstens <hca@linux.ibm.com> Signed-off-by: Heiko Carstens <hca@linux.ibm.com> However, if you keep it as it is, that's also fine. Thanks a lot! Heiko
On Mon, Sep 16, 2024 at 01:01:08PM +0200, Heiko Carstens wrote: > Hi Jason, > > > On Sat, Sep 14, 2024 at 07:42:46PM +0200, Heiko Carstens wrote: > > > Please feel free to change the code in whatever way you like. > > > If you prefer separate patches, I will provide them. > > > > Just wanted to make sure you saw https://lore.kernel.org/all/20240914231241.3647749-1-Jason@zx2c4.com/ > > Yes, looks good to me. I just gave it also a quick test. > > FWIW, I think the tags for the commit message should be > > Co-developed-by: Heiko Carstens <hca@linux.ibm.com> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> Thanks, fixed.
Hi Jason, quite late but finally the s390 vdso getrandom implementation which applies on top of your random git tree. As a prerequisite this requires some changes to s390 core code to allow alternatives in vdso code. It is fine when all of this gets routed via your tree. Please note that the first patch of this series is already in linux-next [1], but given that it is so small it seems to be the easiest to apply it also on your tree; but I'm sure somebody will complain :) Thanks, Heiko [1] 0147addc4fb7 ("s390/facility: Disable compile time optimization for decompressor code") Heiko Carstens (7): s390/facility: Disable compile time optimization for decompressor code s390/alternatives: Remove ALT_FACILITY_EARLY s390/facility: Let test_facility() generate static branch if possible s390/module: Provide find_section() helper s390/vdso: Allow alternatives in vdso code s390/vdso: Move vdso symbol handling to separate header file s390/vdso: Wire up getrandom() vdso implementation arch/s390/Kconfig | 1 + arch/s390/include/asm/alternative.h | 6 +- arch/s390/include/asm/facility.h | 37 +++- arch/s390/include/asm/fpu-insn-asm.h | 22 +++ arch/s390/include/asm/module.h | 14 ++ arch/s390/include/asm/vdso-symbols.h | 17 ++ arch/s390/include/asm/vdso.h | 12 -- arch/s390/include/asm/vdso/getrandom.h | 40 +++++ arch/s390/include/asm/vdso/vsyscall.h | 15 ++ arch/s390/kernel/compat_signal.c | 2 +- arch/s390/kernel/entry.S | 2 +- arch/s390/kernel/signal.c | 2 +- arch/s390/kernel/vdso.c | 26 ++- arch/s390/kernel/vdso64/Makefile | 9 +- arch/s390/kernel/vdso64/vdso.h | 1 + arch/s390/kernel/vdso64/vdso64.lds.S | 7 + arch/s390/kernel/vdso64/vgetrandom-chacha.S | 184 ++++++++++++++++++++ arch/s390/kernel/vdso64/vgetrandom.c | 14 ++ tools/arch/s390/vdso | 1 + tools/testing/selftests/vDSO/Makefile | 2 +- 20 files changed, 378 insertions(+), 36 deletions(-) create mode 100644 arch/s390/include/asm/vdso-symbols.h create mode 100644 arch/s390/include/asm/vdso/getrandom.h create mode 100644 arch/s390/kernel/vdso64/vgetrandom-chacha.S create mode 100644 arch/s390/kernel/vdso64/vgetrandom.c create mode 120000 tools/arch/s390/vdso