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]>
---
Changes in v2:
- Always reallocate the SVE state.
- Rebase onto v6.5-rc2.
- Link to v1: https://lore.kernel.org/r/20230713-arm64-fix-sve-sme-vl-change-v1-0-129dd8611413@kernel.org
---
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 | 33 +++++--
tools/testing/selftests/arm64/fp/vec-syscfg.c | 127 +++++++++++++++++++++++++-
2 files changed, 148 insertions(+), 12 deletions(-)
---
base-commit: 06785562d1b99ff6dc1cd0af54be5e3ff999dc02
change-id: 20230713-arm64-fix-sve-sme-vl-change-60eb1fa6a707
Best regards,
--
Mark Brown <[email protected]>
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
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, 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 | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7a1aeb95d7c3..89d54a5242d1 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -847,6 +847,8 @@ 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;
+
if (flags & ~(unsigned long)(PR_SVE_VL_INHERIT |
PR_SVE_SET_VL_ONEXEC))
return -EINVAL;
@@ -897,21 +899,36 @@ 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;
+ }
}
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, SME will be
+ * reallocated to the correct size on next use and we just
+ * allocate SVE now in case it is needed for use in streaming
+ * mode.
*/
- sve_free(task);
- if (system_supports_sme() && type == ARM64_VEC_SME)
+ if (system_supports_sve()) {
+ sve_free(task);
+ sve_alloc(task, true);
+ }
+
+ if (free_sme)
sme_free(task);
task_set_vl(task, type, vl);
--
2.30.2
On Thu, 20 Jul 2023 19:38:57 +0100, Mark Brown wrote:
> 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.
>
>
Applied first patch to arm64 (for-next/fixes), thanks!
[1/3] arm64/fpsimd: Ensure SME storage is allocated after SVE VL changes
https://git.kernel.org/arm64/c/d4d5be94a878
I'll look at the selftests stuff for 6.6 when I get to that (probably
next week).
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
On Thu, 20 Jul 2023 19:38:57 +0100, Mark Brown wrote:
> 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.
>
>
Applied to arm64 (for-next/selftests), thanks!
[1/3] arm64/fpsimd: Ensure SME storage is allocated after SVE VL changes
https://git.kernel.org/arm64/c/d4d5be94a878
[2/3] kselftest/arm64: Add a test case for SVE VL changes with SME active
https://git.kernel.org/arm64/c/0c7c237b1c35
[3/3] kselftest/arm64: Validate that changing one VL type does not affect another
https://git.kernel.org/arm64/c/0aeead9bb240
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev