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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + */
> +#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
