selftests: arm64: SVE ptrace test rework
cover selftests: arm64: SVE ptrace test rework
This series overhauls the selftests we have for the SVE ptrace interface to make them much more comprehensive than they are currently, making the coverage of the data read and written more complete. The new coverage for setting data on all vector lengths showed the issue with using the wrong buffer size with ptrace reported and fixed by:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
(arm64/sve: Use correct size when reinitialising SVE state).
Mark Brown (8):
selftests: arm64: Use a define for the number of SVE ptrace tests to
be run
selftests: arm64: Don't log child creation as a test in SVE ptrace
test
selftests: arm64: Remove extraneous register setting code
selftests: arm64: Document what the SVE ptrace test is doing
selftests: arm64: Clarify output when verifying SVE register set
selftests: arm64: Verify interoperation of SVE and FPSIMD register
sets
selftests: arm64: More comprehensively test the SVE ptrace interface
selftests: arm64: Move FPSIMD in SVE ptrace test into a function
tools/testing/selftests/arm64/fp/Makefile | 2 +- tools/testing/selftests/arm64/fp/TODO | 9 +- .../selftests/arm64/fp/sve-ptrace-asm.S | 33 -- tools/testing/selftests/arm64/fp/sve-ptrace.c | 460 ++++++++++++------ 4 files changed, 321 insertions(+), 183 deletions(-) delete mode 100644 tools/testing/selftests/arm64/fp/sve-ptrace-asm.S
base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
Signature · 1 line
2.20.1
PATCH 1/8 selftests: arm64: Use a define for the number of SVE ptrace tests to be run
Partly in preparation for future refactoring move from hard coding the number of tests in main() to putting #define at the top of the source instead.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +3 -1
@@ -19,6 +19,8 @@#include "../../kselftest.h" +#define EXPECTED_TESTS 20 +/* <linux/elf.h> and <sys/auxv.h> don't like each other, so: */ #ifndef NT_ARM_SVE #define NT_ARM_SVE 0x405
@@ -313,7 +315,7 @@ int main(void)pid_t child;ksft_print_header (); -ksft_set_plan (20 ); +ksft_set_plan (EXPECTED_TESTS );if (!(getauxval (AT_HWCAP ) &HWCAP_SVE ))ksft_exit_skip ("SVE not available\n" );
Signature · 1 line
2.20.1
PATCH 2/8 selftests: arm64: Don't log child creation as a test in SVE ptrace test
In reply to Mark BrownCurrently we log the creation of the child process as a test but it's not really relevant to what we're trying to test and can make the output a little confusing so don't do that.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +1 -3
@@ -19,7 +19,7 @@#include "../../kselftest.h" -#define EXPECTED_TESTS 20 +#define EXPECTED_TESTS 19/* <linux/elf.h> and <sys/auxv.h> don't like each other, so: */ #ifndef NT_ARM_SVE
@@ -169,8 +169,6 @@ static int do_parent(pid_t child)if (WIFEXITED (status) ||WIFSIGNALED (status))ksft_exit_fail_msg ("Child died unexpectedly\n" ); -ksft_test_result (WIFSTOPPED (status),"WIFSTOPPED(%d)\n" , - status);if (!WIFSTOPPED (status)) goto error;
Signature · 1 line
2.20.1
PATCH 3/8 selftests: arm64: Remove extraneous register setting code
In reply to Mark BrownFor some reason the SVE ptrace test code starts off by setting values in some of the SVE vector registers in the parent process which it then never interacts with when verifying the ptrace interfaces. This is not especially relevant to what's being tested and somewhat confusing when reading the code so let's remove it.
--- tools/testing/selftests/arm64/fp/Makefile | 2 +- .../selftests/arm64/fp/sve-ptrace-asm.S | 33 ------------------- tools/testing/selftests/arm64/fp/sve-ptrace.c | 28 ---------------- 3 files changed, 1 insertion(+), 62 deletions(-) delete mode 100644 tools/testing/selftests/arm64/fp/sve-ptrace-asm.S
tools/testing/selftests/arm64/fp/Makefile +1 -1
@@ -12,7 +12,7 @@ all: $(TEST_GEN_PROGS) $(TEST_PROGS_EXTENDED) fpsimd-test: fpsimd-test.o $(CC) -nostdlib $^ -o $@ rdvl-sve: rdvl-sve.o rdvl.o -sve-ptrace: sve-ptrace.o sve-ptrace-asm.o +sve-ptrace: sve-ptrace.o sve-probe-vls: sve-probe-vls.o rdvl.o sve-test: sve-test.o $(CC) -nostdlib $^ -o $@
tools/testing/selftests/arm64/fp/sve-ptrace-asm.S deleted +0 -33
@@ -1,33 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -// Copyright (C) 2015-2019 ARM Limited. -// Original author: Dave Martin <[email protected]> -#include <asm/unistd.h> - -.arch_extension sve - -.globl sve_store_patterns - -sve_store_patterns: - mov x1, x0 - - index z0.b, #0, #1 - str q0, [x1] - - mov w8, #__NR_getpid - svc #0 - str q0, [x1, #0x10] - - mov z1.d, z0.d - str q0, [x1, #0x20] - - mov w8, #__NR_getpid - svc #0 - str q0, [x1, #0x30] - - mov z1.d, z0.d - str q0, [x1, #0x40] - - ret - -.size sve_store_patterns, . - sve_store_patterns -.type sve_store_patterns, @function
tools/testing/selftests/arm64/fp/sve-ptrace.c +0 -28
@@ -26,11 +26,6 @@#define NT_ARM_SVE 0x405#endif -/* Number of registers filled in by sve_store_patterns */ -#define NR_VREGS 5 - -void sve_store_patterns (__uint128_t v[NR_VREGS ]); -static void dump (const void *buf,size_t size) {size_t i;
@@ -40,23 +35,6 @@ static void dump(const void *buf, size_t size)printf (" %.2x" , *p++); } -static int check_vregs (const __uint128_t vregs[NR_VREGS ]) -{ -int i; -int ok =1 ; - -for (i =0 ; i <NR_VREGS ; ++i) { -printf ("# v[%d]:" , i); -dump (&vregs[i],sizeof vregs[i]); -putchar ('\n' ); - -if (vregs[i] != vregs[0 ]) - ok =0 ; - } - -return ok; -} -static int do_child (void ) {if (ptrace (PTRACE_TRACEME ,-1 ,NULL ,NULL ))
@@ -309,7 +287,6 @@ static int do_parent(pid_t child)int main (void ) {int ret =EXIT_SUCCESS ; -__uint128_t v[NR_VREGS ];pid_t child;ksft_print_header ();
@@ -318,11 +295,6 @@ int main(void)if (!(getauxval (AT_HWCAP ) &HWCAP_SVE ))ksft_exit_skip ("SVE not available\n" ); -sve_store_patterns (v); - -if (!check_vregs (v)) -ksft_exit_fail_msg ("Initial check_vregs() failed\n" ); - child =fork ();if (!child)return do_child ();
Signature · 1 line
2.20.1
PATCH 4/8 selftests: arm64: Document what the SVE ptrace test is doing
In reply to Mark BrownBefore we go modifying it further let's add some comments and output clarifications explaining what this test is actually doing.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +8 -3
@@ -181,6 +181,7 @@ static int do_parent(pid_t child) } } +/* New process should start with FPSIMD registers only */ sve =get_sve (pid, &svebuf, &svebufsz);if (!sve) {int e = errno;
@@ -191,14 +192,15 @@ static int do_parent(pid_t child) goto error; }else { -ksft_test_result_pass ("get_sve\n" ); +ksft_test_result_pass ("get_sve(FPSIMD)\n" ); }ksft_test_result ((sve->flags &SVE_PT_REGS_MASK ) ==SVE_PT_REGS_FPSIMD , -"FPSIMD registers\n" ); +"Set FPSIMD registers\n" );if ((sve->flags &SVE_PT_REGS_MASK ) !=SVE_PT_REGS_FPSIMD ) goto error; +/* Try to set a known FPSIMD state via PT_REGS_SVE */ fpsimd = (struct user_fpsimd_state *)((char *)sve +SVE_PT_FPSIMD_OFFSET );for (i =0 ; i <32 ; ++i) {
@@ -219,6 +221,7 @@ static int do_parent(pid_t child) goto error; } +/* Zero the first SVE Z register */ vq =sve_vq_from_vl (sve->vl); newsvebufsz =SVE_PT_SVE_ZREG_OFFSET (vq,1 );
@@ -245,6 +248,7 @@ static int do_parent(pid_t child) goto error; } +/* Try to read back the value we just set */ new_sve =get_sve (pid, &newsvebuf, &newsvebufsz);if (!new_sve) {int e = errno;
@@ -257,12 +261,13 @@ static int do_parent(pid_t child) }ksft_test_result ((new_sve->flags &SVE_PT_REGS_MASK ) ==SVE_PT_REGS_SVE , -"SVE registers\n" ); +"Get SVE registers\n" );if ((new_sve->flags &SVE_PT_REGS_MASK ) !=SVE_PT_REGS_SVE ) goto error;dump_sve_regs (new_sve,3 ,sizeof fpsimd->vregs[0 ]); +/* Verify that the register we set has the value we expected */ p = (unsigned char *)new_sve +SVE_PT_SVE_ZREG_OFFSET (vq,1 );for (i =0 ; i <sizeof fpsimd->vregs[0 ]; ++i) {unsigned char expected = i;
Signature · 1 line
2.20.1
PATCH 5/8 selftests: arm64: Clarify output when verifying SVE register set
In reply to Mark BrownWhen verifying setting a Z register via ptrace we check each byte by hand, iterating over the buffer using a pointer called p and treating each register value written as a test. This creates output referring to "p[X]" which is confusing since SVE also has predicate registers Pn. Tweak the output to avoid confusion here.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +1 -1
@@ -275,7 +275,7 @@ static int do_parent(pid_t child)if (__BYTE_ORDER == __BIG_ENDIAN) expected =sizeof fpsimd->vregs[0 ] -1 - expected; -ksft_test_result (p[i] == expected,"p[%d] == expected\n" , i); +ksft_test_result (p[i] == expected,"buf[%d] == expected\n" , i);if (p[i] != expected) goto error; }
Signature · 1 line
2.20.1
PATCH 6/8 selftests: arm64: Verify interoperation of SVE and FPSIMD register sets
In reply to Mark BrownAfter setting the FPSIMD registers via the SVE register set read them back via the FPSIMD register set, validating that the two register sets are interoperating and that the values we thought we set made it into the child process.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +26 -2
@@ -46,6 +46,15 @@ static int do_child(void)return EXIT_SUCCESS ; } +static int get_fpsimd (pid_t pid,struct user_fpsimd_state *fpsimd) +{ +struct iovec iov; + + iov.iov_base = fpsimd; + iov.iov_len =sizeof (*fpsimd); +return ptrace (PTRACE_GETREGSET , pid,NT_PRFPREG , &iov); +} +static struct user_sve_header *get_sve (pid_t pid,void **buf,size_t *size) {struct user_sve_header *sve;
@@ -122,7 +131,7 @@ static int do_parent(pid_t child)void *svebuf =NULL , *newsvebuf;size_t svebufsz =0 , newsvebufsz;struct user_sve_header *sve, *new_sve; -struct user_fpsimd_state *fpsimd; +struct user_fpsimd_state *fpsimd, new_fpsimd;unsigned int i, j;unsigned char *p;unsigned int vq;
@@ -221,7 +230,22 @@ static int do_parent(pid_t child) goto error; } -/* Zero the first SVE Z register */ +/* Verify via the FPSIMD regset */ +if (get_fpsimd (pid, &new_fpsimd)) { +int e = errno; + +ksft_test_result_fail ("get_fpsimd(): %s\n" , +strerror (errno)); +if (e ==ESRCH ) + goto disappeared; + + goto error; + } +if (memcmp (fpsimd, &new_fpsimd,sizeof (*fpsimd)) ==0 ) +ksft_test_result_pass ("get_fpsimd() gave same state\n" ); +else +ksft_test_result_fail ("get_fpsimd() gave different state\n" ); + vq =sve_vq_from_vl (sve->vl); newsvebufsz =SVE_PT_SVE_ZREG_OFFSET (vq,1 );
Signature · 1 line
2.20.1
PATCH 7/8 selftests: arm64: More comprehensively test the SVE ptrace interface
In reply to Mark BrownCurrently the selftest for the SVE register set is not quite as thorough as is desirable - it only validates that the value of a single Z register is not modified by a partial write to a lower numbered Z register after having previously been set through the FPSIMD regset.
Make this more thorough:
- Test the ability to set vector lengths and enumerate those supported in the system.
- Validate data in all Z and P registers, plus FPSR and FPCR.
- Test reads via the FPSIMD regset after set via the SVE regset.
There's still some oversights, the main one being that due to the need to generate a pattern in FFR and the fact that this rewrite is primarily motivated by SME's streaming SVE which doesn't have FFR we don't currently test FFR. Update the TODO to reflect those that occurred to me (and fix an adjacent typo in there).
--- tools/testing/selftests/arm64/fp/TODO | 9 +- tools/testing/selftests/arm64/fp/sve-ptrace.c | 327 +++++++++++++----- 2 files changed, 254 insertions(+), 82 deletions(-)
tools/testing/selftests/arm64/fp/TODO +6 -3
@@ -1,4 +1,7 @@ - Test unsupported values in the ABIs. -- More coverage for ptrace (eg, vector length conversions). -- Coverage for signals. -- Test PR_SVE_VL_INHERITY after a double fork. +- More coverage for ptrace: + - Get/set of FFR. + - Ensure ptraced processes actually see the register state visible through + the ptrace interface. + - Big endian. +- Test PR_SVE_VL_INHERIT after a double fork.
tools/testing/selftests/arm64/fp/sve-ptrace.c +248 -79
389 lines not on this page · raw message
Signature · 1 line
2.20.1
PATCH 8/8 selftests: arm64: Move FPSIMD in SVE ptrace test into a function
In reply to Mark BrownNow that all the other tests are in functions rather than inline in the main parent process function also move the test for accessing the FPSIMD registers via the SVE regset out into their own function.
--- tools/testing/selftests/arm64/fp/sve-ptrace.c | 120 +++++++++--------- 1 file changed, 59 insertions(+), 61 deletions(-)
tools/testing/selftests/arm64/fp/sve-ptrace.c +59 -61
@@ -158,6 +158,63 @@ static void check_u32(unsigned int vl, const char *reg, } } +/* Access the FPSIMD registers via the SVE regset */ +static void ptrace_sve_fpsimd (pid_t child) +{ +void *svebuf =NULL ; +size_t svebufsz =0 ; +struct user_sve_header *sve; +struct user_fpsimd_state *fpsimd, new_fpsimd; +unsigned int i, j; +unsigned char *p; + +/* New process should start with FPSIMD registers only */ + sve =get_sve (child, &svebuf, &svebufsz); +if (!sve) { +ksft_test_result_fail ("get_sve: %s\n" ,strerror (errno)); + +return ; + }else { +ksft_test_result_pass ("get_sve(FPSIMD)\n" ); + } + +ksft_test_result ((sve->flags &SVE_PT_REGS_MASK ) ==SVE_PT_REGS_FPSIMD , +"Set FPSIMD registers\n" ); +if ((sve->flags &SVE_PT_REGS_MASK ) !=SVE_PT_REGS_FPSIMD ) + goto out; + +/* Try to set a known FPSIMD state via PT_REGS_SVE */ + fpsimd = (struct user_fpsimd_state *)((char *)sve + +SVE_PT_FPSIMD_OFFSET ); +for (i =0 ; i <32 ; ++i) { + p = (unsigned char *)&fpsimd->vregs[i]; + +for (j =0 ; j <sizeof (fpsimd->vregs[i]); ++j) + p[j] = j; + } + +if (set_sve (child, sve)) { +ksft_test_result_fail ("set_sve(FPSIMD): %s\n" , +strerror (errno)); + + goto out; + } + +/* Verify via the FPSIMD regset */ +if (get_fpsimd (child, &new_fpsimd)) { +ksft_test_result_fail ("get_fpsimd(): %s\n" , +strerror (errno)); + goto out; + } +if (memcmp (fpsimd, &new_fpsimd,sizeof (*fpsimd)) ==0 ) +ksft_test_result_pass ("get_fpsimd() gave same state\n" ); +else +ksft_test_result_fail ("get_fpsimd() gave different state\n" ); + +out: +free (svebuf); +} +/* Validate attempting to set SVE data and read SVE data */ static void ptrace_set_sve_get_sve_data (pid_t child,unsigned int vl) {
@@ -335,12 +392,6 @@ static int do_parent(pid_t child)pid_t pid;int status;siginfo_t si; -void *svebuf =NULL ; -size_t svebufsz =0 ; -struct user_sve_header *sve; -struct user_fpsimd_state *fpsimd, new_fpsimd; -unsigned int i, j; -unsigned char *p;unsigned int vq, vl;bool vl_supported;
@@ -398,61 +449,8 @@ static int do_parent(pid_t child) } } -/* New process should start with FPSIMD registers only */ - sve =get_sve (pid, &svebuf, &svebufsz); -if (!sve) { -int e = errno; - -ksft_test_result_fail ("get_sve: %s\n" ,strerror (errno)); -if (e ==ESRCH ) - goto disappeared; - - goto error; - }else { -ksft_test_result_pass ("get_sve(FPSIMD)\n" ); - } - -ksft_test_result ((sve->flags &SVE_PT_REGS_MASK ) ==SVE_PT_REGS_FPSIMD , -"Set FPSIMD registers\n" ); -if ((sve->flags &SVE_PT_REGS_MASK ) !=SVE_PT_REGS_FPSIMD ) - goto error; - -/* Try to set a known FPSIMD state via PT_REGS_SVE */ - fpsimd = (struct user_fpsimd_state *)((char *)sve + -SVE_PT_FPSIMD_OFFSET ); -for (i =0 ; i <32 ; ++i) { - p = (unsigned char *)&fpsimd->vregs[i]; - -for (j =0 ; j <sizeof fpsimd->vregs[i]; ++j) - p[j] = j; - } - -if (set_sve (pid, sve)) { -int e = errno; - -ksft_test_result_fail ("set_sve(FPSIMD): %s\n" , -strerror (errno)); -if (e ==ESRCH ) - goto disappeared; - - goto error; - } - -/* Verify via the FPSIMD regset */ -if (get_fpsimd (pid, &new_fpsimd)) { -int e = errno; - -ksft_test_result_fail ("get_fpsimd(): %s\n" , -strerror (errno)); -if (e ==ESRCH ) - goto disappeared; - - goto error; - } -if (memcmp (fpsimd, &new_fpsimd,sizeof (*fpsimd)) ==0 ) -ksft_test_result_pass ("get_fpsimd() gave same state\n" ); -else -ksft_test_result_fail ("get_fpsimd() gave different state\n" ); +/* FPSIMD via SVE regset */ +ptrace_sve_fpsimd (child);/* Step through every possible VQ */ for (vq =SVE_VQ_MIN ; vq <=SVE_VQ_MAX ; vq++) {
Signature · 1 line
2.20.1
Will Deacon
maintainerQuoted 9 lines · Mark Brown
This series overhauls the selftests we have for the SVE ptrace interface to make them much more comprehensive than they are currently, making the coverage of the data read and written more complete. The new coverage for setting data on all vector lengths showed the issue with using the wrong buffer size with ptrace reported and fixed by:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
[...]
Applied to arm64 (for-next/kselftest), thanks!
[1/8] selftests: arm64: Use a define for the number of SVE ptrace tests to be run
https://git.kernel.org/arm64/c/78d2d816c45a [2/8] selftests: arm64: Don't log child creation as a test in SVE ptrace test https://git.kernel.org/arm64/c/09121ad7186e [3/8] selftests: arm64: Remove extraneous register setting code https://git.kernel.org/arm64/c/eab281e3afa6 [4/8] selftests: arm64: Document what the SVE ptrace test is doing https://git.kernel.org/arm64/c/736e6d5a5451 [5/8] selftests: arm64: Clarify output when verifying SVE register set https://git.kernel.org/arm64/c/8c9eece0bfbf [6/8] selftests: arm64: Verify interoperation of SVE and FPSIMD register sets https://git.kernel.org/arm64/c/9f7d03a2c5a1 [7/8] selftests: arm64: More comprehensively test the SVE ptrace interface https://git.kernel.org/arm64/c/a1d7111257cd [8/8] selftests: arm64: Move FPSIMD in SVE ptrace test into a function https://git.kernel.org/arm64/c/34785030dc06
Cheers,
Signature · 5 lines
Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev