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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + */
> +#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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
