[PATCH v1 0/3] kselftest/arm64: Vector length configuration tests

[PATCH v1 0/3] kselftest/arm64: Vector length configuration tests

[Mark Brown]

Currently we don't have full automated tests for the vector length
configuation ABIs offered for SVE, we have a helper binary for setting
the vector length which can be used for manual tests and we use the
prctl() interface to enumerate the vector lengths but don't actually
verify that the vector lengths enumerated were set.

This patch series provides a small helper which allows us to get the
currently configured vector length using the RDVL instruction via either
a library call or stdout of a process and then uses this to both add
verification of enumerated vector lengths to our existing tests and also
add a new test program which exercises both the prctl() and sysfs
interfaces.

In preparation for the forthcomng support for the Scalable Matrix
Extension (SME) [1] which introduces a new vector length managed via a
very similar hardware interface the helper and new test program are
parameterised with the goal of allowing reuse for SME.

[1] https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/scalable-matrix-extension-armv9-a-architecture

Mark Brown (3):
  kselftest/arm64: Provide a helper binary and "library" for SVE RDVL
  kselftest/arm64: Validate vector lengths are set in sve-probe-vls
  kselftest/arm64: Add tests for SVE vector configuration

 tools/testing/selftests/arm64/fp/.gitignore   |   2 +
 tools/testing/selftests/arm64/fp/Makefile     |  11 +-
 tools/testing/selftests/arm64/fp/rdvl-sve.c   |  14 +
 tools/testing/selftests/arm64/fp/rdvl.S       |   9 +
 tools/testing/selftests/arm64/fp/rdvl.h       |   8 +
 .../selftests/arm64/fp/sve-probe-vls.c        |   5 +
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 578 ++++++++++++++++++
 7 files changed, 624 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c


base-commit: ff1176468d368232b684f75e82563369208bc371
-- 
2.20.1

[PATCH v1 1/3] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

[Mark Brown]

SVE provides an instruction RDVL which reports the currently configured
vector length. In order to validate that our vector length configuration
interfaces are working correctly without having to build the C code for
our test programs with SVE enabled provide a trivial assembly library
with a C callable function that executes RDVL. Since these interfaces
also control behaviour on exec*() provide a trivial wrapper program which
reports the currently configured vector length on stdout, tests can use
this to verify that behaviour on exec*() is as expected.

In preparation for providing similar helper functionality for SME, the
Scalable Matrix Extension, which allows separately configured vector
lengths to be read back both the assembler function and wrapper binary
have SVE included in their name.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore |  1 +
 tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
 tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
 tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
 tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
 create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index d66f76d2a650..6b53a7b60fee 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -1,4 +1,5 @@
 fpsimd-test
+rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index a57009d3a0dc..ed62e7003b96 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -2,12 +2,16 @@
 
 CFLAGS += -I../../../../../usr/include/
 TEST_GEN_PROGS := sve-ptrace sve-probe-vls
-TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
+TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
+	rdvl-sve \
+	sve-test sve-stress \
+	vlset
 
 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-probe-vls: sve-probe-vls.o
 sve-test: sve-test.o
diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
new file mode 100644
index 000000000000..7f8a13a18f5d
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdio.h>
+
+#include "rdvl.h"
+
+int main(void)
+{
+	int vl = rdvl_sve();
+
+	printf("%d\n", vl);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
new file mode 100644
index 000000000000..6e76dd720b87
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.S
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2021 ARM Limited.
+
+.arch_extension sve
+
+.globl rdvl_sve
+rdvl_sve:
+	rdvl	x0, #1
+	ret
diff --git a/tools/testing/selftests/arm64/fp/rdvl.h b/tools/testing/selftests/arm64/fp/rdvl.h
new file mode 100644
index 000000000000..7c9d953fc9e7
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef RDVL_H
+#define RDVL_H
+
+int rdvl_sve(void);
+
+#endif
-- 
2.20.1

[PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

[Mark Brown]

Currently sve-probe-vls does not verify that the vector lengths reported
by the prctl() interface are actually what is reported by the architecture,
use the rdvl_sve() helper to validate this.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/Makefile        | 2 +-
 tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index ed62e7003b96..fa3a0167db2d 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -13,7 +13,7 @@ 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-probe-vls: sve-probe-vls.o
+sve-probe-vls: sve-probe-vls.o rdvl.o
 sve-test: sve-test.o
 	$(CC) -nostdlib $^ -o $@
 vlset: vlset.o
diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
index 76e138525d55..a6cd1bd6e515 100644
--- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c
+++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
@@ -13,6 +13,7 @@
 #include <asm/sigcontext.h>
 
 #include "../../kselftest.h"
+#include "rdvl.h"
 
 int main(int argc, char **argv)
 {
@@ -38,6 +39,10 @@ int main(int argc, char **argv)
 
 		vl &= PR_SVE_VL_LEN_MASK;
 
+		if (rdvl_sve() != vl)
+			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
+					   vl, rdvl_sve());
+
 		if (!sve_vl_valid(vl))
 			ksft_exit_fail_msg("VL %d invalid\n", vl);
 		vq = sve_vq_from_vl(vl);
-- 
2.20.1

[PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Mark Brown]

We provide interfaces for configuring the SVE vector length seen by
processes using prctl and also via /proc for configuring the default
values. Provide tests that exercise all these interfaces and verify that
they take effect as expected, though at present no test fully enumerates
all the possible vector lengths.

A subset of this is already tested via sve-probe-vls but the /proc
interfaces are not currently covered at all.

In preparation for the forthcoming support for SME, the Scalable Matrix
Extension, which has separately but similarly configured vector lengths
which we expect to offer similar userspace interfaces for, all the actual
files and prctls used are parameterised and we don't validate that the
architectural minimum vector length is the minimum we see.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/fp/.gitignore   |   1 +
 tools/testing/selftests/arm64/fp/Makefile     |   3 +-
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 578 ++++++++++++++++++
 3 files changed, 581 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c

diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
index 6b53a7b60fee..b67395903b9b 100644
--- a/tools/testing/selftests/arm64/fp/.gitignore
+++ b/tools/testing/selftests/arm64/fp/.gitignore
@@ -3,4 +3,5 @@ rdvl-sve
 sve-probe-vls
 sve-ptrace
 sve-test
+vec-syscfg
 vlset
diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
index fa3a0167db2d..f2abdd6ba12e 100644
--- a/tools/testing/selftests/arm64/fp/Makefile
+++ b/tools/testing/selftests/arm64/fp/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 CFLAGS += -I../../../../../usr/include/
-TEST_GEN_PROGS := sve-ptrace sve-probe-vls
+TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
 TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
 	rdvl-sve \
 	sve-test sve-stress \
@@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
 sve-probe-vls: sve-probe-vls.o rdvl.o
 sve-test: sve-test.o
 	$(CC) -nostdlib $^ -o $@
+vec-syscfg: vec-syscfg.o rdvl.o
 vlset: vlset.o
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
new file mode 100644
index 000000000000..360c3a7cae26
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 ARM Limited.
+ * Original author: Mark Brown <broonie@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <sys/ptrace.h>
+#include <sys/types.h>
+#include <sys/uio.h>
+#include <sys/wait.h>
+#include <asm/sigcontext.h>
+#include <asm/ptrace.h>
+
+#include "../../kselftest.h"
+#include "rdvl.h"
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+
+#define MIN_VL 16
+
+struct vec_data {
+	const char *name;
+	unsigned long hwcap_type;
+	unsigned long hwcap;
+	const char *rdvl_binary;
+	int (*rdvl)(void);
+
+	int prctl_get;
+	int prctl_set;
+	const char *default_vl_file;
+
+	int default_vl;
+	int min_vl;
+	int max_vl;
+} vec_data[] = {
+	{
+		.name = "SVE",
+		.hwcap_type = AT_HWCAP,
+		.hwcap = HWCAP_SVE,
+		.rdvl = rdvl_sve,
+		.rdvl_binary = "./rdvl-sve",
+		.prctl_get = PR_SVE_GET_VL,
+		.prctl_set = PR_SVE_SET_VL,
+		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
+	},
+};
+
+/* Start a new process and return the vector length it sees */
+int get_child_rdvl(struct vec_data *data)
+{
+	char buf[10];
+	int pipefd[2];
+	pid_t pid, child;
+	int read_vl, ret;
+
+	ret = pipe(pipefd);
+	if (ret == -1) {
+		ksft_print_msg("pipe() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	child = fork();
+	if (child == -1) {
+		ksft_print_msg("fork() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		return -1;
+	}
+
+	/* Child: put vector length on the pipe */
+	if (child == 0) {
+		/*
+		 * Replace stdout with the pipe, errors to stderr from
+		 * here as kselftest prints to stdout.
+		 */
+		ret = dup2(pipefd[1], 1);
+		if (ret == -1) {
+			fprintf(stderr, "dup2() %d\n", errno);
+			exit(-1);
+		}
+
+		/* exec() a new binary which puts the VL on stdout */
+		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
+		fprintf(stderr, "execl(%s) failed: %d\n",
+			data->rdvl_binary, errno, strerror(errno));
+
+		exit(-1);
+	}
+
+	close(pipefd[1]);
+
+	/* Parent; wait for the exit status from the child & verify it */
+	while (1) {
+		pid = wait(&ret);
+		if (pid == -1) {
+			ksft_print_msg("wait() failed: %d (%s)\n",
+				       errno, strerror(errno));
+			return -1;
+		}
+
+		if (pid != child)
+			continue;
+
+		if (!WIFEXITED(ret)) {
+			ksft_print_msg("child exited abnormally\n");
+			return -1;
+		}
+
+		if (WEXITSTATUS(ret) != 0) {
+			ksft_print_msg("child returned error %d\n",
+				       WEXITSTATUS(ret));
+			return -1;
+		}
+
+		memset(buf, 0, sizeof(buf));
+		ret = read(pipefd[0], buf, sizeof(buf) - 1);
+		if (ret <= 0) {
+			ksft_print_msg("read() failed: %d (%s)\n",
+				       errno, strerror(errno));
+			return -1;
+		}
+		close(pipefd[0]);
+
+		ret = sscanf(buf, "%d", &read_vl);
+		if (ret != 1) {
+			ksft_print_msg("failed to parse VL from '%s'\n",
+				       buf);
+			return -1;
+		}
+
+		return read_vl;
+	}
+}
+
+int file_read_integer(const char *name, int *val)
+{
+	char buf[40];
+	int f, ret;
+
+	f = open(name, O_RDONLY);
+	if (f < 0) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	memset(buf, 0, sizeof(buf));
+	ret = read(f, buf, sizeof(buf) - 1);
+	if (ret < 0) {
+		ksft_test_result_fail("Error reading %s: %d (%s)\n",
+				      name, errno, strerror(errno));
+		return -1;
+	}
+	close(f);
+
+	ret = sscanf(buf, "%d", val);
+	if (ret != 1) {
+		ksft_test_result_fail("Failed to parse %s\n", name);
+		return -1;
+	}
+
+	return 0;
+}
+
+int file_write_integer(const char *name, int val)
+{
+	char buf[40];
+	int f, ret;
+
+	f = open(name, O_WRONLY);
+	if (f < 0) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	snprintf(buf, sizeof(buf), "%d", val);
+	ret = write(f, buf, strlen(buf));
+	if (ret < 0) {
+		ksft_test_result_fail("Error writing %d to %s: %d (%s)\n",
+				      val, name, errno, strerror(errno));
+		return -1;
+	}
+	close(f);
+
+	return 0;
+}
+
+/*
+ * Verify that we can read the default VL via proc, checking that it
+ * is set in a freshly spawned child.
+ */
+void proc_read_default(struct vec_data *data)
+{
+	int default_vl, child_vl, ret;
+
+	ret = file_read_integer(data->default_vl_file, &default_vl);
+	if (ret != 0)
+		return;
+
+	/* Is this the actual default seen by new processes? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != default_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      default_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s default vector length %d\n", data->name,
+			      default_vl);
+	data->default_vl = default_vl;
+}
+
+/* Verify that we can write a minimum value and have it take effect */
+void proc_write_min(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	ret = file_write_integer(data->default_vl_file, MIN_VL);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s minimum vector length %d\n", data->name,
+			      new_default);
+	data->min_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Verify that we can write a maximum value and have it take effect */
+void proc_write_max(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	/* -1 is accepted by the /proc interface as the maximum VL */
+	ret = file_write_integer(data->default_vl_file, -1);
+	if (ret != 0)
+		return;
+
+	/* What was the new value? */
+	ret = file_read_integer(data->default_vl_file, &new_default);
+	if (ret != 0)
+		return;
+
+	/* Did it take effect in a new process? */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != new_default) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      new_default, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s maximum vector length %d\n", data->name,
+			      new_default);
+	data->max_vl = new_default;
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* Can we read back a VL from prctl? */
+void prctl_get(struct vec_data *data)
+{
+	int ret;
+
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	/* Mask out any flags */
+	ret &= PR_SVE_VL_LEN_MASK;
+
+	/* Is that what we can read back directly? */
+	if (ret == data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Does the prctl let us set the VL we already have? */
+void prctl_set_same(struct vec_data *data)
+{
+	int cur_vl = data->rdvl();
+	int ret;
+
+	ret = prctl(data->prctl_set, cur_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (cur_vl != data->rdvl())
+		ksft_test_result_pass("%s current VL is %d\n",
+				      data->name, ret);
+	else
+		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
+				      data->name, ret, data->rdvl());
+}
+
+/* Can we set a new VL for this process? */
+void prctl_set(struct vec_data *data)
+{
+	int ret;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Try to set the minimum VL */
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->min_vl) {
+		ksft_test_result_fail("%s prctl set %d but return value is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	if (data->rdvl() != data->min_vl) {
+		ksft_test_result_fail("%s set %d but RDVL is %d\n",
+				      data->name, data->min_vl, data->rdvl());
+		return;
+	}
+
+	/* Try to set the maximum VL */
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->max_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	if ((ret & PR_SVE_VL_LEN_MASK) != data->max_vl) {
+		ksft_test_result_fail("%s prctl() set %d but return value is %d\n",
+				      data->name, data->max_vl, data->rdvl());
+		return;
+	}
+
+	/* The _INHERIT flag should not be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+
+	if (ret & PR_SVE_VL_INHERIT) {
+		ksft_test_result_fail("%s prctl() reports _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	ksft_test_result_pass("%s prctl() set min/max\n", data->name);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_no_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child has the default we just set */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->max_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->max_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length used default\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* If we didn't request it a new VL shouldn't affect the child */
+void prctl_set_for_child(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_VL_INHERIT);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* The _INHERIT flag should be present when we read the VL */
+	ret = prctl(data->prctl_get);
+	if (ret == -1) {
+		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
+				      data->name, errno, strerror(errno));
+		return;
+	}
+	if (!(ret & PR_SVE_VL_INHERIT)) {
+		ksft_test_result_fail("%s prctl() does not report _INHERIT\n",
+				      data->name);
+		return;
+	}
+
+	/* Ensure the default VL is different */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length was inherited\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+/* _ONEXEC takes effect only in the child process */
+void prctl_set_onexec(struct vec_data *data)
+{
+	int ret, child_vl;
+
+	if (data->min_vl == data->max_vl) {
+		ksft_test_result_skip("%s only one VL supported\n",
+				      data->name);
+		return;
+	}
+
+	/* Set a known value for the default and our current VL */
+	ret = file_write_integer(data->default_vl_file, data->max_vl);
+	if (ret != 0)
+		return;
+
+	ret = prctl(data->prctl_set, data->max_vl);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Set a different value for the child to have on exec */
+	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_SET_VL_ONEXEC);
+	if (ret < 0) {
+		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
+				      data->name, data->min_vl,
+				      errno, strerror(errno));
+		return;
+	}
+
+	/* Our current VL should stay the same */
+	if (data->rdvl() != data->max_vl) {
+		ksft_test_result_fail("%s VL changed by _ONEXEC prctl()\n",
+				      data->name);
+		return;
+	}
+
+	/* Check that the child inherited our VL */
+	child_vl = get_child_rdvl(data);
+	if (child_vl != data->min_vl) {
+		ksft_test_result_fail("%s is %d but child VL is %d\n",
+				      data->default_vl_file,
+				      data->min_vl, child_vl);
+		return;
+	}
+
+	ksft_test_result_pass("%s vector length set on exec\n", data->name);
+
+	file_write_integer(data->default_vl_file, data->default_vl);
+}
+
+typedef void (*test_type)(struct vec_data *);
+
+test_type tests[] = {
+	/*
+	 * The default/min/max tests must be first to provide data for
+	 * other tests.
+	 */
+	proc_read_default,
+	proc_write_min,
+	proc_write_max,
+
+	prctl_get,
+	prctl_set,
+	prctl_set_no_child,
+	prctl_set_for_child,
+	prctl_set_onexec,
+};
+
+int main(void)
+{
+	int i, j;
+
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
+
+	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
+		struct vec_data *data = &vec_data[i];
+		int supported = getauxval(data->hwcap_type) & data->hwcap;
+
+		for (j = 0; j < ARRAY_SIZE(tests); j++) {
+			if (supported)
+				tests[j](data);
+			else
+				ksft_test_result_skip("%s not supported\n",
+						      data->name);
+		}
+	}
+
+	ksft_exit_pass();
+}
-- 
2.20.1

Re: [PATCH v1 1/3] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

[Dave Martin]

On Tue, Jul 27, 2021 at 07:06:47PM +0100, Mark Brown wrote:
> SVE provides an instruction RDVL which reports the currently configured
> vector length. In order to validate that our vector length configuration
> interfaces are working correctly without having to build the C code for
> our test programs with SVE enabled provide a trivial assembly library
> with a C callable function that executes RDVL. Since these interfaces
> also control behaviour on exec*() provide a trivial wrapper program which
> reports the currently configured vector length on stdout, tests can use
> this to verify that behaviour on exec*() is as expected.
> 
> In preparation for providing similar helper functionality for SME, the
> Scalable Matrix Extension, which allows separately configured vector
> lengths to be read back both the assembler function and wrapper binary
> have SVE included in their name.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/arm64/fp/.gitignore |  1 +
>  tools/testing/selftests/arm64/fp/Makefile   |  6 +++++-
>  tools/testing/selftests/arm64/fp/rdvl-sve.c | 14 ++++++++++++++
>  tools/testing/selftests/arm64/fp/rdvl.S     |  9 +++++++++
>  tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
>  5 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl-sve.c
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.S
>  create mode 100644 tools/testing/selftests/arm64/fp/rdvl.h
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index d66f76d2a650..6b53a7b60fee 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -1,4 +1,5 @@
>  fpsimd-test
> +rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index a57009d3a0dc..ed62e7003b96 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -2,12 +2,16 @@
>  
>  CFLAGS += -I../../../../../usr/include/
>  TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> -TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress sve-test sve-stress vlset
> +TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
> +	rdvl-sve \
> +	sve-test sve-stress \
> +	vlset
>  
>  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-probe-vls: sve-probe-vls.o
>  sve-test: sve-test.o
> diff --git a/tools/testing/selftests/arm64/fp/rdvl-sve.c b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> new file mode 100644
> index 000000000000..7f8a13a18f5d
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl-sve.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <stdio.h>
> +
> +#include "rdvl.h"
> +
> +int main(void)
> +{
> +	int vl = rdvl_sve();
> +
> +	printf("%d\n", vl);
> +
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/arm64/fp/rdvl.S b/tools/testing/selftests/arm64/fp/rdvl.S
> new file mode 100644
> index 000000000000..6e76dd720b87
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/rdvl.S
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (C) 2021 ARM Limited.
> +
> +.arch_extension sve
> +
> +.globl rdvl_sve
> +rdvl_sve:
> +	rdvl	x0, #1
> +	ret

This works, but can we use an ACLE intrinsic for this?  I'm pretty GCC
and LLVM have been up to date with that stuff for some time, though
you'd have to check with the compiler folks.

Alternatively:

static int rvdl_sve(void)
{
	int vl;

	asm ("rvdl %0, #1" : "=r" (vl));

	return vl;
}

would also work.

&rdvl_sve would not be the same in different translation units, but I
don't think the tests care about that(?)

[...]

Cheers
---Dave

Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

[Dave Martin]

On Tue, Jul 27, 2021 at 07:06:48PM +0100, Mark Brown wrote:
> Currently sve-probe-vls does not verify that the vector lengths reported
> by the prctl() interface are actually what is reported by the architecture,
> use the rdvl_sve() helper to validate this.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/arm64/fp/Makefile        | 2 +-
>  tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index ed62e7003b96..fa3a0167db2d 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -13,7 +13,7 @@ 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-probe-vls: sve-probe-vls.o
> +sve-probe-vls: sve-probe-vls.o rdvl.o
>  sve-test: sve-test.o
>  	$(CC) -nostdlib $^ -o $@
>  vlset: vlset.o
> diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
> index 76e138525d55..a6cd1bd6e515 100644
> --- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c
> +++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c
> @@ -13,6 +13,7 @@
>  #include <asm/sigcontext.h>
>  
>  #include "../../kselftest.h"
> +#include "rdvl.h"
>  
>  int main(int argc, char **argv)
>  {

This test was originally a diagnostic tool, so the way VLs are probed
aims to be efficient, rather than being defensive against the kernel
doing weird stuff.

If the kernel returns a vl > than the one we tried to set, we'll end
up in an infinite loop because of the way the loop deliberately uses the
kernel's return value to skip unsupported VLs.  So, it might be better
to probe every single architecturally possible VL and sanity check
everything instead.

While taking this opportunity, a check that the resulting vl is <= the
requested VL might be good too.

Also, if the kernel rounds any given vl down to some vl_actual, then it
should also round every possible other_vl down to vl_actual, where
vl_actual <= other_vl < vl.  It's not OK for different vls to be rounded
down to different actual values that have nothing to do with each other.

Since this test is our example of how to do this probing, it would be
good to preserve the "efficient" version though, and check that the
probed set of vls matches what is obtained by exhaustive probing.
Using comments and well-chosen function names, we can hopefully steer
people to use the efficient version that trusts the kernel in their own
code.

> @@ -38,6 +39,10 @@ int main(int argc, char **argv)
>  
>  		vl &= PR_SVE_VL_LEN_MASK;
>  
> +		if (rdvl_sve() != vl)
> +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
> +					   vl, rdvl_sve());
> +

If this fails, the VL vl wasn't "Set" at all.  I found this a bit
confusing from just looking at this hunk.

Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?

[...]

Thinking about it, due to the way these tests do inline vector length
changes we should also be making sure that they are not built with SVE
code gen by default, since inline VL changes could cause bad things to
happen in that case.

This isn't a problem for now, but as time goes on the toolchain may
start to be configured to compile for SVE by default.

I'm not sure of the correct option for forcing SVE off against the
compiler default -- check with the tools folks.  If there isn't a
proper way to do this, it's a toolchain defect so we should flag it up,
but -mgeneral-regs-only might work for us even though it's a bit of a
sledgehammer.

Compiler options aside, I think we _probably_ get away with a
native-SVE libc, since if everything in libc is VL-agnostic then it
shouldn't care what VL we call in with.  If being paranoid, we could
always restore the VL before doing anything other then prctl() and
RVDL, but we should probably wait for something to break first.


If we wanted to be super-robust, we could make the business end of each
test a pure-asm binary and control them via ptrace.  But that's probably
too much pain for now, even if the test harness were reusable.

Cheers
---Dave

Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Dave Martin]

On Tue, Jul 27, 2021 at 07:06:49PM +0100, Mark Brown wrote:
> We provide interfaces for configuring the SVE vector length seen by
> processes using prctl and also via /proc for configuring the default
> values. Provide tests that exercise all these interfaces and verify that
> they take effect as expected, though at present no test fully enumerates
> all the possible vector lengths.

Does "at present" mean that this test doesn't do it either?

(It doesn't seem to try every VL, unless I've missed something?  Is this
planned?)

> A subset of this is already tested via sve-probe-vls but the /proc
> interfaces are not currently covered at all.
> 
> In preparation for the forthcoming support for SME, the Scalable Matrix
> Extension, which has separately but similarly configured vector lengths
> which we expect to offer similar userspace interfaces for, all the actual
> files and prctls used are parameterised and we don't validate that the
> architectural minimum vector length is the minimum we see.

This looks like a good addition overall...

> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  tools/testing/selftests/arm64/fp/.gitignore   |   1 +
>  tools/testing/selftests/arm64/fp/Makefile     |   3 +-
>  tools/testing/selftests/arm64/fp/vec-syscfg.c | 578 ++++++++++++++++++
>  3 files changed, 581 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/arm64/fp/vec-syscfg.c
> 
> diff --git a/tools/testing/selftests/arm64/fp/.gitignore b/tools/testing/selftests/arm64/fp/.gitignore
> index 6b53a7b60fee..b67395903b9b 100644
> --- a/tools/testing/selftests/arm64/fp/.gitignore
> +++ b/tools/testing/selftests/arm64/fp/.gitignore
> @@ -3,4 +3,5 @@ rdvl-sve
>  sve-probe-vls
>  sve-ptrace
>  sve-test
> +vec-syscfg
>  vlset
> diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile
> index fa3a0167db2d..f2abdd6ba12e 100644
> --- a/tools/testing/selftests/arm64/fp/Makefile
> +++ b/tools/testing/selftests/arm64/fp/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  CFLAGS += -I../../../../../usr/include/
> -TEST_GEN_PROGS := sve-ptrace sve-probe-vls
> +TEST_GEN_PROGS := sve-ptrace sve-probe-vls vec-syscfg
>  TEST_PROGS_EXTENDED := fpsimd-test fpsimd-stress \
>  	rdvl-sve \
>  	sve-test sve-stress \
> @@ -16,6 +16,7 @@ sve-ptrace: sve-ptrace.o sve-ptrace-asm.o
>  sve-probe-vls: sve-probe-vls.o rdvl.o
>  sve-test: sve-test.o
>  	$(CC) -nostdlib $^ -o $@
> +vec-syscfg: vec-syscfg.o rdvl.o
>  vlset: vlset.o
>  
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> new file mode 100644
> index 000000000000..360c3a7cae26
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 ARM Limited.
> + * Original author: Mark Brown <broonie@kernel.org>
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>

Not used? ^

> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>

Not used? ^

> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/auxv.h>
> +#include <sys/prctl.h>
> +#include <sys/ptrace.h>

Not used? ^

> +#include <sys/types.h>
> +#include <sys/uio.h>

Not used? ^

(This would have been for struct iovec, for ptrace regset access.
I'm guessing some of this #include list was pasted from the existing
ptrace tests?)

> +#include <sys/wait.h>
> +#include <asm/sigcontext.h>
> +#include <asm/ptrace.h>

Not used? ^

> +
> +#include "../../kselftest.h"
> +#include "rdvl.h"
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +
> +#define MIN_VL 16

<asm/sigcontext.h> has SVE_MIN_VL.  Maybe we can use that everywhere
these days?

> +
> +struct vec_data {

Could be const; persumably also static?

Maybe split the struct definition from the definition of vec_data[],
since it's a bit long.

> +	const char *name;
> +	unsigned long hwcap_type;
> +	unsigned long hwcap;
> +	const char *rdvl_binary;
> +	int (*rdvl)(void);
> +
> +	int prctl_get;
> +	int prctl_set;
> +	const char *default_vl_file;
> +
> +	int default_vl;
> +	int min_vl;
> +	int max_vl;

Ah, mind you, these are written by the test.  Never mind.  Seems
overkill to split this up just to constify.

> +} vec_data[] = {
> +	{
> +		.name = "SVE",
> +		.hwcap_type = AT_HWCAP,
> +		.hwcap = HWCAP_SVE,
> +		.rdvl = rdvl_sve,
> +		.rdvl_binary = "./rdvl-sve",
> +		.prctl_get = PR_SVE_GET_VL,
> +		.prctl_set = PR_SVE_SET_VL,
> +		.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
> +	},
> +};
> +
> +/* Start a new process and return the vector length it sees */
> +int get_child_rdvl(struct vec_data *data)
> +{
> +	char buf[10];

10?

> +	int pipefd[2];
> +	pid_t pid, child;
> +	int read_vl, ret;
> +
> +	ret = pipe(pipefd);
> +	if (ret == -1) {
> +		ksft_print_msg("pipe() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;
> +	}

Hmm, come to think of it:

--8<--
#include <stdio.h>

int main(void)
{
	puts("Listen very carefully, I shall say zees only once!");
	fork();
	return 0;
}
-->8--

$ ./fork

->
	Listen very carefully, I shall say zees only once!

$ ./fork | cat

->
	Listen very carefully, I shall say zees only once!
	Listen very carefully, I shall say zees only once!

Funny that doesn't come up more often.  I don't seem to recall this
being described anywhere.

Since we are about to change fd 1 under stdout's feet, it's probably at
least a good idea to fflush(stdout) before forking here.  I wonder
whether this should really be up to libc, but it's a tricky call since
flushing a stream could cause the process to block if there are clogged
pipes hanging off it.

> +
> +	child = fork();
> +	if (child == -1) {
> +		ksft_print_msg("fork() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		return -1;
> +	}
> +
> +	/* Child: put vector length on the pipe */
> +	if (child == 0) {
> +		/*
> +		 * Replace stdout with the pipe, errors to stderr from
> +		 * here as kselftest prints to stdout.
> +		 */
> +		ret = dup2(pipefd[1], 1);
> +		if (ret == -1) {
> +			fprintf(stderr, "dup2() %d\n", errno);
> +			exit(-1);

Should these exit(-1) be exit(1)?  The exit status always gets truncated
to 8 bits, but I don't like to deliberately tease the kernel.

Or use the EXIT_foo codes from <stdlib.h>.

> +		}
> +
> +		/* exec() a new binary which puts the VL on stdout */
> +		ret = execl(data->rdvl_binary, data->rdvl_binary, NULL);
> +		fprintf(stderr, "execl(%s) failed: %d\n",
> +			data->rdvl_binary, errno, strerror(errno));
> +
> +		exit(-1);
> +	}
> +
> +	close(pipefd[1]);
> +
> +	/* Parent; wait for the exit status from the child & verify it */
> +	while (1) {
> +		pid = wait(&ret);
> +		if (pid == -1) {
> +			ksft_print_msg("wait() failed: %d (%s)\n",
> +				       errno, strerror(errno));
> +			return -1;
> +		}
> +
> +		if (pid != child)
> +			continue;
> +
> +		if (!WIFEXITED(ret)) {
> +			ksft_print_msg("child exited abnormally\n");
> +			return -1;
> +		}
> +
> +		if (WEXITSTATUS(ret) != 0) {
> +			ksft_print_msg("child returned error %d\n",
> +				       WEXITSTATUS(ret));
> +			return -1;
> +		}
> +
> +		memset(buf, 0, sizeof(buf));
> +		ret = read(pipefd[0], buf, sizeof(buf) - 1);
> +		if (ret <= 0) {
> +			ksft_print_msg("read() failed: %d (%s)\n",
> +				       errno, strerror(errno));
> +			return -1;
> +		}
> +		close(pipefd[0]);
> +
> +		ret = sscanf(buf, "%d", &read_vl);

To be robust against truncation, we could do

	int n = 0;
	ret = sscanf(buf, "%d%*1[\n]%n", &read_vl, &n);
	if (ret < 1 || n < 1) { ...

Alternatively, can we just use fdopen() and fscanf()?  Then we don't
have to sweat details like buffer sizes at all.

This "read a single integer from an fd" thing is done a few times, maybe
it's worth wrapping up in a helper, or just use stdio everywhere
possible.

> +		if (ret != 1) {
> +			ksft_print_msg("failed to parse VL from '%s'\n",
> +				       buf);
> +			return -1;
> +		}
> +
> +		return read_vl;
> +	}
> +}
> +
> +int file_read_integer(const char *name, int *val)
> +{
> +	char buf[40];
> +	int f, ret;
> +
> +	f = open(name, O_RDONLY);
> +	if (f < 0) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	memset(buf, 0, sizeof(buf));
> +	ret = read(f, buf, sizeof(buf) - 1);
> +	if (ret < 0) {
> +		ksft_test_result_fail("Error reading %s: %d (%s)\n",
> +				      name, errno, strerror(errno));
> +		return -1;
> +	}
> +	close(f);
> +
> +	ret = sscanf(buf, "%d", val);
> +	if (ret != 1) {
> +		ksft_test_result_fail("Failed to parse %s\n", name);
> +		return -1;
> +	}

Can all this be done with fopen+fscanf+fclose?

> +
> +	return 0;
> +}
> +
> +int file_write_integer(const char *name, int val)
> +{
> +	char buf[40];
> +	int f, ret;
> +
> +	f = open(name, O_WRONLY);
> +	if (f < 0) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	snprintf(buf, sizeof(buf), "%d", val);
> +	ret = write(f, buf, strlen(buf));
> +	if (ret < 0) {
> +		ksft_test_result_fail("Error writing %d to %s: %d (%s)\n",
> +				      val, name, errno, strerror(errno));
> +		return -1;
> +	}
> +	close(f);

Similarly here.

> +
> +	return 0;
> +}
> +
> +/*
> + * Verify that we can read the default VL via proc, checking that it
> + * is set in a freshly spawned child.
> + */
> +void proc_read_default(struct vec_data *data)

Since we also check that the default takes effect, can this be renamed
proc_verify_default() or similar?

> +{
> +	int default_vl, child_vl, ret;
> +
> +	ret = file_read_integer(data->default_vl_file, &default_vl);
> +	if (ret != 0)
> +		return;
> +
> +	/* Is this the actual default seen by new processes? */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != default_vl) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      default_vl, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s default vector length %d\n", data->name,
> +			      default_vl);
> +	data->default_vl = default_vl;
> +}
> +
> +/* Verify that we can write a minimum value and have it take effect */
> +void proc_write_min(struct vec_data *data)

Could be proc_write_check_min() (though the "check" is a bit more
redundant here; from "write" it's clear that this function actually
does something nontrivial).

> +{
> +	int ret, new_default, child_vl;
> +
> +	ret = file_write_integer(data->default_vl_file, MIN_VL);
> +	if (ret != 0)
> +		return;
> +
> +	/* What was the new value? */
> +	ret = file_read_integer(data->default_vl_file, &new_default);
> +	if (ret != 0)
> +		return;
> +
> +	/* Did it take effect in a new process? */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != new_default) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      new_default, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s minimum vector length %d\n", data->name,
> +			      new_default);
> +	data->min_vl = new_default;
> +
> +	file_write_integer(data->default_vl_file, data->default_vl);
> +}
> +
> +/* Verify that we can write a maximum value and have it take effect */
> +void proc_write_max(struct vec_data *data)
> +{
> +	int ret, new_default, child_vl;
> +
> +	/* -1 is accepted by the /proc interface as the maximum VL */
> +	ret = file_write_integer(data->default_vl_file, -1);
> +	if (ret != 0)
> +		return;
> +
> +	/* What was the new value? */
> +	ret = file_read_integer(data->default_vl_file, &new_default);
> +	if (ret != 0)
> +		return;
> +
> +	/* Did it take effect in a new process? */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != new_default) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      new_default, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s maximum vector length %d\n", data->name,
> +			      new_default);
> +	data->max_vl = new_default;
> +
> +	file_write_integer(data->default_vl_file, data->default_vl);
> +}
> +
> +/* Can we read back a VL from prctl? */

It's certainly possible.

I used to have a tool that found out the current VL of a running PID
using ptrace, but I didn't want people thinking that connecting to
random running processes just to find this out was a good idea...

For these tests you could just exec /bin/true (or even argv[0], to
avoid fs dependencies -- doesn't matter what is exec'd) and just read
the VL at the ptrace post-exec stop before throwing the child a SIGKILL.

Since this would test different kernel paths from getting the child
itself to do RVDL / PR_SVE_GET_VL, it would be a different test though.
I think this diff is still good, but beefing up the ptrace tests to do
the appropriate checks would be good too (if we don't have that already).

> +void prctl_get(struct vec_data *data)
> +{
> +	int ret;
> +
> +	ret = prctl(data->prctl_get);
> +	if (ret == -1) {
> +		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
> +				      data->name, errno, strerror(errno));
> +		return;
> +	}
> +
> +	/* Mask out any flags */
> +	ret &= PR_SVE_VL_LEN_MASK;
> +
> +	/* Is that what we can read back directly? */
> +	if (ret == data->rdvl())
> +		ksft_test_result_pass("%s current VL is %d\n",
> +				      data->name, ret);
> +	else
> +		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
> +				      data->name, ret, data->rdvl());
> +}
> +
> +/* Does the prctl let us set the VL we already have? */
> +void prctl_set_same(struct vec_data *data)
> +{
> +	int cur_vl = data->rdvl();
> +	int ret;
> +
> +	ret = prctl(data->prctl_set, cur_vl);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed: %d (%s)\n",
> +				      data->name, errno, strerror(errno));
> +		return;
> +	}
> +
> +	if (cur_vl != data->rdvl())
> +		ksft_test_result_pass("%s current VL is %d\n",
> +				      data->name, ret);
> +	else
> +		ksft_test_result_fail("%s prctl() VL %d but RDVL is %d\n",
> +				      data->name, ret, data->rdvl());
> +}
> +
> +/* Can we set a new VL for this process? */
> +void prctl_set(struct vec_data *data)
> +{
> +	int ret;
> +
> +	if (data->min_vl == data->max_vl) {
> +		ksft_test_result_skip("%s only one VL supported\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	/* Try to set the minimum VL */
> +	ret = prctl(data->prctl_set, data->min_vl);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->min_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	if ((ret & PR_SVE_VL_LEN_MASK) != data->min_vl) {
> +		ksft_test_result_fail("%s prctl set %d but return value is %d\n",
> +				      data->name, data->min_vl, data->rdvl());
> +		return;
> +	}
> +
> +	if (data->rdvl() != data->min_vl) {
> +		ksft_test_result_fail("%s set %d but RDVL is %d\n",
> +				      data->name, data->min_vl, data->rdvl());
> +		return;
> +	}
> +
> +	/* Try to set the maximum VL */
> +	ret = prctl(data->prctl_set, data->max_vl);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->max_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	if ((ret & PR_SVE_VL_LEN_MASK) != data->max_vl) {
> +		ksft_test_result_fail("%s prctl() set %d but return value is %d\n",
> +				      data->name, data->max_vl, data->rdvl());
> +		return;
> +	}
> +
> +	/* The _INHERIT flag should not be present when we read the VL */
> +	ret = prctl(data->prctl_get);
> +	if (ret == -1) {
> +		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
> +				      data->name, errno, strerror(errno));
> +		return;
> +	}
> +
> +	if (ret & PR_SVE_VL_INHERIT) {
> +		ksft_test_result_fail("%s prctl() reports _INHERIT\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s prctl() set min/max\n", data->name);
> +}
> +
> +/* If we didn't request it a new VL shouldn't affect the child */
> +void prctl_set_no_child(struct vec_data *data)
> +{
> +	int ret, child_vl;
> +
> +	if (data->min_vl == data->max_vl) {
> +		ksft_test_result_skip("%s only one VL supported\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	ret = prctl(data->prctl_set, data->min_vl);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->min_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	/* Ensure the default VL is different */
> +	ret = file_write_integer(data->default_vl_file, data->max_vl);
> +	if (ret != 0)
> +		return;
> +
> +	/* Check that the child has the default we just set */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != data->max_vl) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      data->max_vl, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s vector length used default\n", data->name);
> +
> +	file_write_integer(data->default_vl_file, data->default_vl);
> +}
> +
> +/* If we didn't request it a new VL shouldn't affect the child */
> +void prctl_set_for_child(struct vec_data *data)
> +{
> +	int ret, child_vl;
> +
> +	if (data->min_vl == data->max_vl) {
> +		ksft_test_result_skip("%s only one VL supported\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_VL_INHERIT);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->min_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	/* The _INHERIT flag should be present when we read the VL */
> +	ret = prctl(data->prctl_get);
> +	if (ret == -1) {
> +		ksft_test_result_fail("%s prctl() read failed: %d (%s)\n",
> +				      data->name, errno, strerror(errno));
> +		return;
> +	}
> +	if (!(ret & PR_SVE_VL_INHERIT)) {
> +		ksft_test_result_fail("%s prctl() does not report _INHERIT\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	/* Ensure the default VL is different */
> +	ret = file_write_integer(data->default_vl_file, data->max_vl);
> +	if (ret != 0)
> +		return;
> +
> +	/* Check that the child inherited our VL */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != data->min_vl) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      data->min_vl, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s vector length was inherited\n", data->name);
> +
> +	file_write_integer(data->default_vl_file, data->default_vl);
> +}
> +
> +/* _ONEXEC takes effect only in the child process */
> +void prctl_set_onexec(struct vec_data *data)
> +{
> +	int ret, child_vl;
> +
> +	if (data->min_vl == data->max_vl) {
> +		ksft_test_result_skip("%s only one VL supported\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	/* Set a known value for the default and our current VL */
> +	ret = file_write_integer(data->default_vl_file, data->max_vl);
> +	if (ret != 0)
> +		return;
> +
> +	ret = prctl(data->prctl_set, data->max_vl);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->min_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	/* Set a different value for the child to have on exec */
> +	ret = prctl(data->prctl_set, data->min_vl | PR_SVE_SET_VL_ONEXEC);
> +	if (ret < 0) {
> +		ksft_test_result_fail("%s prctl set failed for %d: %d (%s)\n",
> +				      data->name, data->min_vl,
> +				      errno, strerror(errno));
> +		return;
> +	}
> +
> +	/* Our current VL should stay the same */
> +	if (data->rdvl() != data->max_vl) {
> +		ksft_test_result_fail("%s VL changed by _ONEXEC prctl()\n",
> +				      data->name);
> +		return;
> +	}
> +
> +	/* Check that the child inherited our VL */
> +	child_vl = get_child_rdvl(data);
> +	if (child_vl != data->min_vl) {
> +		ksft_test_result_fail("%s is %d but child VL is %d\n",
> +				      data->default_vl_file,
> +				      data->min_vl, child_vl);
> +		return;
> +	}
> +
> +	ksft_test_result_pass("%s vector length set on exec\n", data->name);
> +
> +	file_write_integer(data->default_vl_file, data->default_vl);
> +}
> +
> +typedef void (*test_type)(struct vec_data *);
> +
> +test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first to provide data for
> +	 * other tests.
> +	 */
> +	proc_read_default,
> +	proc_write_min,
> +	proc_write_max,

Can we also check what happens when writing unsupported values here?

e.g.

	vl where !sve_vl_valid();
	vl = 0;
	vl = max_supported_vl + 16;
	vl = SVE_VL_MAX;
	vl = INT_MAX / 16 * 16; (or possibly LONG_MAX.  What's the type in the
		kernel?)
	Any unsupported vl (on platforms where the set of supported vls is
		sparse, can be tested with qemu)

(As for what should happen, I can't remember for all cases -- check
sve.rst and the kernel code...)

If this patch is more about establishing the framework, these could be
TODOs for now.

> +
> +	prctl_get,
> +	prctl_set,
> +	prctl_set_no_child,
> +	prctl_set_for_child,
> +	prctl_set_onexec,
> +};
> +
> +int main(void)
> +{
> +	int i, j;
> +
> +	ksft_print_header();
> +	ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
> +
> +	for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
> +		struct vec_data *data = &vec_data[i];
> +		int supported = getauxval(data->hwcap_type) & data->hwcap;
> +
> +		for (j = 0; j < ARRAY_SIZE(tests); j++) {
> +			if (supported)
> +				tests[j](data);
> +			else
> +				ksft_test_result_skip("%s not supported\n",
> +						      data->name);
> +		}
> +	}

Can we be a good citizen and restore sve_default_vector_length to its
original value?

Also, we should probably disable the default vector length writing tests
if not running with EUID==0.  Verifying that writing the default vector
length fails when non-root would be worth testing.  If running as root,
a temporary seteuid(nobody_uid) could be used for that.

(Not sure whether there's a correct way to get nobody_uid other than
with setpwent()/while { getpwent() ... }/endpwent(), but that's not too
hard.  Or just hardcode 65534 -- I'd be prepared to bet other tests do
that.  That would work fine without a proper filesystem.)

Cheers
---Dave

Re: [PATCH v1 1/3] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Wed, Jul 28, 2021 at 10:41:30AM +0100, Dave Martin wrote:
> On Tue, Jul 27, 2021 at 07:06:47PM +0100, Mark Brown wrote:
> > SVE provides an instruction RDVL which reports the currently configured
> > vector length. In order to validate that our vector length configuration
> > interfaces are working correctly without having to build the C code for

> > +.globl rdvl_sve
> > +rdvl_sve:
> > +	rdvl	x0, #1
> > +	ret
> 
> This works, but can we use an ACLE intrinsic for this?  I'm pretty GCC
> and LLVM have been up to date with that stuff for some time, though
> you'd have to check with the compiler folks.

If we put SVE into C source files we need to build the C code with SVE
support enabled, I felt it was safer to avoid any possibility that we
might get some interference with the tests due to interactions with SVE
code generated by the compiler.  It should be fine, but it's even easier
to not have to think about it at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

[Dave Martin]

On Wed, Jul 28, 2021 at 11:20:35AM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 10:41:30AM +0100, Dave Martin wrote:
> > On Tue, Jul 27, 2021 at 07:06:47PM +0100, Mark Brown wrote:
> > > SVE provides an instruction RDVL which reports the currently configured
> > > vector length. In order to validate that our vector length configuration
> > > interfaces are working correctly without having to build the C code for
> 
> > > +.globl rdvl_sve
> > > +rdvl_sve:
> > > +	rdvl	x0, #1
> > > +	ret
> > 
> > This works, but can we use an ACLE intrinsic for this?  I'm pretty GCC
> > and LLVM have been up to date with that stuff for some time, though
> > you'd have to check with the compiler folks.
> 
> If we put SVE into C source files we need to build the C code with SVE
> support enabled, I felt it was safer to avoid any possibility that we
> might get some interference with the tests due to interactions with SVE
> code generated by the compiler.  It should be fine, but it's even easier
> to not have to think about it at all.

Good point, stick with the out-of-line asm in that case.

(Also saves having to find out about the intrinsics!)

Cheers
---Dave

Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Wed, Jul 28, 2021 at 10:41:51AM +0100, Dave Martin wrote:

> This test was originally a diagnostic tool, so the way VLs are probed
> aims to be efficient, rather than being defensive against the kernel
> doing weird stuff.

Yeah, the whole probing thing doesn't really fit into kselftest's idea
of what a test is - I just put this in here since it seemed like a cheap
extra validation that we could add in with little bother rather than
because it's a complete and thorough validation of every possible thing
that could go wrong.  I'd be just as happy to not modify this at all but
since it does try the intermediate VLs it didn't seem like a terrible
idea to throw in some validation while we're at it.

> If the kernel returns a vl > than the one we tried to set, we'll end
> up in an infinite loop because of the way the loop deliberately uses the
> kernel's return value to skip unsupported VLs.  So, it might be better
> to probe every single architecturally possible VL and sanity check
> everything instead.

Yup, that'd obviously be a complete rewrite though.  We'd not only need
to probe every possible vector length, but also validate that any
unsupported vector lengths report the expected vector length instead
when we try them.

> > +		if (rdvl_sve() != vl)
> > +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
> > +					   vl, rdvl_sve());

> If this fails, the VL vl wasn't "Set" at all.  I found this a bit
> confusing from just looking at this hunk.

> Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?

Sure.

> I'm not sure of the correct option for forcing SVE off against the
> compiler default -- check with the tools folks.  If there isn't a
> proper way to do this, it's a toolchain defect so we should flag it up,
> but -mgeneral-regs-only might work for us even though it's a bit of a
> sledgehammer.

-march should do the trick (if it doesn't the base kernel already has
issues).  This is a separate issue that affects all the arm64 selftests
I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

[Dave Martin]

On Wed, Jul 28, 2021 at 12:07:01PM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 10:41:51AM +0100, Dave Martin wrote:
> 
> > This test was originally a diagnostic tool, so the way VLs are probed
> > aims to be efficient, rather than being defensive against the kernel
> > doing weird stuff.
> 
> Yeah, the whole probing thing doesn't really fit into kselftest's idea
> of what a test is - I just put this in here since it seemed like a cheap
> extra validation that we could add in with little bother rather than
> because it's a complete and thorough validation of every possible thing
> that could go wrong.  I'd be just as happy to not modify this at all but
> since it does try the intermediate VLs it didn't seem like a terrible
> idea to throw in some validation while we're at it.
> 
> > If the kernel returns a vl > than the one we tried to set, we'll end
> > up in an infinite loop because of the way the loop deliberately uses the
> > kernel's return value to skip unsupported VLs.  So, it might be better
> > to probe every single architecturally possible VL and sanity check
> > everything instead.
> 
> Yup, that'd obviously be a complete rewrite though.  We'd not only need
> to probe every possible vector length, but also validate that any
> unsupported vector lengths report the expected vector length instead
> when we try them.

Fair enough, but is it worth dropping a comment in to clarify what this
does and doesn't test?  This could be an area for future improvement.

> > > +		if (rdvl_sve() != vl)
> > > +			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
> > > +					   vl, rdvl_sve());
> 
> > If this fails, the VL vl wasn't "Set" at all.  I found this a bit
> > confusing from just looking at this hunk.
> 
> > Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?
> 
> Sure.
> 
> > I'm not sure of the correct option for forcing SVE off against the
> > compiler default -- check with the tools folks.  If there isn't a
> > proper way to do this, it's a toolchain defect so we should flag it up,
> > but -mgeneral-regs-only might work for us even though it's a bit of a
> > sledgehammer.
> 
> -march should do the trick (if it doesn't the base kernel already has
> issues).  This is a separate issue that affects all the arm64 selftests
> I think.

OK, if there's already precedent then I guess we can go with that.

Cheers
---Dave

Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

On Wed, Jul 28, 2021 at 10:41:58AM +0100, Dave Martin wrote:
> On Tue, Jul 27, 2021 at 07:06:49PM +0100, Mark Brown wrote:

> > We provide interfaces for configuring the SVE vector length seen by
> > processes using prctl and also via /proc for configuring the default
> > values. Provide tests that exercise all these interfaces and verify that
> > they take effect as expected, though at present no test fully enumerates
> > all the possible vector lengths.

> Does "at present" mean that this test doesn't do it either?

> (It doesn't seem to try every VL, unless I've missed something?  Is this
> planned?)

Nothing currently does it, and nor does this patch.  Planned is a strong
term but yes, ideally we should probe all the VLs.

> > +#include <stddef.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> 
> Not used? ^

We call exit() which is declared in stdlib.h.

> > +#define MIN_VL 16

> <asm/sigcontext.h> has SVE_MIN_VL.  Maybe we can use that everywhere
> these days?

I partly wanted the vector type neutral name, and I'm considering
modifying the sysfs ABI file to define 0 as a valid vector length for
consistency with accepting -1 as the maximum since SME doesn't have any
architected guarantees as to which particular vector lengths are defined.

> > +/* Verify that we can write a minimum value and have it take effect */
> > +void proc_write_min(struct vec_data *data)

> Could be proc_write_check_min() (though the "check" is a bit more
> redundant here; from "write" it's clear that this function actually
> does something nontrivial).

TBH I'm not sure people will be excssively confused by the idea that a
test would validate the values it was trying to read or write were
accurate.

> > +/* Can we read back a VL from prctl? */
> 
> It's certainly possible.

The comment is describing what the test is verifying.

> Since this would test different kernel paths from getting the child
> itself to do RVDL / PR_SVE_GET_VL, it would be a different test though.
> I think this diff is still good, but beefing up the ptrace tests to do
> the appropriate checks would be good too (if we don't have that already).

Yes, the ptrace stuff could have a bit more coverage.

> > +	proc_write_min,
> > +	proc_write_max,

> Can we also check what happens when writing unsupported values here?

We could.

> If this patch is more about establishing the framework, these could be
> TODOs for now.

It definitely feels like something we can do incrementally.

> Can we be a good citizen and restore sve_default_vector_length to its
> original value?

We do that in the tests that fiddle with the default vector length, it
seems useful to keep it at a value different from min and max as much as
possible to increase the chance that we notice a failure to set things.

> Also, we should probably disable the default vector length writing tests
> if not running with EUID==0.  Verifying that writing the default vector

kselftest in general isn't going to have a great time run as non-root
but yes, it wouldn't hurt to check.

> length fails when non-root would be worth testing.  If running as root,
> a temporary seteuid(nobody_uid) could be used for that.

This feels like another thing that could be done incrementally.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Dave Martin]

On Wed, Jul 28, 2021 at 01:59:18PM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 10:41:58AM +0100, Dave Martin wrote:
> > On Tue, Jul 27, 2021 at 07:06:49PM +0100, Mark Brown wrote:
> 
> > > We provide interfaces for configuring the SVE vector length seen by
> > > processes using prctl and also via /proc for configuring the default
> > > values. Provide tests that exercise all these interfaces and verify that
> > > they take effect as expected, though at present no test fully enumerates
> > > all the possible vector lengths.
> 
> > Does "at present" mean that this test doesn't do it either?
> 
> > (It doesn't seem to try every VL, unless I've missed something?  Is this
> > planned?)
> 
> Nothing currently does it, and nor does this patch.  Planned is a strong
> term but yes, ideally we should probe all the VLs.
> 
> > > +#include <stddef.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > 
> > Not used? ^
> 
> We call exit() which is declared in stdlib.h.

Ignore me, I confused exit() with _exit().

> > > +#define MIN_VL 16
> 
> > <asm/sigcontext.h> has SVE_MIN_VL.  Maybe we can use that everywhere
> > these days?
> 
> I partly wanted the vector type neutral name, and I'm considering
> modifying the sysfs ABI file to define 0 as a valid vector length for
> consistency with accepting -1 as the maximum since SME doesn't have any
> architected guarantees as to which particular vector lengths are defined.

I see what you mean, but it is more than mere coincidence that this is
the same value as SVE_MIN_VL.

You could view SVE as defining the base architecture which SME then
extends.

Perhaps

#define ARCH_MIN_VL SVE_MIN_VL	/* architectural minimim VL */

would be neutral enough.  Anyway, I won't lose sleep over it.

> > > +/* Verify that we can write a minimum value and have it take effect */
> > > +void proc_write_min(struct vec_data *data)
> 
> > Could be proc_write_check_min() (though the "check" is a bit more
> > redundant here; from "write" it's clear that this function actually
> > does something nontrivial).
> 
> TBH I'm not sure people will be excssively confused by the idea that a
> test would validate the values it was trying to read or write were
> accurate.

It's not 100% clear that this is a test until one has read all the way
to the end of the file.  But now that I understand the pattern here I
wouldn't be too concerned, and the comment accurately describes what the
function does.

> 
> > > +/* Can we read back a VL from prctl? */
> > 
> > It's certainly possible.
> 
> The comment is describing what the test is verifying.

Ignore that, I somehow read the comment as something like

	[TODO] Can we read back a VL via ptrace?

which is not what the comment says.

> > Since this would test different kernel paths from getting the child
> > itself to do RVDL / PR_SVE_GET_VL, it would be a different test though.
> > I think this diff is still good, but beefing up the ptrace tests to do
> > the appropriate checks would be good too (if we don't have that already).
> 
> Yes, the ptrace stuff could have a bit more coverage.
> 
> > > +	proc_write_min,
> > > +	proc_write_max,
> 
> > Can we also check what happens when writing unsupported values here?
> 
> We could.
> 
> > If this patch is more about establishing the framework, these could be
> > TODOs for now.
> 
> It definitely feels like something we can do incrementally.

Is it worth committing a TODO list somewhere?  There's always the
possibility that someone else gets interested and contributes some tests
for us; otherwise, at least it makes it harder to forget them.

> > Can we be a good citizen and restore sve_default_vector_length to its
> > original value?
> 
> We do that in the tests that fiddle with the default vector length, it
> seems useful to keep it at a value different from min and max as much as
> possible to increase the chance that we notice a failure to set things.

Ah right, I hadn't understood that proc_read_default() reads the default
and then the subsequent tests write it back.

This might be a bit clearer if the setup code was clearly separate from
the tests, but so long as the ordering requirements are clearly
documented that seems reasonably OK.

In:

> +test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first to provide data for
> +	 * other tests.
> +	 */
> +	proc_read_default,
> +	proc_write_min,
> +	proc_write_max,

can you also comment that proc_read_default needs to come first among
these?

> +
> +	prctl_get,
> +	prctl_set,
> +	prctl_set_no_child,
> +	prctl_set_for_child,
> +	prctl_set_onexec,
> +};

[...]

Cheers
---Dave

Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Wed, Jul 28, 2021 at 02:44:07PM +0100, Dave Martin wrote:

> This might be a bit clearer if the setup code was clearly separate from
> the tests, but so long as the ordering requirements are clearly
> documented that seems reasonably OK.

We can't really split the setup code out since the setup code would have
to rely on one of the ABIs we're trying to test which is all fine and
good until the tests actually help us catch something, we have to build
up knowledge of the values as we go and hope that the cross checking we
end up with helps us at least catch inconsistencies.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Dave Martin]

On Wed, Jul 28, 2021 at 05:29:53PM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 02:44:07PM +0100, Dave Martin wrote:
> 
> > This might be a bit clearer if the setup code was clearly separate from
> > the tests, but so long as the ordering requirements are clearly
> > documented that seems reasonably OK.
> 
> We can't really split the setup code out since the setup code would have
> to rely on one of the ABIs we're trying to test which is all fine and
> good until the tests actually help us catch something, we have to build
> up knowledge of the values as we go and hope that the cross checking we
> end up with helps us at least catch inconsistencies.

Ack.  The setup code could be defensive, but you would in effect be
doing some of the tests twice then.  Or the fact that the setup code
is "separate" from the tests might be a bit of an illusion.

Cheers
---Dave