Skip to contentSkip to conversation

selftests: arm64: SVE ptrace test rework

PATCH In review Mark Brown 2 participants 10 messages Sep 13, 2021 – Sep 29, 2021

Subsystems (2)
lore
Share & tools
lore

Mark Brown

author
#0 lore raw

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
author
#1 lore raw

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
index 612d3899614a..7f7ed1c96867 100644
@@ -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
author
#2 lore raw

PATCH 2/8 selftests: arm64: Don't log child creation as a test in SVE ptrace test

In reply to Mark Brown

Currently 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
index 7f7ed1c96867..7035f01423b3 100644
@@ -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
author
#3 lore raw

PATCH 3/8 selftests: arm64: Remove extraneous register setting code

In reply to Mark Brown

For 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
index f2abdd6ba12e..4367125b7c27 100644
@@ -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
deleted file mode 100644
index 3e81f9fab574..000000000000
@@ -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
index 7035f01423b3..d2ec48f649f9 100644
@@ -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
author
#4 lore raw

PATCH 4/8 selftests: arm64: Document what the SVE ptrace test is doing

In reply to Mark Brown

Before 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
index d2ec48f649f9..fc4a672825eb 100644
@@ -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
author
#5 lore raw

PATCH 5/8 selftests: arm64: Clarify output when verifying SVE register set

In reply to Mark Brown

When 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
index fc4a672825eb..2d130fedc019 100644
@@ -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
author
#6 lore raw

PATCH 6/8 selftests: arm64: Verify interoperation of SVE and FPSIMD register sets

In reply to Mark Brown

After 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
index 2d130fedc019..31a2c2fc529d 100644
@@ -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
author
#7 lore raw

PATCH 7/8 selftests: arm64: More comprehensively test the SVE ptrace interface

In reply to Mark Brown

Currently 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
index b6b7ebfcf362..44004e53da33 100644
@@ -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
author
#8 lore raw

PATCH 8/8 selftests: arm64: Move FPSIMD in SVE ptrace test into a function

In reply to Mark Brown

Now 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
index 199710ba65c7..ac0629f05365 100644
@@ -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
maintainer
#9 lore raw
In reply to Mark Brown
Quoted 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