[PATCH v3 0/4] kselftest/arm64: Vector length configuration tests

[PATCH v3 0/4] 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

v3:
 - Add BTI landing pads to the asm helper functions.
 - Clean up pipes used to talk to children.
 - Remove another unneeded include.
 - Make functions in the main executable static.
 - Match the newline when parsing vector length from the child.
 - Factor out the fscanf() and fclose() from parsing integers from file
   descriptors.
 - getauxval() returns unsigned long.
v2:
 - Tweak log message on failure in sve-probe-vls.
 - Stylistic changes in vec-syscfg.
 - Flush stdout before forking in vec-syscfg.
 - Use EXIT_FAILURE.
 - Use fdopen() to get child output.
 - Replace a bunch of UNIX API usage with stdio.
 - Add a TODO list.
 - Verify that we're root before testing writes to /proc.

Mark Brown (4):
  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
  kselftest/arm64: Add a TODO list for floating point tests

 tools/testing/selftests/arm64/fp/.gitignore   |   2 +
 tools/testing/selftests/arm64/fp/Makefile     |  11 +-
 tools/testing/selftests/arm64/fp/TODO         |   3 +
 tools/testing/selftests/arm64/fp/rdvl-sve.c   |  14 +
 tools/testing/selftests/arm64/fp/rdvl.S       |  10 +
 tools/testing/selftests/arm64/fp/rdvl.h       |   8 +
 .../selftests/arm64/fp/sve-probe-vls.c        |   5 +
 tools/testing/selftests/arm64/fp/vec-syscfg.c | 594 ++++++++++++++++++
 8 files changed, 644 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/4] 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>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
---
 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     | 10 ++++++++++
 tools/testing/selftests/arm64/fp/rdvl.h     |  8 ++++++++
 5 files changed, 38 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..c916c1c9defd
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/rdvl.S
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2021 ARM Limited.
+
+.arch_extension sve
+
+.globl rdvl_sve
+rdvl_sve:
+	hint	34			// BTI C
+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/4] 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>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
---
 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..a24eca7a4ecb 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("PR_SVE_SET_VL reports %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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] 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 | 594 ++++++++++++++++++
 3 files changed, 597 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..e8ba679aec29
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -0,0 +1,594 @@
+// 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 <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <asm/sigcontext.h>
+#include <asm/hwcap.h>
+
+#include "../../kselftest.h"
+#include "rdvl.h"
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+
+#define ARCH_MIN_VL SVE_VL_MIN
+
+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;
+};
+
+
+static struct vec_data 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",
+	},
+};
+
+static int stdio_read_integer(FILE *f, const char *what, int *val)
+{
+	int ret;
+
+	ret = fscanf(f, "%d*1[\n]*n", val);
+	fclose(f);
+	if (ret != 1) {
+		ksft_print_msg("failed to parse VL from %s\n", what);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Start a new process and return the vector length it sees */
+static int get_child_rdvl(struct vec_data *data)
+{
+	FILE *out;
+	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;
+	}
+
+	fflush(stdout);
+
+	child = fork();
+	if (child == -1) {
+		ksft_print_msg("fork() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		close(pipefd[0]);
+		close(pipefd[1]);
+		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(EXIT_FAILURE);
+		}
+
+		/* 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(EXIT_FAILURE);
+	}
+
+	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));
+			close(pipefd[0]);
+			return -1;
+		}
+
+		if (pid != child)
+			continue;
+
+		if (!WIFEXITED(ret)) {
+			ksft_print_msg("child exited abnormally\n");
+			close(pipefd[0]);
+			return -1;
+		}
+
+		if (WEXITSTATUS(ret) != 0) {
+			ksft_print_msg("child returned error %d\n",
+				       WEXITSTATUS(ret));
+			close(pipefd[0]);
+			return -1;
+		}
+
+		break;
+	}
+
+	out = fdopen(pipefd[0], "r");
+	if (!out) {
+		ksft_print_msg("failed to open child stdout\n");
+		close(pipefd[0]);
+		return -1;
+	}
+
+	ret = stdio_read_integer(out, "child", &read_vl);
+	if (ret != 0)
+		return ret;
+
+	return read_vl;
+}
+
+static int file_read_integer(const char *name, int *val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "r");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	return stdio_read_integer(f, name, val);
+
+	return 0;
+}
+
+static int file_write_integer(const char *name, int val)
+{
+	FILE *f;
+	int ret;
+
+	f = fopen(name, "w");
+	if (!f) {
+		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
+				      name, errno,
+				      strerror(errno));
+		return -1;
+	}
+
+	fprintf(f, "%d", val);
+	fclose(f);
+	if (ret < 0) {
+		ksft_test_result_fail("Error writing %d to %s\n",
+				      val, name);
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Verify that we can read the default VL via proc, checking that it
+ * is set in a freshly spawned child.
+ */
+static 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 */
+static void proc_write_min(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	ret = file_write_integer(data->default_vl_file, ARCH_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 */
+static void proc_write_max(struct vec_data *data)
+{
+	int ret, new_default, child_vl;
+
+	if (geteuid() != 0) {
+		ksft_test_result_skip("Need to be root to write to /proc\n");
+		return;
+	}
+
+	/* -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? */
+static 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? */
+static 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? */
+static 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 */
+static 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 */
+static 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 */
+static 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 *);
+
+static const test_type tests[] = {
+	/*
+	 * The default/min/max tests must be first and in this order
+	 * 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];
+		unsigned long supported;
+
+		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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/4] kselftest/arm64: Add a TODO list for floating point tests

[Mark Brown]

Write down some ideas for additional coverage for floating point in case
someone feels inspired to look into them.

Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
---
 tools/testing/selftests/arm64/fp/TODO | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/fp/TODO

diff --git a/tools/testing/selftests/arm64/fp/TODO b/tools/testing/selftests/arm64/fp/TODO
new file mode 100644
index 000000000000..eada915227cf
--- /dev/null
+++ b/tools/testing/selftests/arm64/fp/TODO
@@ -0,0 +1,3 @@
+- Test unsupported values in the ABIs
+- More coverage for ptrace (eg, vector length conversions).
+- Coverage for signals.
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Thu, Jul 29, 2021 at 04:15:17PM +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.
> 
> 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.

Looks good except for the fscanf thing below.

The rest is really at the pedantic nit level, so I won't mind too much
if those are quietly dropped.

> 
> 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 | 594 ++++++++++++++++++
>  3 files changed, 597 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..e8ba679aec29
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
> @@ -0,0 +1,594 @@
> +// 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 <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/auxv.h>
> +#include <sys/prctl.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <asm/sigcontext.h>
> +#include <asm/hwcap.h>
> +
> +#include "../../kselftest.h"
> +#include "rdvl.h"
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +
> +#define ARCH_MIN_VL SVE_VL_MIN
> +
> +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;
> +};
> +
> +
> +static struct vec_data 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",
> +	},
> +};
> +
> +static int stdio_read_integer(FILE *f, const char *what, int *val)
> +{
> +	int ret;
> +
> +	ret = fscanf(f, "%d*1[\n]*n", val);

Argh, I mangled this in my last reply, though it was right in the
previous reply.  I think this should be

	int ret, n = 0;

	ret = fscanf(f, "%d%*1[\n]%n", val, &n);
	fclose(f);
	if (ret < 1 || n < 1) {
		/* ... */

The %n ... &n is needed because nobody knows whether the scanf return
value includes suppressed assignments, so we might get 1 whether or not
the %*1[\n] is matched...
	
> +	fclose(f);

It feels a bit unbalanced to have the fclose() in here, since the
fopen() is outside.  Can it be pushed back out into file_read_integer()
and get_child_rdvl()?

Or call this function stdio_read_integer_and_close(), I suppose.

> +	if (ret != 1) {
> +		ksft_print_msg("failed to parse VL from %s\n", what);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Start a new process and return the vector length it sees */
> +static int get_child_rdvl(struct vec_data *data)
> +{
> +	FILE *out;
> +	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;
> +	}
> +
> +	fflush(stdout);
> +
> +	child = fork();
> +	if (child == -1) {
> +		ksft_print_msg("fork() failed: %d (%s)\n",
> +			       errno, strerror(errno));
> +		close(pipefd[0]);
> +		close(pipefd[1]);
> +		return -1;

Since nothing reopens pipefd[0] or pipefd[1], you could also follow the
"goto out" convention and just (re)close both fds at the end, rather
than having to repeat the close() multiple times.  But it works as-is.

> +	}
> +
> +	/* 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(EXIT_FAILURE);
> +		}
> +
> +		/* 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(EXIT_FAILURE);
> +	}
> +
> +	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));
> +			close(pipefd[0]);
> +			return -1;
> +		}
> +
> +		if (pid != child)
> +			continue;
> +
> +		if (!WIFEXITED(ret)) {
> +			ksft_print_msg("child exited abnormally\n");
> +			close(pipefd[0]);
> +			return -1;
> +		}

The WEXITSTATUS() check could go outside the loop.

> +
> +		if (WEXITSTATUS(ret) != 0) {
> +			ksft_print_msg("child returned error %d\n",
> +				       WEXITSTATUS(ret));
> +			close(pipefd[0]);
> +			return -1;
> +		}
> +
> +		break;
> +	}

The break looks funny to me, since there is only one possible child
process anyway, perhaps this could be rewritten as

	do {
		pid = wait(&ret);
		if (pid == -1) {
			/* ... */
			return -1;
		}
	} while (pid != child);

	assert(pid == child);

	if (!WIFEXITED(ret))
		/* barf */

	if (WEXITSTATUS(ret) != 0)
		/* barf */

	out = fdopen( /* ... */
> +
> +	out = fdopen(pipefd[0], "r");
> +	if (!out) {
> +		ksft_print_msg("failed to open child stdout\n");
> +		close(pipefd[0]);
> +		return -1;
> +	}
> +
> +	ret = stdio_read_integer(out, "child", &read_vl);
> +	if (ret != 0)
> +		return ret;
> +
> +	return read_vl;
> +}
> +
> +static int file_read_integer(const char *name, int *val)
> +{
> +	FILE *f;
> +	int ret;
> +
> +	f = fopen(name, "r");
> +	if (!f) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	return stdio_read_integer(f, name, val);
> +
> +	return 0;
> +}
> +
> +static int file_write_integer(const char *name, int val)
> +{
> +	FILE *f;
> +	int ret;
> +
> +	f = fopen(name, "w");
> +	if (!f) {
> +		ksft_test_result_fail("Unable to open %s: %d (%s)\n",
> +				      name, errno,
> +				      strerror(errno));
> +		return -1;
> +	}
> +
> +	fprintf(f, "%d", val);
> +	fclose(f);
> +	if (ret < 0) {
> +		ksft_test_result_fail("Error writing %d to %s\n",
> +				      val, name);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Verify that we can read the default VL via proc, checking that it
> + * is set in a freshly spawned child.
> + */
> +static 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 */
> +static void proc_write_min(struct vec_data *data)
> +{
> +	int ret, new_default, child_vl;
> +
> +	if (geteuid() != 0) {
> +		ksft_test_result_skip("Need to be root to write to /proc\n");
> +		return;
> +	}
> +
> +	ret = file_write_integer(data->default_vl_file, ARCH_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 */
> +static void proc_write_max(struct vec_data *data)
> +{
> +	int ret, new_default, child_vl;
> +
> +	if (geteuid() != 0) {
> +		ksft_test_result_skip("Need to be root to write to /proc\n");
> +		return;
> +	}
> +
> +	/* -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? */
> +static 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? */
> +static 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? */
> +static 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 */
> +static 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 */
> +static 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 */
> +static 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 *);
> +
> +static const test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first and in this order
> +	 * 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];
> +		unsigned long supported;
> +
> +		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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1319 bytes --]

On Thu, Jul 29, 2021 at 05:06:42PM +0100, Dave Martin wrote:
> On Thu, Jul 29, 2021 at 04:15:17PM +0100, Mark Brown wrote:

> > +	child = fork();
> > +	if (child == -1) {
> > +		ksft_print_msg("fork() failed: %d (%s)\n",
> > +			       errno, strerror(errno));
> > +		close(pipefd[0]);
> > +		close(pipefd[1]);
> > +		return -1;

> Since nothing reopens pipefd[0] or pipefd[1], you could also follow the
> "goto out" convention and just (re)close both fds at the end, rather
> than having to repeat the close() multiple times.  But it works as-is.

I find that when fork() gets involved that starts to get confusing since
you have multiple contexts/control flows around and working out which
cleanup path goes where is more of the issue.

> > +		if (!WIFEXITED(ret)) {
> > +			ksft_print_msg("child exited abnormally\n");
> > +			close(pipefd[0]);
> > +			return -1;
> > +		}

> The WEXITSTATUS() check could go outside the loop.

OTOH I find that logically it's part of working out what happened with
the child which is what the loop body is doing.  Anyway I changed to the
do/while you suggested.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Thu, Jul 29, 2021 at 06:34:11PM +0100, Mark Brown wrote:
> On Thu, Jul 29, 2021 at 05:06:42PM +0100, Dave Martin wrote:
> > On Thu, Jul 29, 2021 at 04:15:17PM +0100, Mark Brown wrote:
> 
> > > +	child = fork();
> > > +	if (child == -1) {
> > > +		ksft_print_msg("fork() failed: %d (%s)\n",
> > > +			       errno, strerror(errno));
> > > +		close(pipefd[0]);
> > > +		close(pipefd[1]);
> > > +		return -1;
> 
> > Since nothing reopens pipefd[0] or pipefd[1], you could also follow the
> > "goto out" convention and just (re)close both fds at the end, rather
> > than having to repeat the close() multiple times.  But it works as-is.
> 
> I find that when fork() gets involved that starts to get confusing since
> you have multiple contexts/control flows around and working out which
> cleanup path goes where is more of the issue.

Ack, the other option would be to factor out the child stuff into a
separate function, but this doesn't quite seem worth it here.

Although the code seemed a bit repetitive, it is at least clear in its
current form, so I don't have a strong view.

> 
> > > +		if (!WIFEXITED(ret)) {
> > > +			ksft_print_msg("child exited abnormally\n");
> > > +			close(pipefd[0]);
> > > +			return -1;
> > > +		}
> 
> > The WEXITSTATUS() check could go outside the loop.
> 
> OTOH I find that logically it's part of working out what happened with
> the child which is what the loop body is doing.  Anyway I changed to the
> do/while you suggested.

That's a reasonable position, but thinking about it a bit more, there's
not really any loop at all here.

There definitely is an unwaited-for child and we don't pass WHONANG to
wait(), so it will either return the child pid, or fail.

Without WUNTRACED or similar, the child must terminate to wake up the
wait().

So is this just a matter of

	pid = wait(&ret);
	if (pid == -1) {
		/* barf */
	}
	assert(pid == child);

	if (!WIFEXITED(ret)) {
		/* barf */
	}

	if (WEXITSTATUS(ret) != 0) {
		/* barf */
	}

	/* parse child's stdout etc. */

> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Hmmm, usually I at least try to do that, but I did seem to leave rather
a lot of trailing junk that time.

(Working out which context is relevant is not always an exact science,
but in this case, it looks like I just forgot.)

Cheers
---Dave 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1488 bytes --]

On Mon, Aug 02, 2021 at 11:25:29AM +0100, Dave Martin wrote:

> That's a reasonable position, but thinking about it a bit more, there's
> not really any loop at all here.
> 
> There definitely is an unwaited-for child and we don't pass WHONANG to
> wait(), so it will either return the child pid, or fail.
> 
> Without WUNTRACED or similar, the child must terminate to wake up the
> wait().

> So is this just a matter of

> 	pid = wait(&ret);
> 	if (pid == -1) {
> 		/* barf */
> 	}
> 	assert(pid == child);
> 
> 	if (!WIFEXITED(ret)) {
> 		/* barf */
> 	}
> 
> 	if (WEXITSTATUS(ret) != 0) {
> 		/* barf */
> 	}
> 
> 	/* parse child's stdout etc. */

That really doesn't seem like a good idea - it's just asking for
fragility if a signal gets delivered to the parent process or something.
Even if almost all the time there will only be one trip through the loop
we should still have the loop there for those few cases where it
triggers.

> 
> > Please delete unneeded context from mails when replying.  Doing this
> > makes it much easier to find your reply in the message, helping ensure
> > it won't be missed by people scrolling through the irrelevant quoted
> > material.
> 
> Hmmm, usually I at least try to do that, but I did seem to leave rather
> a lot of trailing junk that time.
> 
> (Working out which context is relevant is not always an exact science,
> but in this case, it looks like I just forgot.)
> 
> Cheers
> ---Dave 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Mon, Aug 02, 2021 at 12:33:30PM +0100, Mark Brown wrote:
> On Mon, Aug 02, 2021 at 11:25:29AM +0100, Dave Martin wrote:
> 
> > That's a reasonable position, but thinking about it a bit more, there's
> > not really any loop at all here.
> > 
> > There definitely is an unwaited-for child and we don't pass WHONANG to
> > wait(), so it will either return the child pid, or fail.
> > 
> > Without WUNTRACED or similar, the child must terminate to wake up the
> > wait().
> 
> > So is this just a matter of
> 
> > 	pid = wait(&ret);
> > 	if (pid == -1) {
> > 		/* barf */
> > 	}
> > 	assert(pid == child);
> > 
> > 	if (!WIFEXITED(ret)) {
> > 		/* barf */
> > 	}
> > 
> > 	if (WEXITSTATUS(ret) != 0) {
> > 		/* barf */
> > 	}
> > 
> > 	/* parse child's stdout etc. */
> 
> That really doesn't seem like a good idea - it's just asking for
> fragility if a signal gets delivered to the parent process or something.
> Even if almost all the time there will only be one trip through the loop
> we should still have the loop there for those few cases where it
> triggers.

This concern only applies when the program actually registers signal
handlers.

wait() can't return for any other reason, and it mustn't, precisely
because historically software would have made this assumption.  This is
one reason why wait3() etc. are separate functions.

That aside though, can't we use popen(3)?

I tend to forget about popen because it is "boring" to use it, but it
looks like it fits this case quite well.  Then it would be libc's
problem how to fork and wait safely.

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1797 bytes --]

On Mon, Aug 02, 2021 at 01:37:50PM +0100, Dave Martin wrote:
> On Mon, Aug 02, 2021 at 12:33:30PM +0100, Mark Brown wrote:

> > That really doesn't seem like a good idea - it's just asking for
> > fragility if a signal gets delivered to the parent process or something.
> > Even if almost all the time there will only be one trip through the loop
> > we should still have the loop there for those few cases where it
> > triggers.

> This concern only applies when the program actually registers signal
> handlers.

> wait() can't return for any other reason, and it mustn't, precisely
> because historically software would have made this assumption.  This is
> one reason why wait3() etc. are separate functions.

That's great for the reader with a detailed knowledge of exactly what
error handling can be skipped and how standards conforming Linux is but
less good for the reader who is merely aware of best practices.  I am
not clear what the problem that is solved by removing the loop here is
TBH - to me it just makes it less obvious that we've handled everything.

> That aside though, can't we use popen(3)?

> I tend to forget about popen because it is "boring" to use it, but it
> looks like it fits this case quite well.  Then it would be libc's
> problem how to fork and wait safely.

popen() appears to be break the _SET_VL_ONEXEC test.  Between a lack of
strace in my test filesystem and not spotting anything obvious in the
glibc sources I can't tell exactly where it's doing something different,
though it does feel like it should be a separate testcase if it's
anything interesting.  I do think there is value in having exactly
what's done to start the child process be clear in the test program, and
that coverage of anything interesting from popen() could be done
incrementally.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Mon, Aug 02, 2021 at 03:19:39PM +0100, Mark Brown wrote:
> On Mon, Aug 02, 2021 at 01:37:50PM +0100, Dave Martin wrote:
> > On Mon, Aug 02, 2021 at 12:33:30PM +0100, Mark Brown wrote:
> 
> > > That really doesn't seem like a good idea - it's just asking for
> > > fragility if a signal gets delivered to the parent process or something.
> > > Even if almost all the time there will only be one trip through the loop
> > > we should still have the loop there for those few cases where it
> > > triggers.
> 
> > This concern only applies when the program actually registers signal
> > handlers.
> 
> > wait() can't return for any other reason, and it mustn't, precisely
> > because historically software would have made this assumption.  This is
> > one reason why wait3() etc. are separate functions.
> 
> That's great for the reader with a detailed knowledge of exactly what
> error handling can be skipped and how standards conforming Linux is but
> less good for the reader who is merely aware of best practices.  I am
> not clear what the problem that is solved by removing the loop here is
> TBH - to me it just makes it less obvious that we've handled everything.

Ok, leave it as is then.

(It would be good to collect some best-practice guidance on how to
actually use syscalls, but that's clearly way out of scope here...)

> > That aside though, can't we use popen(3)?
> 
> > I tend to forget about popen because it is "boring" to use it, but it
> > looks like it fits this case quite well.  Then it would be libc's
> > problem how to fork and wait safely.
> 
> popen() appears to be break the _SET_VL_ONEXEC test.  Between a lack of
> strace in my test filesystem and not spotting anything obvious in the
> glibc sources I can't tell exactly where it's doing something different,
> though it does feel like it should be a separate testcase if it's
> anything interesting.  I do think there is value in having exactly
> what's done to start the child process be clear in the test program, and
> that coverage of anything interesting from popen() could be done
> incrementally.

Ah, dang, popen() will run the target program via a shell, so there will
actually be two fork-exec()s, with the VL being reset to default by the
second exec.

Using PR_SET_SET_VL with popen() still makes sense, but if you want the
target program to get the new VL (not just the shell) then you'd need
PR_SVE_VL_INHERIT.  Then we would get confused later when trying to
test the !PR_SVE_VL_INHERIT case.  The way to "fix" this would be to
have the shell invoke something like vlset, but that will blur the test
in a different way, adding even more confusion.

So Ack, we can't test all the variations using the popen() method, so we
probably shouldn't use it here at all.  

This is the kind of reason why I tend not to go for it, I guess --
it looks convenient, but it's just that little bit overcooked as an API.
*sigh*


I'll review your final version of the series, but I guess we're all good.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]

On Mon, Aug 02, 2021 at 04:36:15PM +0100, Dave Martin wrote:
> On Mon, Aug 02, 2021 at 03:19:39PM +0100, Mark Brown wrote:

> > popen() appears to be break the _SET_VL_ONEXEC test.  Between a lack of
> > strace in my test filesystem and not spotting anything obvious in the
> > glibc sources I can't tell exactly where it's doing something different,
> > though it does feel like it should be a separate testcase if it's

> Ah, dang, popen() will run the target program via a shell, so there will
> actually be two fork-exec()s, with the VL being reset to default by the
> second exec.

Oh, of course - it's basically a more useful system().  The bit where it
adds the sh invocation was buried somewhere in the glibc source I didn't
find.

> This is the kind of reason why I tend not to go for it, I guess --
> it looks convenient, but it's just that little bit overcooked as an API.
> *sigh*

Well, if you're working at the stdio level it's fine I think but this
program is at the level below that.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel