[PATCH 0/6] arm64/sme: Collected SME fixes

[PATCH 0/6] arm64/sme: Collected SME fixes

[Mark Brown]

This series collects the various SME related fixes that were previously
posted separately.  These should address all the issues I am aware of so
a patch which reenables the SME configuration option is also included.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (6):
      arm64/sme: Flush foreign register state in do_sme_acc()
      arm64/fp: Don't corrupt FPMR when streaming mode changes
      arm64/ptrace: Zero FPMR on streaming mode entry/exit
      arm64/signal: Consistently invalidate the in register FP state in restore
      arm64/signal: Avoid corruption of SME state when entering signal handler
      arm64/sme: Reenable SME

 arch/arm64/Kconfig              |  1 -
 arch/arm64/include/asm/fpsimd.h |  1 +
 arch/arm64/kernel/fpsimd.c      | 49 +++++++++++++++++++++--
 arch/arm64/kernel/ptrace.c      | 12 +++++-
 arch/arm64/kernel/signal.c      | 89 +++++++++++------------------------------
 5 files changed, 79 insertions(+), 73 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241202-arm64-sme-reenable-98e64c161a8e

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Mark Brown]

When do_sme_acc() runs with foreign FP state it does not do any updates of
the task structure, relying on the next return to userspace to reload the
register state appropriately, but leaves the task's last loaded CPU
untouched. This means that if the task returns to userspace on the last
CPU it ran on then the checks in fpsimd_bind_task_to_cpu() will incorrectly
determine that the register state on the CPU is current and suppress reload
of the floating point register state before returning to userspace. This
will result in spurious warnings due to SME access traps occuring for the
task after TIF_SME is set.

Call fpsimd_flush_task_state() to invalidate the last loaded CPU
recorded in the task, forcing detection of the task as foreign.

Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")
Reported-by: Mark Rutlamd <mark.rutland@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kernel/fpsimd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 8c4c1a2186cc510a7826d15ec36225857c07ed71..eca0b6a2fc6fa25d8c850a5b9e109b4d58809f54 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1460,6 +1460,8 @@ void do_sme_acc(unsigned long esr, struct pt_regs *regs)
 		sme_set_vq(vq_minus_one);
 
 		fpsimd_bind_task_to_cpu();
+	} else {
+		fpsimd_flush_task_state(current);
 	}
 
 	put_cpu_fpsimd_context();

-- 
2.39.5

[PATCH 2/6] arm64/fp: Don't corrupt FPMR when streaming mode changes

[Mark Brown]

When we enter or exit streaming more FPMR is reset to 0.  This means
that when restoring the floating point state from memory we need to
restore FPMR after we restore SVCR, otherwise if we are entering or
exiting streaming mode as part of loading the new state the value of
FPMR will be corrupted.

Fixes: 203f2b95a882 ("arm64/fpsimd: Support FEAT_FPMR")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index eca0b6a2fc6fa25d8c850a5b9e109b4d58809f54..a3bb17c88942eba031d26e9f75ad46f37b6dc621 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -359,9 +359,6 @@ static void task_fpsimd_load(void)
 	WARN_ON(preemptible());
 	WARN_ON(test_thread_flag(TIF_KERNEL_FPSTATE));
 
-	if (system_supports_fpmr())
-		write_sysreg_s(current->thread.uw.fpmr, SYS_FPMR);
-
 	if (system_supports_sve() || system_supports_sme()) {
 		switch (current->thread.fp_type) {
 		case FP_STATE_FPSIMD:
@@ -413,6 +410,9 @@ static void task_fpsimd_load(void)
 			restore_ffr = system_supports_fa64();
 	}
 
+	if (system_supports_fpmr())
+		write_sysreg_s(current->thread.uw.fpmr, SYS_FPMR);
+
 	if (restore_sve_regs) {
 		WARN_ON_ONCE(current->thread.fp_type != FP_STATE_SVE);
 		sve_load_state(sve_pffr(&current->thread),

-- 
2.39.5

[PATCH 3/6] arm64/ptrace: Zero FPMR on streaming mode entry/exit

[Mark Brown]

When FPMR and SME are both present then entering and exiting streaming mode
clears FPMR in the same manner as it clears the V/Z and P registers.
Since entering and exiting streaming mode via ptrace is expected to have
the same effect as doing so via SMSTART/SMSTOP it should clear FPMR too
but this was missed when FPMR support was added. Add the required reset
of FPMR.

Since changing the vector length resets SVCR a SME vector length change
implemented via a write to ZA can trigger an exit of streaming mode and
we need to check when writing to ZA as well.

Fixes: 4035c22ef7d4 ("arm64/ptrace: Expose FPMR via ptrace")
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kernel/ptrace.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index e4437f62a2cda93734052c44b48886db83d75b3e..43a9397d5903ff87b608befdcaed3f9a7e48f976 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -877,6 +877,7 @@ static int sve_set_common(struct task_struct *target,
 			  const void *kbuf, const void __user *ubuf,
 			  enum vec_type type)
 {
+	u64 old_svcr = target->thread.svcr;
 	int ret;
 	struct user_sve_header header;
 	unsigned int vq;
@@ -908,8 +909,6 @@ static int sve_set_common(struct task_struct *target,
 
 	/* Enter/exit streaming mode */
 	if (system_supports_sme()) {
-		u64 old_svcr = target->thread.svcr;
-
 		switch (type) {
 		case ARM64_VEC_SVE:
 			target->thread.svcr &= ~SVCR_SM_MASK;
@@ -1008,6 +1007,10 @@ static int sve_set_common(struct task_struct *target,
 				 start, end);
 
 out:
+	/* If we entered or exited streaming mode then reset FPMR */
+	if ((target->thread.svcr & SVCR_SM) != (old_svcr & SVCR_SM))
+		target->thread.uw.fpmr = 0;
+
 	fpsimd_flush_task_state(target);
 	return ret;
 }
@@ -1104,6 +1107,7 @@ static int za_set(struct task_struct *target,
 		  unsigned int pos, unsigned int count,
 		  const void *kbuf, const void __user *ubuf)
 {
+	u64 old_svcr = target->thread.svcr;
 	int ret;
 	struct user_za_header header;
 	unsigned int vq;
@@ -1184,6 +1188,10 @@ static int za_set(struct task_struct *target,
 	target->thread.svcr |= SVCR_ZA_MASK;
 
 out:
+	/* If we entered or exited streaming mode then reset FPMR */
+	if ((target->thread.svcr & SVCR_SM) != (old_svcr & SVCR_SM))
+		target->thread.uw.fpmr = 0;
+
 	fpsimd_flush_task_state(target);
 	return ret;
 }

-- 
2.39.5

[PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

[Mark Brown]

When restoring the SVE and SME specific floating point register states we
flush the task floating point state, marking the hardware state as stale so
that preemption does not result in us saving register state from the signal
handler on top of the restored context and forcing a reload from memory.
For the plain FPSIMD state we don't do this, we just copy the state from
userspace and then force an immediate reload of the register state.
This isn't racy against context switch since we copy the incoming data
onto the stack rather than directly into the task struct but it's still
messy and inconsistent.

Simplify things and avoid a potential source of error by moving the
invalidation of the CPU state to the main restore_sigframe() and
reworking the restore of the FPSIMD state to update the task struct and
rely on loading as part of the general do_notify_resume() handling for
return to user like we do for the SVE and SME state.

As a result of this the only user of fpsimd_update_current_state() is
the 32 bit signal code which should not have any SVE state, add an
assert there that we don't have SVE enabled.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/fpsimd.c |  2 +-
 arch/arm64/kernel/signal.c | 70 +++++++++++++++-------------------------------
 2 files changed, 23 insertions(+), 49 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index a3bb17c88942eba031d26e9f75ad46f37b6dc621..f02762762dbcf954e9add6dfd3575ae7055b6b0e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1828,7 +1828,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 	get_cpu_fpsimd_context();
 
 	current->thread.uw.fpsimd_state = *state;
-	if (test_thread_flag(TIF_SVE))
+	if (WARN_ON_ONCE(test_thread_flag(TIF_SVE)))
 		fpsimd_to_sve(current);
 
 	task_fpsimd_load();
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 14ac6fdb872b9672e4b16a097f1b577aae8dec50..abd0907061fe664bf22d1995319f9559c4bbed91 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -271,7 +271,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 
 static int restore_fpsimd_context(struct user_ctxs *user)
 {
-	struct user_fpsimd_state fpsimd;
+	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
 	int err = 0;
 
 	/* check the size information */
@@ -279,18 +279,14 @@ static int restore_fpsimd_context(struct user_ctxs *user)
 		return -EINVAL;
 
 	/* copy the FP and status/control registers */
-	err = __copy_from_user(fpsimd.vregs, &(user->fpsimd->vregs),
-			       sizeof(fpsimd.vregs));
-	__get_user_error(fpsimd.fpsr, &(user->fpsimd->fpsr), err);
-	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
+	err = __copy_from_user(fpsimd->vregs, &(user->fpsimd->vregs),
+			       sizeof(fpsimd->vregs));
+	__get_user_error(fpsimd->fpsr, &(user->fpsimd->fpsr), err);
+	__get_user_error(fpsimd->fpcr, &(user->fpsimd->fpcr), err);
 
 	clear_thread_flag(TIF_SVE);
 	current->thread.fp_type = FP_STATE_FPSIMD;
 
-	/* load the hardware registers from the fpsimd_state structure */
-	if (!err)
-		fpsimd_update_current_state(&fpsimd);
-
 	return err ? -EFAULT : 0;
 }
 
@@ -396,7 +392,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 {
 	int err = 0;
 	unsigned int vl, vq;
-	struct user_fpsimd_state fpsimd;
+	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
 	u16 user_vl, flags;
 
 	if (user->sve_size < sizeof(*user->sve))
@@ -439,16 +435,6 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (user->sve_size < SVE_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
 
-	/*
-	 * Careful: we are about __copy_from_user() directly into
-	 * thread.sve_state with preemption enabled, so protection is
-	 * needed to prevent a racing context switch from writing stale
-	 * registers back over the new data.
-	 */
-
-	fpsimd_flush_task_state(current);
-	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
-
 	sve_alloc(current, true);
 	if (!current->thread.sve_state) {
 		clear_thread_flag(TIF_SVE);
@@ -471,14 +457,10 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 fpsimd_only:
 	/* copy the FP and status/control registers */
 	/* restore_sigframe() already checked that user->fpsimd != NULL. */
-	err = __copy_from_user(fpsimd.vregs, user->fpsimd->vregs,
-			       sizeof(fpsimd.vregs));
-	__get_user_error(fpsimd.fpsr, &user->fpsimd->fpsr, err);
-	__get_user_error(fpsimd.fpcr, &user->fpsimd->fpcr, err);
-
-	/* load the hardware registers from the fpsimd_state structure */
-	if (!err)
-		fpsimd_update_current_state(&fpsimd);
+	err = __copy_from_user(fpsimd->vregs, user->fpsimd->vregs,
+			       sizeof(fpsimd->vregs));
+	__get_user_error(fpsimd->fpsr, &user->fpsimd->fpsr, err);
+	__get_user_error(fpsimd->fpcr, &user->fpsimd->fpcr, err);
 
 	return err ? -EFAULT : 0;
 }
@@ -587,16 +569,6 @@ static int restore_za_context(struct user_ctxs *user)
 	if (user->za_size < ZA_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
 
-	/*
-	 * Careful: we are about __copy_from_user() directly into
-	 * thread.sme_state with preemption enabled, so protection is
-	 * needed to prevent a racing context switch from writing stale
-	 * registers back over the new data.
-	 */
-
-	fpsimd_flush_task_state(current);
-	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
-
 	sme_alloc(current, true);
 	if (!current->thread.sme_state) {
 		current->thread.svcr &= ~SVCR_ZA_MASK;
@@ -664,16 +636,6 @@ static int restore_zt_context(struct user_ctxs *user)
 	if (nregs != 1)
 		return -EINVAL;
 
-	/*
-	 * Careful: we are about __copy_from_user() directly into
-	 * thread.zt_state with preemption enabled, so protection is
-	 * needed to prevent a racing context switch from writing stale
-	 * registers back over the new data.
-	 */
-
-	fpsimd_flush_task_state(current);
-	/* From now, fpsimd_thread_switch() won't touch ZT in thread state */
-
 	err = __copy_from_user(thread_zt_state(&current->thread),
 			       (char __user const *)user->zt +
 					ZT_SIG_REGS_OFFSET,
@@ -1028,6 +990,18 @@ static int restore_sigframe(struct pt_regs *regs,
 	if (err == 0)
 		err = parse_user_sigframe(&user, sf);
 
+	/*
+	 * Careful: we are about __copy_from_user() directly into
+	 * thread floating point state with preemption enabled, so
+	 * protection is needed to prevent a racing context switch
+	 * from writing stale registers back over the new data. Mark
+	 * the register floating point state as invalid and unbind the
+	 * task from the CPU to force a reload before we return to
+	 * userspace. fpsimd_flush_task_state() has a check for FP
+	 * support.
+	 */
+	fpsimd_flush_task_state(current);
+
 	if (err == 0 && system_supports_fpsimd()) {
 		if (!user.fpsimd)
 			return -EINVAL;

-- 
2.39.5

[PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Mark Brown]

We intend that signal handlers are entered with PSTATE.{SM,ZA}={0,0}.
The logic for this in setup_return() manipulates the saved state and
live CPU state in an unsafe manner, and consequently, when a task enters
a signal handler:

 * The task entering the signal handler might not have its PSTATE.{SM,ZA}
   bits cleared, and other register state that is affected by changes to
   PSTATE.{SM,ZA} might not be zeroed as expected.

 * An unrelated task might have its PSTATE.{SM,ZA} bits cleared
   unexpectedly, potentially zeroing other register state that is
   affected by changes to PSTATE.{SM,ZA}.

   Tasks which do not set PSTATE.{SM,ZA} (i.e. those only using plain
   FPSIMD or non-streaming SVE) are not affected, as there is no
   resulting change to PSTATE.{SM,ZA}.

Consider for example two tasks on one CPU:

 A: Begins signal entry in kernel mode, is preempted prior to SMSTOP.
 B: Using SM and/or ZA in userspace with register state current on the
    CPU, is preempted.
 A: Scheduled in, no register state changes made as in kernel mode.
 A: Executes SMSTOP, modifying live register state.
 A: Scheduled out.
 B: Scheduled in, fpsimd_thread_switch() sees the register state on the
    CPU is tracked as being that for task B so the state is not reloaded
    prior to returning to userspace.

Task B is now running with SM and ZA incorrectly cleared.

Fix this by:

 * Checking TIF_FOREIGN_FPSTATE, and only updating the saved or live
   state as appropriate.

 * Using {get,put}_cpu_fpsimd_context() to ensure mutual exclusion
   against other code which manipulates this state. To allow their use,
   the logic is moved into a new fpsimd_enter_sighandler() helper in
   fpsimd.c.

This race has been observed intermittently with fp-stress, especially
with preempt disabled, commonly but not exclusively reporting "Bad SVCR: 0".

While we're at it also fix a discrepancy between in register and in memory
entries. When operating on the register state we issue a SMSTOP, exiting
streaming mode if we were in it. This clears the V/Z and P register and
FPMR but nothing else. The in memory version clears all the user FPSIMD
state including FPCR and FPSR but does not clear FPMR. Add the clear of
FPMR and limit the existing memset() to only cover the vregs, preserving
the state of FPCR and FPSR like SMSTOP does.

Fixes: 40a8e87bb3285 ("arm64/sme: Disable ZA and streaming mode when handling signals")
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/include/asm/fpsimd.h |  1 +
 arch/arm64/kernel/fpsimd.c      | 39 +++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c      | 19 +------------------
 3 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index f2a84efc361858d4deda99faf1967cc7cac386c1..09af7cfd9f6c2cec26332caa4c254976e117b1bf 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -76,6 +76,7 @@ extern void fpsimd_load_state(struct user_fpsimd_state *state);
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
+extern void fpsimd_enter_sighandler(void);
 extern void fpsimd_signal_preserve_current_state(void);
 extern void fpsimd_preserve_current_state(void);
 extern void fpsimd_restore_current_state(void);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index f02762762dbcf954e9add6dfd3575ae7055b6b0e..c5465c8ec467cb1ab8bd211dc5370f91aa2bcf35 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1696,6 +1696,45 @@ void fpsimd_signal_preserve_current_state(void)
 		sve_to_fpsimd(current);
 }
 
+/*
+ * Called by the signal handling code when preparing current to enter
+ * a signal handler. Currently this only needs to take care of exiting
+ * streaming mode and clearing ZA on SME systems.
+ */
+void fpsimd_enter_sighandler(void)
+{
+	if (!system_supports_sme())
+		return;
+
+	get_cpu_fpsimd_context();
+
+	if (test_thread_flag(TIF_FOREIGN_FPSTATE)) {
+		/*
+		 * Exiting streaming mode zeros the V/Z and P
+		 * registers and FPMR.  Zero FPMR and the V registers,
+		 * marking the state as FPSIMD only to force a clear
+		 * of the remaining bits during reload if needed.
+		 */
+		if (current->thread.svcr & SVCR_SM_MASK) {
+			memset(&current->thread.uw.fpsimd_state.vregs, 0,
+			       sizeof(current->thread.uw.fpsimd_state.vregs));
+			current->thread.uw.fpmr = 0;
+			current->thread.fp_type = FP_STATE_FPSIMD;
+		}
+
+		current->thread.svcr &= ~(SVCR_ZA_MASK |
+					  SVCR_SM_MASK);
+
+		/* Ensure any copies on other CPUs aren't reused */
+		fpsimd_flush_task_state(current);
+	} else {
+		/* The register state is current, just update it. */
+		sme_smstop();
+	}
+
+	put_cpu_fpsimd_context();
+}
+
 /*
  * Called by KVM when entering the guest.
  */
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index abd0907061fe664bf22d1995319f9559c4bbed91..335c2327baf74eac9634cf594855dbf26a7d6b01 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -1461,24 +1461,7 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
 	/* TCO (Tag Check Override) always cleared for signal handlers */
 	regs->pstate &= ~PSR_TCO_BIT;
 
-	/* Signal handlers are invoked with ZA and streaming mode disabled */
-	if (system_supports_sme()) {
-		/*
-		 * If we were in streaming mode the saved register
-		 * state was SVE but we will exit SM and use the
-		 * FPSIMD register state - flush the saved FPSIMD
-		 * register state in case it gets loaded.
-		 */
-		if (current->thread.svcr & SVCR_SM_MASK) {
-			memset(&current->thread.uw.fpsimd_state, 0,
-			       sizeof(current->thread.uw.fpsimd_state));
-			current->thread.fp_type = FP_STATE_FPSIMD;
-		}
-
-		current->thread.svcr &= ~(SVCR_ZA_MASK |
-					  SVCR_SM_MASK);
-		sme_smstop();
-	}
+	fpsimd_enter_sighandler();
 
 	if (ksig->ka.sa.sa_flags & SA_RESTORER)
 		sigtramp = ksig->ka.sa.sa_restorer;

-- 
2.39.5

[PATCH 6/6] arm64/sme: Reenable SME

[Mark Brown]

Now that fixes for all the known issues with SME have been applied
remove the BROKEN dependency from it so it's generally available again.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 100570a048c5e8892c0112704f9ca74c4fc55b27..7e3182dd6fa0dadd961c352f88484cff0e520eaa 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2270,7 +2270,6 @@ config ARM64_SME
 	bool "ARM Scalable Matrix Extension support"
 	default y
 	depends on ARM64_SVE
-	depends on BROKEN
 	help
 	  The Scalable Matrix Extension (SME) is an extension to the AArch64
 	  execution state which utilises a substantial subset of the SVE

-- 
2.39.5

Re: [PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Dave Martin]

On Tue, Dec 03, 2024 at 12:45:53PM +0000, Mark Brown wrote:
> When do_sme_acc() runs with foreign FP state it does not do any updates of
> the task structure, relying on the next return to userspace to reload the
> register state appropriately, but leaves the task's last loaded CPU
> untouched. This means that if the task returns to userspace on the last
> CPU it ran on then the checks in fpsimd_bind_task_to_cpu() will incorrectly
> determine that the register state on the CPU is current and suppress reload
> of the floating point register state before returning to userspace. This
> will result in spurious warnings due to SME access traps occuring for the
> task after TIF_SME is set.
> 
> Call fpsimd_flush_task_state() to invalidate the last loaded CPU
> recorded in the task, forcing detection of the task as foreign.
> 
> Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")
> Reported-by: Mark Rutlamd <mark.rutland@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kernel/fpsimd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 8c4c1a2186cc510a7826d15ec36225857c07ed71..eca0b6a2fc6fa25d8c850a5b9e109b4d58809f54 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1460,6 +1460,8 @@ void do_sme_acc(unsigned long esr, struct pt_regs *regs)
>  		sme_set_vq(vq_minus_one);
>  
>  		fpsimd_bind_task_to_cpu();
> +	} else {
> +		fpsimd_flush_task_state(current);

TIF_FOREIGN_FPSTATE is (or was) a cache of the task<->CPU binding that
you're clobbering here.

So, this fpsimd_flush_task_state() should have no effect unless
TIF_FOREIGN_FPSTATE is already wrong?  I'm wondering if the apparent
need for this means that there is an undiagnosed bug elsewhere.

(My understanding is based on FPSIMD/SVE; I'm less familiar with the
SME changes, so I may be missing something important here.)

[...]

Cheers
---Dave

Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Dave Martin]

On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
> We intend that signal handlers are entered with PSTATE.{SM,ZA}={0,0}.
> The logic for this in setup_return() manipulates the saved state and
> live CPU state in an unsafe manner, and consequently, when a task enters
> a signal handler:
> 
>  * The task entering the signal handler might not have its PSTATE.{SM,ZA}
>    bits cleared, and other register state that is affected by changes to
>    PSTATE.{SM,ZA} might not be zeroed as expected.
> 
>  * An unrelated task might have its PSTATE.{SM,ZA} bits cleared
>    unexpectedly, potentially zeroing other register state that is
>    affected by changes to PSTATE.{SM,ZA}.
> 
>    Tasks which do not set PSTATE.{SM,ZA} (i.e. those only using plain
>    FPSIMD or non-streaming SVE) are not affected, as there is no
>    resulting change to PSTATE.{SM,ZA}.
> 
> Consider for example two tasks on one CPU:
> 
>  A: Begins signal entry in kernel mode, is preempted prior to SMSTOP.
>  B: Using SM and/or ZA in userspace with register state current on the
>     CPU, is preempted.
>  A: Scheduled in, no register state changes made as in kernel mode.
>  A: Executes SMSTOP, modifying live register state.
>  A: Scheduled out.
>  B: Scheduled in, fpsimd_thread_switch() sees the register state on the
>     CPU is tracked as being that for task B so the state is not reloaded
>     prior to returning to userspace.
> 
> Task B is now running with SM and ZA incorrectly cleared.
> 
> Fix this by:
> 
>  * Checking TIF_FOREIGN_FPSTATE, and only updating the saved or live
>    state as appropriate.
> 
>  * Using {get,put}_cpu_fpsimd_context() to ensure mutual exclusion
>    against other code which manipulates this state. To allow their use,
>    the logic is moved into a new fpsimd_enter_sighandler() helper in
>    fpsimd.c.
> 
> This race has been observed intermittently with fp-stress, especially
> with preempt disabled, commonly but not exclusively reporting "Bad SVCR: 0".
> 
> While we're at it also fix a discrepancy between in register and in memory
> entries. When operating on the register state we issue a SMSTOP, exiting
> streaming mode if we were in it. This clears the V/Z and P register and
> FPMR but nothing else. The in memory version clears all the user FPSIMD
> state including FPCR and FPSR but does not clear FPMR. Add the clear of
> FPMR and limit the existing memset() to only cover the vregs, preserving
> the state of FPCR and FPSR like SMSTOP does.
> 
> Fixes: 40a8e87bb3285 ("arm64/sme: Disable ZA and streaming mode when handling signals")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/include/asm/fpsimd.h |  1 +
>  arch/arm64/kernel/fpsimd.c      | 39 +++++++++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/signal.c      | 19 +------------------
>  3 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index f2a84efc361858d4deda99faf1967cc7cac386c1..09af7cfd9f6c2cec26332caa4c254976e117b1bf 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -76,6 +76,7 @@ extern void fpsimd_load_state(struct user_fpsimd_state *state);
>  extern void fpsimd_thread_switch(struct task_struct *next);
>  extern void fpsimd_flush_thread(void);
>  
> +extern void fpsimd_enter_sighandler(void);
>  extern void fpsimd_signal_preserve_current_state(void);
>  extern void fpsimd_preserve_current_state(void);
>  extern void fpsimd_restore_current_state(void);
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index f02762762dbcf954e9add6dfd3575ae7055b6b0e..c5465c8ec467cb1ab8bd211dc5370f91aa2bcf35 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1696,6 +1696,45 @@ void fpsimd_signal_preserve_current_state(void)
>  		sve_to_fpsimd(current);
>  }
>  
> +/*
> + * Called by the signal handling code when preparing current to enter
> + * a signal handler. Currently this only needs to take care of exiting
> + * streaming mode and clearing ZA on SME systems.
> + */
> +void fpsimd_enter_sighandler(void)
> +{
> +	if (!system_supports_sme())
> +		return;
> +
> +	get_cpu_fpsimd_context();
> +
> +	if (test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> +		/*
> +		 * Exiting streaming mode zeros the V/Z and P
> +		 * registers and FPMR.  Zero FPMR and the V registers,
> +		 * marking the state as FPSIMD only to force a clear
> +		 * of the remaining bits during reload if needed.
> +		 */
> +		if (current->thread.svcr & SVCR_SM_MASK) {
> +			memset(&current->thread.uw.fpsimd_state.vregs, 0,
> +			       sizeof(current->thread.uw.fpsimd_state.vregs));

Do we need to hold the CPU fpsimd context across this memset?

IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
dumping of the regs into thread_struct (from current's PoV), but never
spontaneously set again.  So ... -> [*]

> +			current->thread.uw.fpmr = 0;
> +			current->thread.fp_type = FP_STATE_FPSIMD;
> +		}
> +
> +		current->thread.svcr &= ~(SVCR_ZA_MASK |
> +					  SVCR_SM_MASK);
> +
> +		/* Ensure any copies on other CPUs aren't reused */
> +		fpsimd_flush_task_state(current);

(This is very similar to fpsimd_flush_thread(); can they be unified?)

> +	} else {
> +		/* The register state is current, just update it. */
> +		sme_smstop();

... [*] the critical thing seems to be that the CPU fpsimd context is
held from the test on TIF_FOREIGN_FPSTATE, across this else clause.

(Whether or not this is a worthwhile optimisation is another matter.
But if the behaviour of TIF_FOREIGN_FPSTATE is still the same, then it
may be a good idea to avoid sending mixed messages about this in the
code.)

(A similar argument applies in fpsimd_flush_thread().)

[...]

Cheers
---Dave

Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

[Dave Martin]

On Tue, Dec 03, 2024 at 12:45:56PM +0000, Mark Brown wrote:
> When restoring the SVE and SME specific floating point register states we
> flush the task floating point state, marking the hardware state as stale so
> that preemption does not result in us saving register state from the signal
> handler on top of the restored context and forcing a reload from memory.
> For the plain FPSIMD state we don't do this, we just copy the state from
> userspace and then force an immediate reload of the register state.
> This isn't racy against context switch since we copy the incoming data
> onto the stack rather than directly into the task struct but it's still
> messy and inconsistent.
> 
> Simplify things and avoid a potential source of error by moving the
> invalidation of the CPU state to the main restore_sigframe() and
> reworking the restore of the FPSIMD state to update the task struct and
> rely on loading as part of the general do_notify_resume() handling for
> return to user like we do for the SVE and SME state.
> 
> As a result of this the only user of fpsimd_update_current_state() is
> the 32 bit signal code which should not have any SVE state, add an
> assert there that we don't have SVE enabled.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c |  2 +-
>  arch/arm64/kernel/signal.c | 70 +++++++++++++++-------------------------------
>  2 files changed, 23 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index a3bb17c88942eba031d26e9f75ad46f37b6dc621..f02762762dbcf954e9add6dfd3575ae7055b6b0e 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1828,7 +1828,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state)
>  	get_cpu_fpsimd_context();
>  
>  	current->thread.uw.fpsimd_state = *state;
> -	if (test_thread_flag(TIF_SVE))

Add a comment here?

(I guess people can dig for the commit message, but it's easy to miss
the original intent of WARNs when editing code.)

> +	if (WARN_ON_ONCE(test_thread_flag(TIF_SVE)))
>  		fpsimd_to_sve(current);
>  
>  	task_fpsimd_load();
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 14ac6fdb872b9672e4b16a097f1b577aae8dec50..abd0907061fe664bf22d1995319f9559c4bbed91 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -271,7 +271,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>  
>  static int restore_fpsimd_context(struct user_ctxs *user)
>  {
> -	struct user_fpsimd_state fpsimd;
> +	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
>  	int err = 0;
>  
>  	/* check the size information */
> @@ -279,18 +279,14 @@ static int restore_fpsimd_context(struct user_ctxs *user)
>  		return -EINVAL;
>  
>  	/* copy the FP and status/control registers */
> -	err = __copy_from_user(fpsimd.vregs, &(user->fpsimd->vregs),
> -			       sizeof(fpsimd.vregs));
> -	__get_user_error(fpsimd.fpsr, &(user->fpsimd->fpsr), err);
> -	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
> +	err = __copy_from_user(fpsimd->vregs, &(user->fpsimd->vregs),
> +			       sizeof(fpsimd->vregs));
> +	__get_user_error(fpsimd->fpsr, &(user->fpsimd->fpsr), err);
> +	__get_user_error(fpsimd->fpcr, &(user->fpsimd->fpcr), err);

It does kinda make sense to align this with the way SVE/SME are handled.

I just left this as-is when developing the SVE support, because it
wasn't feasible to stage the SVE state via the stack, and I didn't want
to break the world by messing with the FPSIMD-only code paths any more
than I had to.

But you're right that we don't really gain anything any more by doing
things a special way for the plain FPSIMD case...

Doing everything the same way should help maintainability.

>  
>  	clear_thread_flag(TIF_SVE);
>  	current->thread.fp_type = FP_STATE_FPSIMD;
>  
> -	/* load the hardware registers from the fpsimd_state structure */
> -	if (!err)
> -		fpsimd_update_current_state(&fpsimd);
> -
>  	return err ? -EFAULT : 0;
>  }
>  
> @@ -396,7 +392,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  {
>  	int err = 0;
>  	unsigned int vl, vq;
> -	struct user_fpsimd_state fpsimd;
> +	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
>  	u16 user_vl, flags;
>  
>  	if (user->sve_size < sizeof(*user->sve))
> @@ -439,16 +435,6 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	if (user->sve_size < SVE_SIG_CONTEXT_SIZE(vq))
>  		return -EINVAL;
>  
> -	/*
> -	 * Careful: we are about __copy_from_user() directly into
> -	 * thread.sve_state with preemption enabled, so protection is
> -	 * needed to prevent a racing context switch from writing stale
> -	 * registers back over the new data.
> -	 */
> -
> -	fpsimd_flush_task_state(current);
> -	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
> -
>  	sve_alloc(current, true);
>  	if (!current->thread.sve_state) {
>  		clear_thread_flag(TIF_SVE);
> @@ -471,14 +457,10 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  fpsimd_only:
>  	/* copy the FP and status/control registers */
>  	/* restore_sigframe() already checked that user->fpsimd != NULL. */
> -	err = __copy_from_user(fpsimd.vregs, user->fpsimd->vregs,
> -			       sizeof(fpsimd.vregs));
> -	__get_user_error(fpsimd.fpsr, &user->fpsimd->fpsr, err);
> -	__get_user_error(fpsimd.fpcr, &user->fpsimd->fpcr, err);
> -
> -	/* load the hardware registers from the fpsimd_state structure */
> -	if (!err)
> -		fpsimd_update_current_state(&fpsimd);
> +	err = __copy_from_user(fpsimd->vregs, user->fpsimd->vregs,
> +			       sizeof(fpsimd->vregs));
> +	__get_user_error(fpsimd->fpsr, &user->fpsimd->fpsr, err);
> +	__get_user_error(fpsimd->fpcr, &user->fpsimd->fpcr, err);
>  
>  	return err ? -EFAULT : 0;
>  }
> @@ -587,16 +569,6 @@ static int restore_za_context(struct user_ctxs *user)
>  	if (user->za_size < ZA_SIG_CONTEXT_SIZE(vq))
>  		return -EINVAL;
>  
> -	/*
> -	 * Careful: we are about __copy_from_user() directly into
> -	 * thread.sme_state with preemption enabled, so protection is
> -	 * needed to prevent a racing context switch from writing stale
> -	 * registers back over the new data.
> -	 */
> -
> -	fpsimd_flush_task_state(current);
> -	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
> -
>  	sme_alloc(current, true);
>  	if (!current->thread.sme_state) {
>  		current->thread.svcr &= ~SVCR_ZA_MASK;
> @@ -664,16 +636,6 @@ static int restore_zt_context(struct user_ctxs *user)
>  	if (nregs != 1)
>  		return -EINVAL;
>  
> -	/*
> -	 * Careful: we are about __copy_from_user() directly into
> -	 * thread.zt_state with preemption enabled, so protection is
> -	 * needed to prevent a racing context switch from writing stale
> -	 * registers back over the new data.
> -	 */
> -
> -	fpsimd_flush_task_state(current);
> -	/* From now, fpsimd_thread_switch() won't touch ZT in thread state */
> -
>  	err = __copy_from_user(thread_zt_state(&current->thread),
>  			       (char __user const *)user->zt +
>  					ZT_SIG_REGS_OFFSET,
> @@ -1028,6 +990,18 @@ static int restore_sigframe(struct pt_regs *regs,
>  	if (err == 0)
>  		err = parse_user_sigframe(&user, sf);
>  
> +	/*
> +	 * Careful: we are about __copy_from_user() directly into

Nit: we are about _to_ __copy_from_user()...

(Looks like this was my typo in the original ancestor of this comment
block!  Might as well fix it now, since the comments are being unified
here.)

> +	 * thread floating point state with preemption enabled, so
> +	 * protection is needed to prevent a racing context switch
> +	 * from writing stale registers back over the new data. Mark
> +	 * the register floating point state as invalid and unbind the
> +	 * task from the CPU to force a reload before we return to
> +	 * userspace. fpsimd_flush_task_state() has a check for FP
> +	 * support.
> +	 */

Maybe add a comment in fpsimd_flush_task_state() about why the
system_supports_fpsimd() check is important?  It's not obvious there
why we should ever be calling that function on non-FPSIMD systems.

> +	fpsimd_flush_task_state(current);
> +

This seems a definite improvement.  We know we're about to load
something over the task's FPSIMD / vector state, even if we don't know
exactly what.

But would it be a good idea to stick a
WARN_ON(!test_thread_flag(TIF_FOREIGN_FPSTATE)) at the start of the
functions that rely on this?

This code isn't super hot-path.

[...]

Cheers
---Dave

Re: [PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Tue, Dec 03, 2024 at 03:32:22PM +0000, Dave Martin wrote:
> On Tue, Dec 03, 2024 at 12:45:53PM +0000, Mark Brown wrote:

> > @@ -1460,6 +1460,8 @@ void do_sme_acc(unsigned long esr, struct pt_regs *regs)
> >  		sme_set_vq(vq_minus_one);
> >  
> >  		fpsimd_bind_task_to_cpu();
> > +	} else {
> > +		fpsimd_flush_task_state(current);

> TIF_FOREIGN_FPSTATE is (or was) a cache of the task<->CPU binding that
> you're clobbering here.

> So, this fpsimd_flush_task_state() should have no effect unless
> TIF_FOREIGN_FPSTATE is already wrong?  I'm wondering if the apparent
> need for this means that there is an undiagnosed bug elsewhere.

> (My understanding is based on FPSIMD/SVE; I'm less familiar with the
> SME changes, so I may be missing something important here.)

It's to ensure that the last recorded CPU for the current task is
invalid so that if the state was loaded on another CPU and we switch
back to that CPU we reload the state from memory, we need to at least
trigger configuration of the SME VL.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Tue, Dec 03, 2024 at 03:33:18PM +0000, Dave Martin wrote:
> On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:

> > +	get_cpu_fpsimd_context();

> > +		if (current->thread.svcr & SVCR_SM_MASK) {
> > +			memset(&current->thread.uw.fpsimd_state.vregs, 0,
> > +			       sizeof(current->thread.uw.fpsimd_state.vregs));

> Do we need to hold the CPU fpsimd context across this memset?

> IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
> dumping of the regs into thread_struct (from current's PoV), but never
> spontaneously set again.  So ... -> [*]

Yes, we could drop the lock here.  OTOH this is very simple and easy to
understand.

> > +		/* Ensure any copies on other CPUs aren't reused */
> > +		fpsimd_flush_task_state(current);

> (This is very similar to fpsimd_flush_thread(); can they be unified?)

I have a half finished series to replace the whole setup around
accessing the state with get/put operations for working on the state
which should remove all these functions.  The pile of similarly and
confusingly named operations we have for working on the state is one of
the major sources of issues with this code, even when actively working
on the code it's hard to remember exactly which operation does what
never mind the rules for which is needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On Tue, Dec 03, 2024 at 03:34:01PM +0000, Dave Martin wrote:
> On Tue, Dec 03, 2024 at 12:45:56PM +0000, Mark Brown wrote:
> > When restoring the SVE and SME specific floating point register states we
> > flush the task floating point state, marking the hardware state as stale so
> > that preemption does not result in us saving register state from the signal

Now I think about it again this should probably be dropped from the
series, or at least ordered after the reenablement.

> > +	 * thread floating point state with preemption enabled, so
> > +	 * protection is needed to prevent a racing context switch
> > +	 * from writing stale registers back over the new data. Mark
> > +	 * the register floating point state as invalid and unbind the
> > +	 * task from the CPU to force a reload before we return to
> > +	 * userspace. fpsimd_flush_task_state() has a check for FP
> > +	 * support.
> > +	 */

> Maybe add a comment in fpsimd_flush_task_state() about why the
> system_supports_fpsimd() check is important?  It's not obvious there
> why we should ever be calling that function on non-FPSIMD systems.

There already is a comment in there about it?

> But would it be a good idea to stick a
> WARN_ON(!test_thread_flag(TIF_FOREIGN_FPSTATE)) at the start of the
> functions that rely on this?

As I mentioned in reply to one of your other messages I want to just gut
the whole API here and replace it with get/put functions for the state
which would include the get/put functions making sure they're paired
with each other.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Dave Martin]

On Tue, Dec 03, 2024 at 04:00:45PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:32:22PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:53PM +0000, Mark Brown wrote:
> 
> > > @@ -1460,6 +1460,8 @@ void do_sme_acc(unsigned long esr, struct pt_regs *regs)
> > >  		sme_set_vq(vq_minus_one);
> > >  
> > >  		fpsimd_bind_task_to_cpu();
> > > +	} else {
> > > +		fpsimd_flush_task_state(current);
> 
> > TIF_FOREIGN_FPSTATE is (or was) a cache of the task<->CPU binding that
> > you're clobbering here.
> 
> > So, this fpsimd_flush_task_state() should have no effect unless
> > TIF_FOREIGN_FPSTATE is already wrong?  I'm wondering if the apparent
> > need for this means that there is an undiagnosed bug elsewhere.
> 
> > (My understanding is based on FPSIMD/SVE; I'm less familiar with the
> > SME changes, so I may be missing something important here.)
> 
> It's to ensure that the last recorded CPU for the current task is
> invalid so that if the state was loaded on another CPU and we switch
> back to that CPU we reload the state from memory, we need to at least
> trigger configuration of the SME VL.

OK, so the logic here is something like:

Disregarding SME, the FPSIMD/SVE regs are up to date, which is fine
because SME is trapped.

When we take the SME trap, we suddenly have some work to do in order to
make sure that the SME-specific parts of the register state are up to
date, so we need to mark the state as stale before setting TIF_SME and
returning.

fpsimd_flush_task_state() means that we do the necessary work when re-
entering userspace, but is there a problem with simply marking all the
FPSIMD/vector state as stale?  If FPSR or FPCR is dirty for example, it
now looks like they won't get written back to thread struct if there is
a context switch before current re-enters userspace?

Maybe the other flags distinguish these cases -- I haven't fully got my
head around it.


(Actually, the ARM ARM says (IMHTLZ) that toggling PSTATE.SM by any
means causes FPSR to become 0x800009f.  I'm not sure where that fits in
-- do we handle that anywhere?  I guess the "soft" SM toggling via
ptrace, signal delivery or maybe exec, ought to set this?  Not sure how
that interacts with the expected behaviour of the fenv(3) API...  Hmm.
I see no corresponding statement about FPCR.)

Cheers
---Dave

Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

[Dave Martin]

Hi,

On Tue, Dec 03, 2024 at 04:53:11PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:34:01PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:56PM +0000, Mark Brown wrote:
> > > When restoring the SVE and SME specific floating point register states we
> > > flush the task floating point state, marking the hardware state as stale so
> > > that preemption does not result in us saving register state from the signal
> 
> Now I think about it again this should probably be dropped from the
> series, or at least ordered after the reenablement.
> 
> > > +	 * thread floating point state with preemption enabled, so
> > > +	 * protection is needed to prevent a racing context switch
> > > +	 * from writing stale registers back over the new data. Mark
> > > +	 * the register floating point state as invalid and unbind the
> > > +	 * task from the CPU to force a reload before we return to
> > > +	 * userspace. fpsimd_flush_task_state() has a check for FP
> > > +	 * support.
> > > +	 */
> 
> > Maybe add a comment in fpsimd_flush_task_state() about why the
> > system_supports_fpsimd() check is important?  It's not obvious there
> > why we should ever be calling that function on non-FPSIMD systems.
> 
> There already is a comment in there about it?

There's a comment, but it's not clear that calling that function is
considered correct / useful if there is no FPSIMD.

Not a big deal, anyhow.

> > But would it be a good idea to stick a
> > WARN_ON(!test_thread_flag(TIF_FOREIGN_FPSTATE)) at the start of the
> > functions that rely on this?
> 
> As I mentioned in reply to one of your other messages I want to just gut
> the whole API here and replace it with get/put functions for the state
> which would include the get/put functions making sure they're paired
> with each other.

No argument from me on that, but it would be good to have a way to
check that functions that expect to be called with the FP context held,
actually are (similar to lockdep_assert_held() etc.)

If the number of affected functions is low, I guess comments may be
enough, though.

> 
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Ack, but opinions can differ about what context is unneeded.

I'll try to keep the noise down on these threads.

Cheers
---Dave

Re: [PATCH 5/6] arm64/signal: Avoid corruption of SME state when entering signal handler

[Dave Martin]

Hi,

On Tue, Dec 03, 2024 at 04:12:33PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:33:18PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
> 
> > > +	get_cpu_fpsimd_context();
> 
> > > +		if (current->thread.svcr & SVCR_SM_MASK) {
> > > +			memset(&current->thread.uw.fpsimd_state.vregs, 0,
> > > +			       sizeof(current->thread.uw.fpsimd_state.vregs));
> 
> > Do we need to hold the CPU fpsimd context across this memset?
> 
> > IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
> > dumping of the regs into thread_struct (from current's PoV), but never
> > spontaneously set again.  So ... -> [*]
> 
> Yes, we could drop the lock here.  OTOH this is very simple and easy to
> understand.

Ack; it works either way.

Since this is a Fixes: patch, it may be better to keep it simple.

> 
> > > +		/* Ensure any copies on other CPUs aren't reused */
> > > +		fpsimd_flush_task_state(current);
> 
> > (This is very similar to fpsimd_flush_thread(); can they be unified?)
> 
> I have a half finished series to replace the whole setup around
> accessing the state with get/put operations for working on the state
> which should remove all these functions.  The pile of similarly and
> confusingly named operations we have for working on the state is one of
> the major sources of issues with this code, even when actively working
> on the code it's hard to remember exactly which operation does what
> never mind the rules for which is needed.

Sure, something like that would definitely help.

Cheers
---Dave

Re: [PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

On Tue, Dec 03, 2024 at 05:00:08PM +0000, Dave Martin wrote:
> On Tue, Dec 03, 2024 at 04:00:45PM +0000, Mark Brown wrote:

> > It's to ensure that the last recorded CPU for the current task is
> > invalid so that if the state was loaded on another CPU and we switch
> > back to that CPU we reload the state from memory, we need to at least
> > trigger configuration of the SME VL.

> OK, so the logic here is something like:

> Disregarding SME, the FPSIMD/SVE regs are up to date, which is fine
> because SME is trapped.

> When we take the SME trap, we suddenly have some work to do in order to
> make sure that the SME-specific parts of the register state are up to
> date, so we need to mark the state as stale before setting TIF_SME and
> returning.

We know that the only bit of register state which is not up to date at
this point is the SME vector length, we don't configure that for tasks
that do not have SME.  SVCR is always configured since we have to exit
streaming mode for FPSIMD and SVE to work properly so we know it's
already 0, all the other SME specific state is gated by controls in
SVCR.

> fpsimd_flush_task_state() means that we do the necessary work when re-
> entering userspace, but is there a problem with simply marking all the
> FPSIMD/vector state as stale?  If FPSR or FPCR is dirty for example, it
> now looks like they won't get written back to thread struct if there is
> a context switch before current re-enters userspace?

> Maybe the other flags distinguish these cases -- I haven't fully got my
> head around it.

We are doing fpsimd_flush_task_state() in the TIF_FOREIGN_FPSTATE case
so we know there is no dirty state in the registers.

> (Actually, the ARM ARM says (IMHTLZ) that toggling PSTATE.SM by any
> means causes FPSR to become 0x800009f.  I'm not sure where that fits in
> -- do we handle that anywhere?  I guess the "soft" SM toggling via

Urgh, not seen that one - that needs handling in the signal entry path
and ptrace.  That will have been defined while the feature was being
implemented.  It's not relevant here though since we are in the SME
access trap, we might be trapping due to a SMSTART or equivalent
operation but that SMSTART has not yet run at the point where we return
to userspace.

> ptrace, signal delivery or maybe exec, ought to set this?  Not sure how
> that interacts with the expected behaviour of the fenv(3) API...  Hmm.
> I see no corresponding statement about FPCR.)

Fun.  I'm not sure how the ABI is defined there by libc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/6] arm64/sme: Flush foreign register state in do_sme_acc()

[Dave Martin]

On Tue, Dec 03, 2024 at 05:24:39PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 05:00:08PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 04:00:45PM +0000, Mark Brown wrote:

[...]

> We know that the only bit of register state which is not up to date at
> this point is the SME vector length, we don't configure that for tasks
> that do not have SME.  SVCR is always configured since we have to exit
> streaming mode for FPSIMD and SVE to work properly so we know it's
> already 0, all the other SME specific state is gated by controls in
> SVCR.
> 
> > fpsimd_flush_task_state() means that we do the necessary work when re-
> > entering userspace, but is there a problem with simply marking all the
> > FPSIMD/vector state as stale?  If FPSR or FPCR is dirty for example, it
> > now looks like they won't get written back to thread struct if there is
> > a context switch before current re-enters userspace?
> 
> > Maybe the other flags distinguish these cases -- I haven't fully got my
> > head around it.
> 
> We are doing fpsimd_flush_task_state() in the TIF_FOREIGN_FPSTATE case
> so we know there is no dirty state in the registers.

Ah, that wasn't obvious from the diff context, but you're right.

I was confused by the fpsimd_bind_task_to_cpu() call; I forgot that
there are reasons to call this even when TIF_FOREIGN_FPSTATE is clear.
Perhaps it would be worth splitting some of those uses up, but it would
need some thinking about.  Doesn't really belong in this series anyway.

> > (Actually, the ARM ARM says (IMHTLZ) that toggling PSTATE.SM by any
> > means causes FPSR to become 0x800009f.  I'm not sure where that fits in
> > -- do we handle that anywhere?  I guess the "soft" SM toggling via
> 
> Urgh, not seen that one - that needs handling in the signal entry path
> and ptrace.  That will have been defined while the feature was being
> implemented.  It's not relevant here though since we are in the SME
> access trap, we might be trapping due to a SMSTART or equivalent
> operation but that SMSTART has not yet run at the point where we return
> to userspace.
> 
> > ptrace, signal delivery or maybe exec, ought to set this?  Not sure how
> > that interacts with the expected behaviour of the fenv(3) API...  Hmm.
> > I see no corresponding statement about FPCR.)
> 
> Fun.  I'm not sure how the ABI is defined there by libc.

I guess this should be left as-is, for now.  There's an argument for
sanitising FPCR/FPSR on signal delivery, but neither signal(7) nor
fenv(3) give any clue about the expected behaviour...

For ptrace, the user has the opportunity to specify exactly what they
want to happen to all the registers, so I suppose it's best to stick to
the current model and require the tracer to specify all changes
explicitly rather than add new magic ptrace behaviour.

Not relevant for this series, in any case.

Cheers
---Dave