On Mon, 19 Aug 2024 13:57:21 +0100
Andre Przywara <andre.przywara@xxxxxxx> wrote:
Hi,
sorry, just realised I missed one include...
> Currently a number of SVE/SME related tests have almost identical
> functions to enumerate all supported vector lengths. However over time
> the copy&pasted code has diverged, allowing some bugs to creep in:
> - fake_sigreturn_sme_change_vl reports a failure, not a SKIP if only
> one vector length is supported (but the SVE version is fine)
> - fake_sigreturn_sme_change_vl tries to set the SVE vector length, not
> the SME one (but the other SME tests are fine)
> - za_no_regs keeps iterating forever if only one vector length is
> supported (but za_regs is correct)
>
> Since those bugs seem to be mostly copy&paste ones, let's consolidate
> the enumeration loop into one shared function, and just call that from
> each test. That should fix the above bugs, and prevent similar issues
> from happening again.
>
> Fixes: 4963aeb35a9e ("kselftest/arm64: signal: Add SME signal handling tests")
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> Hi,
>
> a small update, making the SME VL loop check more generic, and adding a
> comment about the reason we have that in the first place.
>
> Cheers,
> Andre
>
> tools/testing/selftests/arm64/signal/Makefile | 2 +-
> .../selftests/arm64/signal/sve_helpers.c | 56 +++++++++++++++++++
> .../selftests/arm64/signal/sve_helpers.h | 21 +++++++
> .../testcases/fake_sigreturn_sme_change_vl.c | 31 +++-------
> .../testcases/fake_sigreturn_sve_change_vl.c | 30 ++--------
> .../arm64/signal/testcases/ssve_regs.c | 36 +++---------
> .../arm64/signal/testcases/ssve_za_regs.c | 36 +++---------
> .../arm64/signal/testcases/sve_regs.c | 32 +++--------
> .../arm64/signal/testcases/za_no_regs.c | 32 +++--------
> .../arm64/signal/testcases/za_regs.c | 36 +++---------
> 10 files changed, 131 insertions(+), 181 deletions(-)
> create mode 100644 tools/testing/selftests/arm64/signal/sve_helpers.c
> create mode 100644 tools/testing/selftests/arm64/signal/sve_helpers.h
>
> diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
> index 37c8207b99cfc..1381039fb36f9 100644
> --- a/tools/testing/selftests/arm64/signal/Makefile
> +++ b/tools/testing/selftests/arm64/signal/Makefile
> @@ -23,7 +23,7 @@ $(TEST_GEN_PROGS): $(PROGS)
> # Common test-unit targets to build common-layout test-cases executables
> # Needs secondary expansion to properly include the testcase c-file in pre-reqs
> COMMON_SOURCES := test_signals.c test_signals_utils.c testcases/testcases.c \
> - signals.S
> + signals.S sve_helpers.c
> COMMON_HEADERS := test_signals.h test_signals_utils.h testcases/testcases.h
>
> .SECONDEXPANSION:
> diff --git a/tools/testing/selftests/arm64/signal/sve_helpers.c b/tools/testing/selftests/arm64/signal/sve_helpers.c
> new file mode 100644
> index 0000000000000..f57265354eaed
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/sve_helpers.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 ARM Limited
> + *
> + * Common helper functions for SVE and SME functionality.
> + */
> +
> +#include <stdbool.h>
> +#include <kselftest.h>
> +#include <asm/sigcontext.h>
> +#include <sys/prctl.h>
> +
> +unsigned int vls[SVE_VQ_MAX];
> +unsigned int nvls;
> +
> +int sve_fill_vls(bool use_sme, int min_vls)
> +{
> + int vq, vl;
> + int pr_set_vl = use_sme ? PR_SME_SET_VL : PR_SVE_SET_VL;
> + int len_mask = use_sme ? PR_SME_VL_LEN_MASK : PR_SVE_VL_LEN_MASK;
> +
> + /*
> + * Enumerate up to SVE_VQ_MAX vector lengths
> + */
> + for (vq = SVE_VQ_MAX; vq > 0; --vq) {
> + vl = prctl(pr_set_vl, vq * 16);
> + if (vl == -1)
> + return KSFT_FAIL;
> +
> + vl &= len_mask;
> +
> + /*
> + * Unlike SVE, SME does not require the minimum vector length
> + * to be implemented, or the VLs to be consecutive, so any call
> + * to the prctl might return the single implemented VL, which
> + * might be larger than 16. So to avoid this loop never
> + * terminating, bail out here when we find a higher VL than
> + * we asked for.
> + * See the ARM ARM, DDI 0487K.a, B1.4.2: I_QQRNR and I_NWYBP.
> + */
> + if (vq < sve_vq_from_vl(vl))
> + break;
> +
> + /* Skip missing VLs */
> + vq = sve_vq_from_vl(vl);
> +
> + vls[nvls++] = vl;
> + }
> +
> + if (nvls < min_vls) {
> + fprintf(stderr, "Only %d VL supported\n", nvls);
> + return KSFT_SKIP;
> + }
> +
> + return KSFT_PASS;
> +}
> diff --git a/tools/testing/selftests/arm64/signal/sve_helpers.h b/tools/testing/selftests/arm64/signal/sve_helpers.h
> new file mode 100644
> index 0000000000000..50948ce471cc6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/sve_helpers.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 ARM Limited
> + *
> + * Common helper functions for SVE and SME functionality.
> + */
> +
> +#ifndef __SVE_HELPERS_H__
> +#define __SVE_HELPERS_H__
> +
> +#include <stdbool.h>
> +
> +#define VLS_USE_SVE false
> +#define VLS_USE_SME true
> +
> +extern unsigned int vls[];
> +extern unsigned int nvls;
> +
> +int sve_fill_vls(bool use_sme, int min_vls);
> +
> +#endif
> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_sme_change_vl.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_sme_change_vl.c
> index ebd5815b54bba..bc10585062d57 100644
> --- a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_sme_change_vl.c
> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_sme_change_vl.c
> @@ -11,39 +11,22 @@
> #include <sys/prctl.h>
We need an "#include <kselftest.h>" here.
Sorry, but that slipped through because the kselftest build throws quite
some warnings and errors for me (some of fixed with the other series).
Will send a v3 ASAP.
Cheers,
Andre.
>
> #include "test_signals_utils.h"
> +#include "sve_helpers.h"
> #include "testcases.h"
>
[ ... ]
