This series fixes an issue which David Spickett found where if we change
the SVE VL while SME is in use we can end up attempting to save state to
an unallocated buffer and adds testing coverage for that plus a bit more
coverage of VL changes, just for paranioa.
Signed-off-by: Mark Brown <[email protected]>
---
Mark Brown (3):
arm64/fpsimd: Ensure SME storage is allocated after SVE VL changes
kselftest/arm64: Add a test case for SVE VL changes with SME active
kselftest/arm64: Validate that changing one VL type does not affect another
arch/arm64/kernel/fpsimd.c | 32 +++++--
tools/testing/selftests/arm64/fp/vec-syscfg.c | 127 +++++++++++++++++++++++++-
2 files changed, 148 insertions(+), 11 deletions(-)
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230713-arm64-fix-sve-sme-vl-change-60eb1fa6a707
Best regards,
--
Mark Brown <[email protected]>
When we reconfigure the SVE vector length we discard the backing storage
for the SVE vectors and then reallocate on next SVE use, leaving the SME
specific state alone. This means that we do not enable SME traps if they
were already disabled. That means that userspace code can enter streaming
mode without trapping, putting the task in a state where if we try to save
the state of the task we will fault.
Since the ABI does not specify that changing the SVE vector length disturbs
SME state, and since SVE code may not be aware of SME code in the process,
we shouldn't simply discard any ZA state. Instead immediately reallocate
the storage for SVE if SME is active, and disable SME if we change the SVE
vector length while there is no SME state active.
Disabling SME traps on SVE vector length changes would make the overall
code more complex since we would have a state where we have valid SME state
stored but might get a SME trap.
Fixes: 9e4ab6c89109 ("arm64/sme: Implement vector length configuration prctl()s")
Reported-by: David Spickett <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
Cc: [email protected]
---
arch/arm64/kernel/fpsimd.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7a1aeb95d7c3..a527b95c06e7 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -847,6 +847,9 @@ void sve_sync_from_fpsimd_zeropad(struct task_struct *task)
int vec_set_vector_length(struct task_struct *task, enum vec_type type,
unsigned long vl, unsigned long flags)
{
+ bool free_sme = false;
+ bool alloc_sve = true;
+
if (flags & ~(unsigned long)(PR_SVE_VL_INHERIT |
PR_SVE_SET_VL_ONEXEC))
return -EINVAL;
@@ -897,22 +900,37 @@ int vec_set_vector_length(struct task_struct *task, enum vec_type type,
task->thread.fp_type = FP_STATE_FPSIMD;
}
- if (system_supports_sme() && type == ARM64_VEC_SME) {
- task->thread.svcr &= ~(SVCR_SM_MASK |
- SVCR_ZA_MASK);
- clear_thread_flag(TIF_SME);
+ if (system_supports_sme()) {
+ if (type == ARM64_VEC_SME ||
+ !(task->thread.svcr & (SVCR_SM_MASK | SVCR_ZA_MASK))) {
+ /*
+ * We are changing the SME VL or weren't using
+ * SME anyway, discard the state and force a
+ * reallocation.
+ */
+ task->thread.svcr &= ~(SVCR_SM_MASK |
+ SVCR_ZA_MASK);
+ clear_thread_flag(TIF_SME);
+ free_sme = true;
+ } else {
+ alloc_sve = true;
+ }
}
if (task == current)
put_cpu_fpsimd_context();
/*
- * Force reallocation of task SVE and SME state to the correct
- * size on next use:
+ * Free the changed states if they are not in use, they will
+ * be reallocated to the correct size on next use. If we need
+ * SVE state due to having untouched SME state then reallocate
+ * it immediately.
*/
sve_free(task);
- if (system_supports_sme() && type == ARM64_VEC_SME)
+ if (free_sme)
sme_free(task);
+ if (alloc_sve)
+ sve_alloc(task, true);
task_set_vl(task, type, vl);
--
2.30.2
We just fixed an issue where changing the SVE VL while SME was active could
result in us attempting to save the streaming mode SVE vectors without any
backing storage. Add a test case which provokes that issue, ideally we
should also verify that the contents of ZA are unaffected by any of what we
did.
Note that since we need to keep streaming mode enabled we can't use any
syscalls to trigger the issue, we have to sit in a loop in usersapce and
hope to be preempted. The chosen numbers trigger with defconfig on all the
virtual platforms for me, this won't be 100% on all systems but avoid an
overcomplicated test implementation.
Signed-off-by: Mark Brown <[email protected]>
---
tools/testing/selftests/arm64/fp/vec-syscfg.c | 105 +++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
index 9bcfcdc34ee9..58ea4bde5be7 100644
--- a/tools/testing/selftests/arm64/fp/vec-syscfg.c
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -6,6 +6,7 @@
#include <assert.h>
#include <errno.h>
#include <fcntl.h>
+#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
@@ -39,9 +40,11 @@ struct vec_data {
int max_vl;
};
+#define VEC_SVE 0
+#define VEC_SME 1
static struct vec_data vec_data[] = {
- {
+ [VEC_SVE] = {
.name = "SVE",
.hwcap_type = AT_HWCAP,
.hwcap = HWCAP_SVE,
@@ -51,7 +54,7 @@ static struct vec_data vec_data[] = {
.prctl_set = PR_SVE_SET_VL,
.default_vl_file = "/proc/sys/abi/sve_default_vector_length",
},
- {
+ [VEC_SME] = {
.name = "SME",
.hwcap_type = AT_HWCAP2,
.hwcap = HWCAP2_SME,
@@ -644,18 +647,107 @@ static const test_type tests[] = {
prctl_set_all_vqs,
};
+static inline void smstart(void)
+{
+ asm volatile("msr S0_3_C4_C7_3, xzr");
+}
+
+static inline void smstart_sm(void)
+{
+ asm volatile("msr S0_3_C4_C3_3, xzr");
+}
+
+static inline void smstop(void)
+{
+ asm volatile("msr S0_3_C4_C6_3, xzr");
+}
+
+
+/*
+ * Verify we can change the SVE vector length while SME is active and
+ * continue to use SME afterwards.
+ */
+static void change_sve_with_za(void)
+{
+ struct vec_data *sve_data = &vec_data[VEC_SVE];
+ bool pass = true;
+ int ret, i;
+
+ if (sve_data->min_vl == sve_data->max_vl) {
+ ksft_print_msg("Only one SVE VL supported, can't change\n");
+ ksft_test_result_skip("change_sve_while_sme\n");
+ return;
+ }
+
+ /* Ensure we will trigger a change when we set the maximum */
+ ret = prctl(sve_data->prctl_set, sve_data->min_vl);
+ if (ret != sve_data->min_vl) {
+ ksft_print_msg("Failed to set SVE VL %d: %d\n",
+ sve_data->min_vl, ret);
+ pass = false;
+ }
+
+ /* Enable SM and ZA */
+ smstart();
+
+ /* Trigger another VL change */
+ ret = prctl(sve_data->prctl_set, sve_data->max_vl);
+ if (ret != sve_data->max_vl) {
+ ksft_print_msg("Failed to set SVE VL %d: %d\n",
+ sve_data->max_vl, ret);
+ pass = false;
+ }
+
+ /*
+ * Spin for a bit with SM enabled to try to trigger another
+ * save/restore. We can't use syscalls without exiting
+ * streaming mode.
+ */
+ for (i = 0; i < 100000000; i++)
+ smstart_sm();
+
+ /*
+ * TODO: Verify that ZA was preserved over the VL change and
+ * spin.
+ */
+
+ /* Clean up after ourselves */
+ smstop();
+ ret = prctl(sve_data->prctl_set, sve_data->default_vl);
+ if (ret != sve_data->default_vl) {
+ ksft_print_msg("Failed to restore SVE VL %d: %d\n",
+ sve_data->default_vl, ret);
+ pass = false;
+ }
+
+ ksft_test_result(pass, "change_sve_with_za\n");
+}
+
+typedef void (*test_all_type)(void);
+
+static const struct {
+ const char *name;
+ test_all_type test;
+} all_types_tests[] = {
+ { "change_sve_with_za", change_sve_with_za },
+};
+
int main(void)
{
+ bool all_supported = true;
int i, j;
ksft_print_header();
- ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data));
+ ksft_set_plan(ARRAY_SIZE(tests) * ARRAY_SIZE(vec_data) +
+ ARRAY_SIZE(all_types_tests));
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;
+ if (!supported)
+ all_supported = false;
for (j = 0; j < ARRAY_SIZE(tests); j++) {
if (supported)
@@ -666,5 +758,12 @@ int main(void)
}
}
+ for (i = 0; i < ARRAY_SIZE(all_types_tests); i++) {
+ if (all_supported)
+ all_types_tests[i].test();
+ else
+ ksft_test_result_skip("%s\n", all_types_tests[i].name);
+ }
+
ksft_exit_pass();
}
--
2.30.2
On a system with both SVE and SME when we change one of the VLs this should
not result in a change in the other VL. Add a check that this is in fact
the case to vec-syscfg.
Signed-off-by: Mark Brown <[email protected]>
---
tools/testing/selftests/arm64/fp/vec-syscfg.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/arm64/fp/vec-syscfg.c b/tools/testing/selftests/arm64/fp/vec-syscfg.c
index 58ea4bde5be7..5f648b97a06f 100644
--- a/tools/testing/selftests/arm64/fp/vec-syscfg.c
+++ b/tools/testing/selftests/arm64/fp/vec-syscfg.c
@@ -554,7 +554,8 @@ static void prctl_set_onexec(struct vec_data *data)
/* For each VQ verify that setting via prctl() does the right thing */
static void prctl_set_all_vqs(struct vec_data *data)
{
- int ret, vq, vl, new_vl;
+ int ret, vq, vl, new_vl, i;
+ int orig_vls[ARRAY_SIZE(vec_data)];
int errors = 0;
if (!data->min_vl || !data->max_vl) {
@@ -563,6 +564,9 @@ static void prctl_set_all_vqs(struct vec_data *data)
return;
}
+ for (i = 0; i < ARRAY_SIZE(vec_data); i++)
+ orig_vls[i] = vec_data[i].rdvl();
+
for (vq = SVE_VQ_MIN; vq <= SVE_VQ_MAX; vq++) {
vl = sve_vl_from_vq(vq);
@@ -585,6 +589,22 @@ static void prctl_set_all_vqs(struct vec_data *data)
errors++;
}
+ /* Did any other VLs change? */
+ for (i = 0; i < ARRAY_SIZE(vec_data); i++) {
+ if (&vec_data[i] == data)
+ continue;
+
+ if (!(getauxval(vec_data[i].hwcap_type) & vec_data[i].hwcap))
+ continue;
+
+ if (vec_data[i].rdvl() != orig_vls[i]) {
+ ksft_print_msg("%s VL changed from %d to %d\n",
+ vec_data[i].name, orig_vls[i],
+ vec_data[i].rdvl());
+ errors++;
+ }
+ }
+
/* Was that the VL we asked for? */
if (new_vl == vl)
continue;
--
2.30.2
I've confirmed on QEMU and Arm's FVP that this fixes the issue I was seeing.
From: Mark Brown <[email protected]>
Sent: 13 July 2023 21:06
To: Catalin Marinas <[email protected]>; Will Deacon <[email protected]>; Shuah Khan <[email protected]>
Cc: David Spickett <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Mark Brown <[email protected]>; [email protected] <[email protected]>
Subject: [PATCH 1/3] arm64/fpsimd: Ensure SME storage is allocated after SVE VL changes
?
When we reconfigure the SVE vector length we discard the backing storage
for the SVE vectors and then reallocate on next SVE use, leaving the SME
specific state alone. This means that we do not enable SME traps if they
were already disabled. That means that userspace code can enter streaming
mode without trapping, putting the task in a state where if we try to save
the state of the task we will fault.
Since the ABI does not specify that changing the SVE vector length disturbs
SME state, and since SVE code may not be aware of SME code in the process,
we shouldn't simply discard any ZA state. Instead immediately reallocate
the storage for SVE if SME is active, and disable SME if we change the SVE
vector length while there is no SME state active.
Disabling SME traps on SVE vector length changes would make the overall
code more complex since we would have a state where we have valid SME state
stored but might get a SME trap.
Fixes: 9e4ab6c89109 ("arm64/sme: Implement vector length configuration prctl()s")
Reported-by: David Spickett <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
Cc: [email protected]
---
?arch/arm64/kernel/fpsimd.c | 32 +++++++++++++++++++++++++-------
?1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7a1aeb95d7c3..a527b95c06e7 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -847,6 +847,9 @@ void sve_sync_from_fpsimd_zeropad(struct task_struct *task)
?int vec_set_vector_length(struct task_struct *task, enum vec_type type,
?????????????????????????? unsigned long vl, unsigned long flags)
?{
+?????? bool free_sme = false;
+?????? bool alloc_sve = true;
+
???????? if (flags & ~(unsigned long)(PR_SVE_VL_INHERIT |
????????????????????????????????????? PR_SVE_SET_VL_ONEXEC))
???????????????? return -EINVAL;
@@ -897,22 +900,37 @@ int vec_set_vector_length(struct task_struct *task, enum vec_type type,
???????????????? task->thread.fp_type = FP_STATE_FPSIMD;
???????? }
?
-?????? if (system_supports_sme() && type == ARM64_VEC_SME) {
-?????????????? task->thread.svcr &= ~(SVCR_SM_MASK |
-????????????????????????????????????? SVCR_ZA_MASK);
-?????????????? clear_thread_flag(TIF_SME);
+?????? if (system_supports_sme()) {
+?????????????? if (type == ARM64_VEC_SME ||
+?????????????????? !(task->thread.svcr & (SVCR_SM_MASK | SVCR_ZA_MASK))) {
+?????????????????????? /*
+??????????????????????? * We are changing the SME VL or weren't using
+??????????????????????? * SME anyway, discard the state and force a
+??????????????????????? * reallocation.
+??????????????????????? */
+?????????????????????? task->thread.svcr &= ~(SVCR_SM_MASK |
+????????????????????????????????????????????? SVCR_ZA_MASK);
+?????????????????????? clear_thread_flag(TIF_SME);
+?????????????????????? free_sme = true;
+?????????????? } else? {
+?????????????????????? alloc_sve = true;
+?????????????? }
???????? }
?
???????? if (task == current)
???????????????? put_cpu_fpsimd_context();
?
???????? /*
-??????? * Force reallocation of task SVE and SME state to the correct
-??????? * size on next use:
+??????? * Free the changed states if they are not in use, they will
+??????? * be reallocated to the correct size on next use.? If we need
+??????? * SVE state due to having untouched SME state then reallocate
+??????? * it immediately.
????????? */
???????? sve_free(task);
-?????? if (system_supports_sme() && type == ARM64_VEC_SME)
+?????? if (free_sme)
???????????????? sme_free(task);
+?????? if (alloc_sve)
+?????????????? sve_alloc(task, true);
?
???????? task_set_vl(task, type, vl);
?
--
2.30.2
On Thu, Jul 13, 2023 at 09:06:04PM +0100, Mark Brown wrote:
> When we reconfigure the SVE vector length we discard the backing storage
> for the SVE vectors and then reallocate on next SVE use, leaving the SME
> specific state alone. This means that we do not enable SME traps if they
> were already disabled. That means that userspace code can enter streaming
> mode without trapping, putting the task in a state where if we try to save
> the state of the task we will fault.
>
> Since the ABI does not specify that changing the SVE vector length disturbs
> SME state, and since SVE code may not be aware of SME code in the process,
> we shouldn't simply discard any ZA state. Instead immediately reallocate
> the storage for SVE if SME is active, and disable SME if we change the SVE
> vector length while there is no SME state active.
What is the advantage of keep the old behaviour in this case? In other
words, if it's acceptable to reallocate the state when SME is active, why
not just reallocate in all cases?
Will
On Thu, Jul 20, 2023 at 11:52:36AM +0100, Will Deacon wrote:
> On Thu, Jul 13, 2023 at 09:06:04PM +0100, Mark Brown wrote:
> > Since the ABI does not specify that changing the SVE vector length disturbs
> > SME state, and since SVE code may not be aware of SME code in the process,
> > we shouldn't simply discard any ZA state. Instead immediately reallocate
> > the storage for SVE if SME is active, and disable SME if we change the SVE
> > vector length while there is no SME state active.
> What is the advantage of keep the old behaviour in this case? In other
> words, if it's acceptable to reallocate the state when SME is active, why
> not just reallocate in all cases?
It was minimising the changes to the status quo given how attached
people often are to these things.