[PATCH 00/20] arm64: FPSIMD/SVE/SME fixes + re-eanble SME

[PATCH 00/20] arm64: FPSIMD/SVE/SME fixes + re-eanble SME

[Mark Rutland]

Hi all,

These patches fix a number of problems in the FPSIMD/SVE/SME code, and
finally re-enable SME support.

There are a few (mostly minor) ABI changes detailed in the patches.
Notably the clone() ABI is changed due to an incompatiblity with the
AAPCS64 ZA lazy saving scheme and the way this has been deployed in
userspace. The full details of that are in patch 13.

The series is based on the for-next/sme-fixes branch from the arm64
tree, and I've pushed the series to the 'arm64/fpsimd/sme-fixes' branch
of my kernel.org repo, also tagged as 'arm64-fpsimd-fixes-20250506'.

That repo can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git
  https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/

Mark.

Mark Rutland (20):
  kselftest/arm64: fp-ptrace: Fix expected FPMR value when PSTATE.SM is
    changed
  arm64/fpsimd: Do not discard modified SVE state
  arm64/fpsimd: signal: Clear PSTATE.SM when restoring FPSIMD frame only
  arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state
  arm64/fpsimd: ptrace: Consistently handle partial writes to
    NT_ARM_(S)SVE
  arm64/fpsimd: Clarify sve_sync_*() functions
  arm64/fpsimd: Factor out {sve,sme}_state_size() helpers
  arm64/fpsimd: Add task_smstop_sm()
  arm64/fpsimd: signal: Use SMSTOP behaviour in setup_return()
  arm64/fpsimd: Remove redundant task->mm check
  arm64/fpsimd: Consistently preserve FPSIMD state during clone()
  arm64/fpsimd: Clear PSTATE.SM during clone()
  arm64/fpsimd: Make clone() compatible with ZA lazy saving
  arm64/fpsimd: ptrace/prctl: Ensure VL changes do not resurrect stale
    data
  arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid
    state
  arm64/fpsimd: ptrace: Save task state before generating SVE header
  arm64/fpsimd: ptrace: Do not present register data for inactive mode
  arm64/fpsimd: ptrace: Mandate SVE payload for streaming-mode state
  arm64/fpsimd: ptrace: Gracefully handle errors
  arm64/fpsimd: Allow CONFIG_ARM64_SME to be selected

 Documentation/arch/arm64/sme.rst             |   6 +-
 arch/arm64/Kconfig                           |   1 -
 arch/arm64/include/asm/fpsimd.h              |  61 +++--
 arch/arm64/kernel/entry-common.c             |  46 +++-
 arch/arm64/kernel/fpsimd.c                   | 231 +++++++++----------
 arch/arm64/kernel/process.c                  | 112 +++++----
 arch/arm64/kernel/ptrace.c                   | 137 ++++++-----
 arch/arm64/kernel/signal.c                   |  36 ++-
 tools/testing/selftests/arm64/abi/tpidr2.c   |  14 +-
 tools/testing/selftests/arm64/fp/fp-ptrace.c |  62 +++--
 10 files changed, 392 insertions(+), 314 deletions(-)

-- 
2.30.2

[PATCH 01/20] kselftest/arm64: fp-ptrace: Fix expected FPMR value when PSTATE.SM is changed

[Mark Rutland]

The fp-ptrace test suite expects that FPMR is set to zero when PSTATE.SM
is changed via ptrace, but ptrace has never altered FPMR in this way,
and the test logic erroneously relies upon (and has concealed) a bug
where task_fpsimd_load() would unexpectedly and non-deterministically
clobber FPMR.

Using ptrace, FPMR can only be altered by writing to the NT_ARM_FPMR
regset. The value of PSTATE.SM can be altered by writing to the
NT_ARM_SVE or NT_ARM_SSVE regsets, and/or by changing the SME vector
length (when writing to the NT_ARM_SVE, NT_ARM_SSVE, or NT_ARM_ZA
regsets), but none of these writes will change the value of FPMR.

The task_fpsimd_load() bug was introduced with the initial FPMR support
in commit:

  203f2b95a882 ("arm64/fpsimd: Support FEAT_FPMR")

The incorrect FPMR test code was introduced in commit:

  7dbd26d0b22d ("kselftest/arm64: Add FPMR coverage to fp-ptrace")

Subsequently, the task_fpsimd_load() bug was fixed in commit:

  e5fa85fce08b ("arm64/fpsimd: Don't corrupt FPMR when streaming mode changes")

... whereupon the fp-ptrace FPMR tests started failing reliably, e.g.

| # # Mismatch in saved FPMR: 915058000 != 0
| # not ok 25 SVE write, SVE 64->64, SME 64/0->64/1

Fix this by changing the test to expect that FPMR is *NOT* changed when
PSTATE.SM is changed via ptrace, matching the extant behaviour.

I've chosen to update the test code rather than modifying ptrace to zero
FPMR when PSTATE.SM changes. Not zeroing FPMR is simpler overall, and
allows the NT_ARM_FPMR regset to be handled independently from other
regsets, leaving less scope for error.

Fixes: 7dbd26d0b22d ("kselftest/arm64: Add FPMR coverage to fp-ptrace")
Fixes: e5fa85fce08b ("arm64/fpsimd: Don't corrupt FPMR when streaming mode changes")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 tools/testing/selftests/arm64/fp/fp-ptrace.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/tools/testing/selftests/arm64/fp/fp-ptrace.c b/tools/testing/selftests/arm64/fp/fp-ptrace.c
index 4930e03a7b990..762048eb354ff 100644
--- a/tools/testing/selftests/arm64/fp/fp-ptrace.c
+++ b/tools/testing/selftests/arm64/fp/fp-ptrace.c
@@ -891,18 +891,11 @@ static void set_initial_values(struct test_config *config)
 {
 	int vq = __sve_vq_from_vl(vl_in(config));
 	int sme_vq = __sve_vq_from_vl(config->sme_vl_in);
-	bool sm_change;
 
 	svcr_in = config->svcr_in;
 	svcr_expected = config->svcr_expected;
 	svcr_out = 0;
 
-	if (sme_supported() &&
-	    (svcr_in & SVCR_SM) != (svcr_expected & SVCR_SM))
-		sm_change = true;
-	else
-		sm_change = false;
-
 	fill_random(&v_in, sizeof(v_in));
 	memcpy(v_expected, v_in, sizeof(v_in));
 	memset(v_out, 0, sizeof(v_out));
@@ -953,12 +946,7 @@ static void set_initial_values(struct test_config *config)
 	if (fpmr_supported()) {
 		fill_random(&fpmr_in, sizeof(fpmr_in));
 		fpmr_in &= FPMR_SAFE_BITS;
-
-		/* Entering or exiting streaming mode clears FPMR */
-		if (sm_change)
-			fpmr_expected = 0;
-		else
-			fpmr_expected = fpmr_in;
+		fpmr_expected = fpmr_in;
 	} else {
 		fpmr_in = 0;
 		fpmr_expected = 0;
-- 
2.30.2

[PATCH 02/20] arm64/fpsimd: Do not discard modified SVE state

[Mark Rutland]

Historically SVE state was discarded deterministically early in the
syscall entry path, before ptrace is notified of syscall entry. This
permitted ptrace to modify SVE state before and after the "real" syscall
logic was executed, with the modified state being retained.

This behaviour was changed by commit:

  8c845e2731041f0f ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")

That commit was intended to speed up workloads that used SVE by
opportunistically leaving SVE enabled when returning from a syscall.
The syscall entry logic was modified to truncate the SVE state without
disabling userspace access to SVE, and fpsimd_save_user_state() was
modified to discard userspace SVE state whenever
in_syscall(current_pt_regs()) is true, i.e. when
current_pt_regs()->syscallno != NO_SYSCALL.

Leaving SVE enabled opportunistically resulted in a couple of changes to
userspace visible behaviour which weren't described at the time, but are
logical consequences of opportunistically leaving SVE enabled:

* Signal handlers can observe the type of saved state in the signal's
  sve_context record. When the kernel only tracks FPSIMD state, the 'vq'
  field is 0 and there is no space allocated for register contents. When
  the kernel tracks SVE state, the 'vq' field is non-zero and the
  register contents are saved into the record.

  As a result of the above commit, 'vq' (and the presence of SVE
  register state) is non-deterministically zero or non-zero for a period
  of time after a syscall. The effective register state is still
  deterministic.

  Hopefully no-one relies on this being deterministic. In general,
  handlers for asynchronous events cannot expect a deterministic state.

* Similarly to signal handlers, ptrace requests can observe the type of
  saved state in the NT_ARM_SVE and NT_ARM_SSVE regsets, as this is
  exposed in the header flags. As a result of the above commit, this is
  now in a non-deterministic state after a syscall. The effective
  register state is still deterministic.

  Hopefully no-one relies on this being deterministic. In general,
  debuggers would have to handle this changing at arbitrary points
  during program flow.

Discarding the SVE state within fpsimd_save_user_state() resulted in
other changes to userspace visible behaviour which are not desirable:

* A ptrace tracer can modify (or create) a tracee's SVE state at syscall
  entry or syscall exit. As a result of the above commit, the tracee's
  SVE state can be discarded non-deterministically after modification,
  rather than being retained as it previously was.

  Note that for co-operative tracer/tracee pairs, the tracer may
  (re)initialise the tracee's state arbitrarily after the tracee sends
  itself an initial SIGSTOP via a syscall, so this affects realistic
  design patterns.

* The current_pt_regs()->syscallno field can be modified via ptrace, and
  can be altered even when the tracee is not really in a syscall,
  causing non-deterministic discarding to occur in situations where this
  was not previously possible.

Further, using current_pt_regs()->syscallno in this way is unsound:

* There are data races between readers and writers of the
  current_pt_regs()->syscallno field.

  The current_pt_regs()->syscallno field is written in interruptible
  task context using plain C accesses, and is read in irq/softirq
  context using plain C accesses. These accesses are subject to data
  races, with the usual concerns with tearing, etc.

* Writes to current_pt_regs()->syscallno are subject to compiler
  reordering.

  As current_pt_regs()->syscallno is written with plain C accesses,
  the compiler is free to move those writes arbitrarily relative to
  anything which doesn't access the same memory location.

  In theory this could break signal return, where prior to restoring the
  SVE state, restore_sigframe() calls forget_syscall(). If the write
  were hoisted after restore of some SVE state, that state could be
  discarded unexpectedly.

  In practice that reordering cannot happen in the absence of LTO (as
  cross compilation-unit function calls happen prevent this reordering),
  and that reordering appears to be unlikely in the presence of LTO.

Additionally, since commit:

  f130ac0ae4412dbe ("arm64: syscall: unmask DAIF earlier for SVCs")

... DAIF is unmasked before el0_svc_common() sets regs->syscallno to the
real syscall number. Consequently state may be saved in SVE format prior
to this point.

Considering all of the above, current_pt_regs()->syscallno should not be
used to infer whether the SVE state can be discarded. Luckily we can
instead use cpu_fp_state::to_save to track when it is safe to discard
the SVE state:

* At syscall entry, after the live SVE register state is truncated, set
  cpu_fp_state::to_save to FP_STATE_FPSIMD to indicate that only the
  FPSIMD portion is live and needs to be saved.

* At syscall exit, once the task's state is gauranteed to be live, set
  cpu_fp_state::to_save to FP_STATE_CURRENT to indicate that TIF_SVE
  must be considered to determine which state needs to be saved.

* Whenever state is modified, it must be saved+flushed prior to
  manipulation. The state will be truncated if necessary when it is
  saved, and reloading the state will set fp_state::to_save to
  FP_STATE_CURRENT, preventing subsequent discarding.

This permits SVE state to be discarded *only* when it is known to have
been truncated (and the non-FPSIMD portions must be zero), and ensures
that SVE state is retained after it is explicitly modified.

For backporting, note that this fix depends on the following commits:

* b2482807fbd4 ("arm64/sme: Optimise SME exit on syscall entry")
* f130ac0ae441 ("arm64: syscall: unmask DAIF earlier for SVCs")
* 929fa99b1215 ("arm64/fpsimd: signal: Always save+flush state early")

Fixes: 8c845e273104 ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")
Fixes: f130ac0ae441 ("arm64: syscall: unmask DAIF earlier for SVCs")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h  |  3 +++
 arch/arm64/kernel/entry-common.c | 46 ++++++++++++++++++++++++--------
 arch/arm64/kernel/fpsimd.c       | 15 ++++++-----
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index b7adb2c72204b..11b9c6a5e0377 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -6,6 +6,7 @@
 #define __ASM_FP_H
 
 #include <asm/errno.h>
+#include <asm/percpu.h>
 #include <asm/ptrace.h>
 #include <asm/processor.h>
 #include <asm/sigcontext.h>
@@ -92,6 +93,8 @@ struct cpu_fp_state {
 	enum fp_type to_save;
 };
 
+DECLARE_PER_CPU(struct cpu_fp_state, fpsimd_last_state);
+
 extern void fpsimd_bind_state_to_cpu(struct cpu_fp_state *fp_state);
 
 extern void fpsimd_flush_task_state(struct task_struct *target);
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index b260ddc4d3e9a..fa8a090339e2b 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -393,20 +393,16 @@ static bool cortex_a76_erratum_1463225_debug_handler(struct pt_regs *regs)
  * As per the ABI exit SME streaming mode and clear the SVE state not
  * shared with FPSIMD on syscall entry.
  */
-static inline void fp_user_discard(void)
+static inline void fpsimd_syscall_enter(void)
 {
-	/*
-	 * If SME is active then exit streaming mode.  If ZA is active
-	 * then flush the SVE registers but leave userspace access to
-	 * both SVE and SME enabled, otherwise disable SME for the
-	 * task and fall through to disabling SVE too.  This means
-	 * that after a syscall we never have any streaming mode
-	 * register state to track, if this changes the KVM code will
-	 * need updating.
-	 */
+	/* Ensure PSTATE.SM is clear, but leave PSTATE.ZA as-is. */
 	if (system_supports_sme())
 		sme_smstop_sm();
 
+	/*
+	 * The CPU is not in streaming mode. If non-streaming SVE is not
+	 * supported, there is no SVE state that needs to be discarded.
+	 */
 	if (!system_supports_sve())
 		return;
 
@@ -416,6 +412,33 @@ static inline void fp_user_discard(void)
 		sve_vq_minus_one = sve_vq_from_vl(task_get_sve_vl(current)) - 1;
 		sve_flush_live(true, sve_vq_minus_one);
 	}
+
+	/*
+	 * Any live non-FPSIMD SVE state has been zeroed. Allow
+	 * fpsimd_save_user_state() to lazily discard SVE state until either
+	 * the live state is unbound or fpsimd_syscall_exit() is called.
+	 */
+	__this_cpu_write(fpsimd_last_state.to_save, FP_STATE_FPSIMD);
+}
+
+static __always_inline void fpsimd_syscall_exit(void)
+{
+	if (!system_supports_sve())
+		return;
+
+	/*
+	 * The current task's user FPSIMD/SVE/SME state is now bound to this
+	 * CPU. The fpsimd_last_state.to_save value is either:
+	 *
+	 * - FP_STATE_FPSIMD, if the state has not been reloaded on this CPU
+	 *   since fpsimd_syscall_enter().
+	 *
+	 * - FP_STATE_CURRENT, if the state has been reloaded on this CPU at
+	 *   any point.
+	 *
+	 * Reset this to FP_STATE_CURRENT to stop lazy discarding.
+	 */
+	__this_cpu_write(fpsimd_last_state.to_save, FP_STATE_CURRENT);
 }
 
 UNHANDLED(el1t, 64, sync)
@@ -739,10 +762,11 @@ static void noinstr el0_svc(struct pt_regs *regs)
 {
 	enter_from_user_mode(regs);
 	cortex_a76_erratum_1463225_svc_handler();
-	fp_user_discard();
+	fpsimd_syscall_enter();
 	local_daif_restore(DAIF_PROCCTX);
 	do_el0_svc(regs);
 	exit_to_user_mode(regs);
+	fpsimd_syscall_exit();
 }
 
 static void noinstr el0_fpac(struct pt_regs *regs, unsigned long esr)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 422b9d43b1e64..9f1890d692dfc 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -119,7 +119,7 @@
  *   whatever is in the FPSIMD registers is not saved to memory, but discarded.
  */
 
-static DEFINE_PER_CPU(struct cpu_fp_state, fpsimd_last_state);
+DEFINE_PER_CPU(struct cpu_fp_state, fpsimd_last_state);
 
 __ro_after_init struct vl_info vl_info[ARM64_VEC_MAX] = {
 #ifdef CONFIG_ARM64_SVE
@@ -451,12 +451,15 @@ static void fpsimd_save_user_state(void)
 		*(last->fpmr) = read_sysreg_s(SYS_FPMR);
 
 	/*
-	 * If a task is in a syscall the ABI allows us to only
-	 * preserve the state shared with FPSIMD so don't bother
-	 * saving the full SVE state in that case.
+	 * Save SVE state if it is live.
+	 *
+	 * The syscall ABI discards live SVE state at syscall entry. When
+	 * entering a syscall, fpsimd_syscall_enter() sets to_save to
+	 * FP_STATE_FPSIMD to allow the SVE state to be lazily discarded until
+	 * either new SVE state is loaded+bound or fpsimd_syscall_exit() is
+	 * called prior to a return to userspace.
 	 */
-	if ((last->to_save == FP_STATE_CURRENT && test_thread_flag(TIF_SVE) &&
-	     !in_syscall(current_pt_regs())) ||
+	if ((last->to_save == FP_STATE_CURRENT && test_thread_flag(TIF_SVE)) ||
 	    last->to_save == FP_STATE_SVE) {
 		save_sve_regs = true;
 		save_ffr = true;
-- 
2.30.2

[PATCH 03/20] arm64/fpsimd: signal: Clear PSTATE.SM when restoring FPSIMD frame only

[Mark Rutland]

On systems with SVE and/or SME, the kernel will always create SVE and
FPSIMD signal frames when delivering a signal, but a user can manipulate
signal frames such that a signal return only observes an FPSIMD signal
frame. When this happens, restore_fpsimd_context() will restore state
such that fp_type==FP_STATE_FPSIMD, but will leave PSTATE.SM as-is.

It is possible for a user to set PSTATE.SM between syscall entry and
execution of the sigreturn logic (e.g. via ptrace), and consequently the
sigreturn may result in the task having PSTATE.SM==1 and
fp_type==FP_STATE_FPSIMD.

For various reasons it is not legitimate for a task to be in a state
where PSTATE.SM==1 and fp_type==FP_STATE_FPSIMD. Portions of the user
ABI are written with the requirement that streaming SVE state is always
presented in SVE format rather than FPSIMD format, and as there is no
mechanism to permit access to only the FPSIMD subset of streaming SVE
state, streaming SVE state must always be saved and restored in SVE
format.

Fix restore_fpsimd_context() to clear PSTATE.SM when restoring an FPSIMD
signal frame without an SVE signal frame. This matches the current
behaviour when an SVE signal frame is present, but the SVE signal frame
has no register payload (e.g. as is the case on SME-only systems which
lack SVE).

This change should have no effect for applications which do not alter
signal frames (i.e. almost all applications). I do not expect
non-{malicious,buggy} applications to hide the SVE signal frame, but
I've chosen to clear PSTATE.SM rather than mandating the presence of an
SVE signal frame in case there is some legacy (non-SME) usage that I am
not currently aware of.

For context, the SME handling was originally introduced in commit:

  85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")

... and subsequently updated/fixed to handle SME-only systems in commits:

  7dde62f0687c ("arm64/signal: Always accept SVE signal frames on SME only systems")
  f26cd7372160 ("arm64/signal: Always allocate SVE signal frames on SME only systems")

Fixes: 85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 48d3c0129dade..fdce1b856f498 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -280,6 +280,7 @@ static int restore_fpsimd_context(struct user_ctxs *user)
 	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
 
 	clear_thread_flag(TIF_SVE);
+	current->thread.svcr &= ~SVCR_SM_MASK;
 	current->thread.fp_type = FP_STATE_FPSIMD;
 
 	/* load the hardware registers from the fpsimd_state structure */
-- 
2.30.2

Re: [PATCH 03/20] arm64/fpsimd: signal: Clear PSTATE.SM when restoring FPSIMD frame only

[Will Deacon]

On Tue, May 06, 2025 at 04:25:06PM +0100, Mark Rutland wrote:
> On systems with SVE and/or SME, the kernel will always create SVE and
> FPSIMD signal frames when delivering a signal, but a user can manipulate
> signal frames such that a signal return only observes an FPSIMD signal
> frame. When this happens, restore_fpsimd_context() will restore state
> such that fp_type==FP_STATE_FPSIMD, but will leave PSTATE.SM as-is.
> 
> It is possible for a user to set PSTATE.SM between syscall entry and
> execution of the sigreturn logic (e.g. via ptrace), and consequently the
> sigreturn may result in the task having PSTATE.SM==1 and
> fp_type==FP_STATE_FPSIMD.
> 
> For various reasons it is not legitimate for a task to be in a state
> where PSTATE.SM==1 and fp_type==FP_STATE_FPSIMD. Portions of the user
> ABI are written with the requirement that streaming SVE state is always
> presented in SVE format rather than FPSIMD format, and as there is no
> mechanism to permit access to only the FPSIMD subset of streaming SVE
> state, streaming SVE state must always be saved and restored in SVE
> format.
> 
> Fix restore_fpsimd_context() to clear PSTATE.SM when restoring an FPSIMD
> signal frame without an SVE signal frame. This matches the current
> behaviour when an SVE signal frame is present, but the SVE signal frame
> has no register payload (e.g. as is the case on SME-only systems which
> lack SVE).
> 
> This change should have no effect for applications which do not alter
> signal frames (i.e. almost all applications). I do not expect
> non-{malicious,buggy} applications to hide the SVE signal frame, but
> I've chosen to clear PSTATE.SM rather than mandating the presence of an
> SVE signal frame in case there is some legacy (non-SME) usage that I am
> not currently aware of.
> 
> For context, the SME handling was originally introduced in commit:
> 
>   85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
> 
> ... and subsequently updated/fixed to handle SME-only systems in commits:
> 
>   7dde62f0687c ("arm64/signal: Always accept SVE signal frames on SME only systems")
>   f26cd7372160 ("arm64/signal: Always allocate SVE signal frames on SME only systems")
> 
> Fixes: 85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/signal.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 48d3c0129dade..fdce1b856f498 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -280,6 +280,7 @@ static int restore_fpsimd_context(struct user_ctxs *user)
>  	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
>  
>  	clear_thread_flag(TIF_SVE);
> +	current->thread.svcr &= ~SVCR_SM_MASK;
>  	current->thread.fp_type = FP_STATE_FPSIMD;

Hmm, I think we're preemptible here so do we need some compiler barriers
to make sure that the context-switching code doesn't see these fields in
an inconsistent state?

Will

Re: [PATCH 03/20] arm64/fpsimd: signal: Clear PSTATE.SM when restoring FPSIMD frame only

[Mark Rutland]

On Wed, May 07, 2025 at 01:46:45PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:06PM +0100, Mark Rutland wrote:
> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index 48d3c0129dade..fdce1b856f498 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -280,6 +280,7 @@ static int restore_fpsimd_context(struct user_ctxs *user)
> >  	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
> >  
> >  	clear_thread_flag(TIF_SVE);
> > +	current->thread.svcr &= ~SVCR_SM_MASK;
> >  	current->thread.fp_type = FP_STATE_FPSIMD;
> 
> Hmm, I think we're preemptible here so do we need some compiler barriers
> to make sure that the context-switching code doesn't see these fields in
> an inconsistent state?

We avoid that problem by ensuring that the task's FPSIMD/SVE/SME state isn't
live on any CPU for the duration of the signal code, such that it is safe to
manipulate the saved state in preemptible context. Details below.

That was handled in one of the earlier patches in the arm64 for-next/sme-fixes
branch, queded as commit:

  929fa99b1215966f ("arm64/fpsimd: signal: Always save+flush state early")

As of that commit, in setup_rt_frame() we call
fpsimd_save_and_flush_current_state(), which does:

  get_cpu_fpsimd_context();
  fpsimd_save_user_state();
  fpsimd_flush_task_state(current);
  put_cpu_fpsimd_context();

That ensures (in a premption-safe way) that the tasks state has been saved
to memory, TIF_FOREIGN_FPSTATE is set, and that
task->thread.fpsimd_cpu==NR_CPUS. Thus on context switch:

* When switching away from this task, fpsimd_thread_switch() sees
  TIF_FOREIGN_FPSTATE is set, and doesn't manipulate the task's saved state.

* When switching to this task, fpsimd_thread_switch() doesn't find any existing
  state bound on the CPU, and sets TIF_FOREIGN_FPSTATE, without manipulating
  the task's saved state.

The task's state will only be relaoded onto the CPU when returning to
userspace, via fpsimd_restore_current_state().

Note that we changed fpsimd_update_current_state() to modify the saved state
without reloading it onto the CPU.

Mark.

Re: [PATCH 03/20] arm64/fpsimd: signal: Clear PSTATE.SM when restoring FPSIMD frame only

[Will Deacon]

On Wed, May 07, 2025 at 03:01:50PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 01:46:45PM +0100, Will Deacon wrote:
> > On Tue, May 06, 2025 at 04:25:06PM +0100, Mark Rutland wrote:
> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > index 48d3c0129dade..fdce1b856f498 100644
> > > --- a/arch/arm64/kernel/signal.c
> > > +++ b/arch/arm64/kernel/signal.c
> > > @@ -280,6 +280,7 @@ static int restore_fpsimd_context(struct user_ctxs *user)
> > >  	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
> > >  
> > >  	clear_thread_flag(TIF_SVE);
> > > +	current->thread.svcr &= ~SVCR_SM_MASK;
> > >  	current->thread.fp_type = FP_STATE_FPSIMD;
> > 
> > Hmm, I think we're preemptible here so do we need some compiler barriers
> > to make sure that the context-switching code doesn't see these fields in
> > an inconsistent state?
> 
> We avoid that problem by ensuring that the task's FPSIMD/SVE/SME state isn't
> live on any CPU for the duration of the signal code, such that it is safe to
> manipulate the saved state in preemptible context. Details below.
> 
> That was handled in one of the earlier patches in the arm64 for-next/sme-fixes
> branch, queded as commit:
> 
>   929fa99b1215966f ("arm64/fpsimd: signal: Always save+flush state early")
> 
> As of that commit, in setup_rt_frame() we call
> fpsimd_save_and_flush_current_state(), which does:
> 
>   get_cpu_fpsimd_context();
>   fpsimd_save_user_state();
>   fpsimd_flush_task_state(current);
>   put_cpu_fpsimd_context();
> 
> That ensures (in a premption-safe way) that the tasks state has been saved
> to memory, TIF_FOREIGN_FPSTATE is set, and that
> task->thread.fpsimd_cpu==NR_CPUS. Thus on context switch:
> 
> * When switching away from this task, fpsimd_thread_switch() sees
>   TIF_FOREIGN_FPSTATE is set, and doesn't manipulate the task's saved state.
> 
> * When switching to this task, fpsimd_thread_switch() doesn't find any existing
>   state bound on the CPU, and sets TIF_FOREIGN_FPSTATE, without manipulating
>   the task's saved state.
> 
> The task's state will only be relaoded onto the CPU when returning to
> userspace, via fpsimd_restore_current_state().
> 
> Note that we changed fpsimd_update_current_state() to modify the saved state
> without reloading it onto the CPU.

Thanks, that makes sense!

Will

[PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Mark Rutland]

Non-streaming SVE state may be preserved without an SVE payload, in
which case the SVE context only has a header with VL==0, and all state
can be restored from the FPSIMD context. Streaming SVE state is always
preserved with an SVE payload, where the SVE context header has VL!=0,
and the SVE_SIG_FLAG_SM flag is set.

The kernel never preserves an SVE context where SVE_SIG_FLAG_SM is set
without an SVE payload. However, restore_sve_fpsimd_context() doesn't
forbid restoring such a context, and will handle this case by clearing
PSTATE.SM and restoring the FPSIMD context into non-streaming mode,
which isn't consistent with the SVE_SIG_FLAG_SM flag.

Forbid this case, and mandate an SVE payload when the SVE_SIG_FLAG_SM
flag is set. This avoids an awkward ABI quirk and reduces the risk that
later rework to this code permits configuring a task with PSTATE.SM==1
and fp_type==FP_STATE_FPSIMD.

I've marked this as a fix given that we never intended to support this
case, and we don't want anyone to start relying upon the old behaviour
once we re-enable SME.

Fixes: 85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/signal.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index fdce1b856f498..e898948a31b65 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -387,6 +387,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	unsigned int vl, vq;
 	struct user_fpsimd_state fpsimd;
 	u16 user_vl, flags;
+	bool sm;
 
 	if (user->sve_size < sizeof(*user->sve))
 		return -EINVAL;
@@ -396,7 +397,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (err)
 		return err;
 
-	if (flags & SVE_SIG_FLAG_SM) {
+	sm = flags & SVE_SIG_FLAG_SM;
+	if (sm) {
 		if (!system_supports_sme())
 			return -EINVAL;
 
@@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (user_vl != vl)
 		return -EINVAL;
 
-	if (user->sve_size == sizeof(*user->sve)) {
+	/*
+	 * Non-streaming SVE state may be preserved without an SVE payload, in
+	 * which case the SVE context only has a header with VL==0, and all
+	 * state can be restored from the FPSIMD context.
+	 *
+	 * Streaming SVE state is always preserved with an SVE payload. For
+	 * consistency and robustness, reject restoring streaming SVE state
+	 * without an SVE payload.
+	 */
+	if (!sm && user->sve_size == sizeof(*user->sve)) {
 		clear_thread_flag(TIF_SVE);
 		current->thread.svcr &= ~SVCR_SM_MASK;
 		current->thread.fp_type = FP_STATE_FPSIMD;
-- 
2.30.2

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Mark Brown]

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

On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> Non-streaming SVE state may be preserved without an SVE payload, in
> which case the SVE context only has a header with VL==0, and all state
> can be restored from the FPSIMD context. Streaming SVE state is always
> preserved with an SVE payload, where the SVE context header has VL!=0,
> and the SVE_SIG_FLAG_SM flag is set.

Reviewed-by: Mark Brown <broonie@kernel.org>

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

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Will Deacon]

On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> Non-streaming SVE state may be preserved without an SVE payload, in
> which case the SVE context only has a header with VL==0, and all state
> can be restored from the FPSIMD context. Streaming SVE state is always
> preserved with an SVE payload, where the SVE context header has VL!=0,
> and the SVE_SIG_FLAG_SM flag is set.
> 
> The kernel never preserves an SVE context where SVE_SIG_FLAG_SM is set
> without an SVE payload. However, restore_sve_fpsimd_context() doesn't
> forbid restoring such a context, and will handle this case by clearing
> PSTATE.SM and restoring the FPSIMD context into non-streaming mode,
> which isn't consistent with the SVE_SIG_FLAG_SM flag.
> 
> Forbid this case, and mandate an SVE payload when the SVE_SIG_FLAG_SM
> flag is set. This avoids an awkward ABI quirk and reduces the risk that
> later rework to this code permits configuring a task with PSTATE.SM==1
> and fp_type==FP_STATE_FPSIMD.
> 
> I've marked this as a fix given that we never intended to support this
> case, and we don't want anyone to start relying upon the old behaviour
> once we re-enable SME.
> 
> Fixes: 85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/signal.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index fdce1b856f498..e898948a31b65 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -387,6 +387,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	unsigned int vl, vq;
>  	struct user_fpsimd_state fpsimd;
>  	u16 user_vl, flags;
> +	bool sm;
>  
>  	if (user->sve_size < sizeof(*user->sve))
>  		return -EINVAL;
> @@ -396,7 +397,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	if (err)
>  		return err;
>  
> -	if (flags & SVE_SIG_FLAG_SM) {
> +	sm = flags & SVE_SIG_FLAG_SM;
> +	if (sm) {
>  		if (!system_supports_sme())
>  			return -EINVAL;
>  
> @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	if (user_vl != vl)
>  		return -EINVAL;
>  
> -	if (user->sve_size == sizeof(*user->sve)) {
> +	/*
> +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> +	 * which case the SVE context only has a header with VL==0, and all
> +	 * state can be restored from the FPSIMD context.
> +	 *
> +	 * Streaming SVE state is always preserved with an SVE payload. For
> +	 * consistency and robustness, reject restoring streaming SVE state
> +	 * without an SVE payload.
> +	 */
> +	if (!sm && user->sve_size == sizeof(*user->sve)) {
>  		clear_thread_flag(TIF_SVE);
>  		current->thread.svcr &= ~SVCR_SM_MASK;
>  		current->thread.fp_type = FP_STATE_FPSIMD;

This (along with the 'fpsimd_only:' block) is now practically identical
to restore_fpsimd_context(). I think that's a sign that we're doing the
right thing, but can we move that into a shared helper function?

Will

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Mark Rutland]

On Wed, May 07, 2025 at 01:59:16PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> > @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
> >  	if (user_vl != vl)
> >  		return -EINVAL;
> >  
> > -	if (user->sve_size == sizeof(*user->sve)) {
> > +	/*
> > +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> > +	 * which case the SVE context only has a header with VL==0, and all
> > +	 * state can be restored from the FPSIMD context.
> > +	 *
> > +	 * Streaming SVE state is always preserved with an SVE payload. For
> > +	 * consistency and robustness, reject restoring streaming SVE state
> > +	 * without an SVE payload.
> > +	 */
> > +	if (!sm && user->sve_size == sizeof(*user->sve)) {
> >  		clear_thread_flag(TIF_SVE);
> >  		current->thread.svcr &= ~SVCR_SM_MASK;
> >  		current->thread.fp_type = FP_STATE_FPSIMD;
> 
> This (along with the 'fpsimd_only:' block) is now practically identical
> to restore_fpsimd_context(). I think that's a sign that we're doing the
> right thing, but can we move that into a shared helper function?

Largely yes, but there's a subtlety.

We can call restore_fpsimd_context() directly for the block above, since
that's equivalent as of the prior patch.

The "fpsimd_only" label is misleading, and is used in two distinct
cases:

(1) When the SVE context has no register state, that's used to resrtore
    an FPSIMD-only context, and is (now) equivalent to
    restore_fpsimd_context().

(2) When the SVE context has register state, we restore the SVE state,
    then fall through to the "fpsimd_only" block to merge the the FPSIMD
    registers into the SVE registers, preserving the upper bits.

    The merging is hidden opaquely in fpsimd_update_current_state().

So we can call restore_fpsimd_context() directly for the !SVE case, and
we can factor out reading the user_fpsimd_state, but for case 2 we MUST
NOT mess with TIF_SVE, SVCR, or fp_type, and MUST merge the register
contents.

If we do that, I'd want to have restore_fpsimd_context() directly assign
to current->thread.uw.fpsimd_state, and replace
restore_sve_fpsimd_context() with a new fpsimd_merge_into_sve() helper
or similar.

If that makes sense, I can go prep tha as a follow-up?

Mark.

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Will Deacon]

On Wed, May 07, 2025 at 03:21:46PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 01:59:16PM +0100, Will Deacon wrote:
> > On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> > > @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
> > >  	if (user_vl != vl)
> > >  		return -EINVAL;
> > >  
> > > -	if (user->sve_size == sizeof(*user->sve)) {
> > > +	/*
> > > +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> > > +	 * which case the SVE context only has a header with VL==0, and all
> > > +	 * state can be restored from the FPSIMD context.
> > > +	 *
> > > +	 * Streaming SVE state is always preserved with an SVE payload. For
> > > +	 * consistency and robustness, reject restoring streaming SVE state
> > > +	 * without an SVE payload.
> > > +	 */
> > > +	if (!sm && user->sve_size == sizeof(*user->sve)) {
> > >  		clear_thread_flag(TIF_SVE);
> > >  		current->thread.svcr &= ~SVCR_SM_MASK;
> > >  		current->thread.fp_type = FP_STATE_FPSIMD;
> > 
> > This (along with the 'fpsimd_only:' block) is now practically identical
> > to restore_fpsimd_context(). I think that's a sign that we're doing the
> > right thing, but can we move that into a shared helper function?
> 
> Largely yes, but there's a subtlety.
> 
> We can call restore_fpsimd_context() directly for the block above, since
> that's equivalent as of the prior patch.
> 
> The "fpsimd_only" label is misleading, and is used in two distinct
> cases:
> 
> (1) When the SVE context has no register state, that's used to resrtore
>     an FPSIMD-only context, and is (now) equivalent to
>     restore_fpsimd_context().
> 
> (2) When the SVE context has register state, we restore the SVE state,
>     then fall through to the "fpsimd_only" block to merge the the FPSIMD
>     registers into the SVE registers, preserving the upper bits.
> 
>     The merging is hidden opaquely in fpsimd_update_current_state().
> 
> So we can call restore_fpsimd_context() directly for the !SVE case, and
> we can factor out reading the user_fpsimd_state, but for case 2 we MUST
> NOT mess with TIF_SVE, SVCR, or fp_type, and MUST merge the register
> contents.

Right, all I'm suggesting here is to remove the 'fpsimd_only:' label and
call into a restore_fpsimd_context()-like helper function in the
FPSIMD-only case. We can leave the SVE case as-is (and potentially
de-duplicate the copying later on if we want to).

Will

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Mark Rutland]

On Wed, May 07, 2025 at 03:29:39PM +0100, Will Deacon wrote:
> On Wed, May 07, 2025 at 03:21:46PM +0100, Mark Rutland wrote:
> > On Wed, May 07, 2025 at 01:59:16PM +0100, Will Deacon wrote:
> > > On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> > > > @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
> > > >  	if (user_vl != vl)
> > > >  		return -EINVAL;
> > > >  
> > > > -	if (user->sve_size == sizeof(*user->sve)) {
> > > > +	/*
> > > > +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> > > > +	 * which case the SVE context only has a header with VL==0, and all
> > > > +	 * state can be restored from the FPSIMD context.
> > > > +	 *
> > > > +	 * Streaming SVE state is always preserved with an SVE payload. For
> > > > +	 * consistency and robustness, reject restoring streaming SVE state
> > > > +	 * without an SVE payload.
> > > > +	 */
> > > > +	if (!sm && user->sve_size == sizeof(*user->sve)) {
> > > >  		clear_thread_flag(TIF_SVE);
> > > >  		current->thread.svcr &= ~SVCR_SM_MASK;
> > > >  		current->thread.fp_type = FP_STATE_FPSIMD;
> > > 
> > > This (along with the 'fpsimd_only:' block) is now practically identical
> > > to restore_fpsimd_context(). I think that's a sign that we're doing the
> > > right thing, but can we move that into a shared helper function?
> > 
> > Largely yes, but there's a subtlety.
> > 
> > We can call restore_fpsimd_context() directly for the block above, since
> > that's equivalent as of the prior patch.
> > 
> > The "fpsimd_only" label is misleading, and is used in two distinct
> > cases:
> > 
> > (1) When the SVE context has no register state, that's used to resrtore
> >     an FPSIMD-only context, and is (now) equivalent to
> >     restore_fpsimd_context().
> > 
> > (2) When the SVE context has register state, we restore the SVE state,
> >     then fall through to the "fpsimd_only" block to merge the the FPSIMD
> >     registers into the SVE registers, preserving the upper bits.
> > 
> >     The merging is hidden opaquely in fpsimd_update_current_state().
> > 
> > So we can call restore_fpsimd_context() directly for the !SVE case, and
> > we can factor out reading the user_fpsimd_state, but for case 2 we MUST
> > NOT mess with TIF_SVE, SVCR, or fp_type, and MUST merge the register
> > contents.
> 
> Right, all I'm suggesting here is to remove the 'fpsimd_only:' label and
> call into a restore_fpsimd_context()-like helper function in the
> FPSIMD-only case. We can leave the SVE case as-is (and potentially
> de-duplicate the copying later on if we want to).

Yep, that makes sense to me.

I reckon that either makes sense to fold that into the prior patch
rather than this one, or to do that as a separate patch.

Any preference?

Mark.

Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Will Deacon]

On Wed, May 07, 2025 at 04:02:40PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 03:29:39PM +0100, Will Deacon wrote:
> > On Wed, May 07, 2025 at 03:21:46PM +0100, Mark Rutland wrote:
> > > On Wed, May 07, 2025 at 01:59:16PM +0100, Will Deacon wrote:
> > > > On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> > > > > @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
> > > > >  	if (user_vl != vl)
> > > > >  		return -EINVAL;
> > > > >  
> > > > > -	if (user->sve_size == sizeof(*user->sve)) {
> > > > > +	/*
> > > > > +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> > > > > +	 * which case the SVE context only has a header with VL==0, and all
> > > > > +	 * state can be restored from the FPSIMD context.
> > > > > +	 *
> > > > > +	 * Streaming SVE state is always preserved with an SVE payload. For
> > > > > +	 * consistency and robustness, reject restoring streaming SVE state
> > > > > +	 * without an SVE payload.
> > > > > +	 */
> > > > > +	if (!sm && user->sve_size == sizeof(*user->sve)) {
> > > > >  		clear_thread_flag(TIF_SVE);
> > > > >  		current->thread.svcr &= ~SVCR_SM_MASK;
> > > > >  		current->thread.fp_type = FP_STATE_FPSIMD;
> > > > 
> > > > This (along with the 'fpsimd_only:' block) is now practically identical
> > > > to restore_fpsimd_context(). I think that's a sign that we're doing the
> > > > right thing, but can we move that into a shared helper function?
> > > 
> > > Largely yes, but there's a subtlety.
> > > 
> > > We can call restore_fpsimd_context() directly for the block above, since
> > > that's equivalent as of the prior patch.
> > > 
> > > The "fpsimd_only" label is misleading, and is used in two distinct
> > > cases:
> > > 
> > > (1) When the SVE context has no register state, that's used to resrtore
> > >     an FPSIMD-only context, and is (now) equivalent to
> > >     restore_fpsimd_context().
> > > 
> > > (2) When the SVE context has register state, we restore the SVE state,
> > >     then fall through to the "fpsimd_only" block to merge the the FPSIMD
> > >     registers into the SVE registers, preserving the upper bits.
> > > 
> > >     The merging is hidden opaquely in fpsimd_update_current_state().
> > > 
> > > So we can call restore_fpsimd_context() directly for the !SVE case, and
> > > we can factor out reading the user_fpsimd_state, but for case 2 we MUST
> > > NOT mess with TIF_SVE, SVCR, or fp_type, and MUST merge the register
> > > contents.
> > 
> > Right, all I'm suggesting here is to remove the 'fpsimd_only:' label and
> > call into a restore_fpsimd_context()-like helper function in the
> > FPSIMD-only case. We can leave the SVE case as-is (and potentially
> > de-duplicate the copying later on if we want to).
> 
> Yep, that makes sense to me.
> 
> I reckon that either makes sense to fold that into the prior patch
> rather than this one, or to do that as a separate patch.
> 
> Any preference?

Totally up to you.

Will

[PATCH 05/20] arm64/fpsimd: ptrace: Consistently handle partial writes to NT_ARM_(S)SVE

[Mark Rutland]

Partial writes to the NT_ARM_SVE and NT_ARM_SSVE regsets using an
payload are handled inconsistently and non-deterministically. A comment
within sve_set_common() indicates that we intended that a partial write
would preserve any effective FPSIMD/SVE state which was not overwritten,
but this has never worked consistently, and during syscalls the FPSIMD
vector state may be non-deterministically preserved and may be
erroneously migrated between streaming and non-streaming SVE modes.

The simplest fix is to handle a partial write by consistently zeroing
the remaining state. As detailed below I do not believe this will
adversely affect any real usage.

Neither GDB nor LLDB attempt partial writes to these regsets, and the
documentation (in Documentation/arch/arm64/sve.rst) has always indicated
that state preservation was not guaranteed, as is says:

| The effect of writing a partial, incomplete payload is unspecified.

When the logic was originally introduced in commit:

  43d4da2c45b2 ("arm64/sve: ptrace and ELF coredump support")

... there were two potential behaviours, depending on TIF_SVE:

* When TIF_SVE was clear, all SVE state would be zeroed, excluding the
  low 128 bits of vectors shared with FPSIMD, FPSR, and FPCR.

* When TIF_SVE was set, all SVE state would be zeroed, including the
  low 128 bits of vectors shared with FPSIMD, but excluding FPSR and
  FPCR.

Note that as writing to NT_ARM_SVE would set TIF_SVE, partial writes to
NT_ARM_SVE would not be idempotent, and if a first write preserved the
low 128 bits, a subsequent (potentially identical) partial write would
discard the low 128 bits.

When support for the NT_ARM_SSVE regset was added in commit:

  e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")

... the above behaviour was retained for writes to the NT_ARM_SVE
regset, though writes to the NT_ARM_SSVE would always zero the SVE
registers and would not inherit FPSIMD register state. This happened as
fpsimd_sync_to_sve() only copied the FPSIMD regs when TIF_SVE was clear
and PSTATE.SM==0.

Subsequently, when FPSIMD/SVE state tracking was changed across commits:

  baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
  a0136be443d5 (arm64/fpsimd: Load FP state based on recorded data type")
  bbc6172eefdb ("arm64/fpsimd: SME no longer requires SVE register state")
  8c845e273104 ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")

... there was no corresponding update to the ptrace code, nor to
fpsimd_sync_to_sve(), which stil considers TIF_SVE and PSTATE.SM rather
than the saved fp_type. The saved state can be in the FPSIMD format
regardless of whether TIF_SVE is set or clear, and the saved type can
change non-deterministically during syscalls. Consequently a subsequent
partial write to the NT_ARM_SVE or NT_ARM_SSVE regsets may
non-deterministically preserve the FPSIMD state, and may migrate this
state between streaming and non-streaming modes.

Clean this up by never attempting to preserve ANY state when writing an
SVE payload to the NT_ARM_SVE/NT_ARM_SSVE regsets, zeroing all relevant
state including FPSR and FPCR. This simplifies the code, makes the
behaviour deterministic, and avoids migrating state between streaming
and non-streaming modes. As above, I do not believe this should
adversely affect existing userspace applications.

At the same time, remove fpsimd_sync_to_sve(). It is no longer used,
doesn't do what its documentation implies, and gets in the way of other
cleanups and fixes.

Fixes: 43d4da2c45b2 ("arm64/sve: ptrace and ELF coredump support")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h |  1 -
 arch/arm64/kernel/fpsimd.c      | 15 ---------------
 arch/arm64/kernel/ptrace.c      | 26 +++++++++-----------------
 3 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 11b9c6a5e0377..da1bd5b9a7e70 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -198,7 +198,6 @@ struct vl_info {
 
 extern void sve_alloc(struct task_struct *task, bool flush);
 extern void fpsimd_release_task(struct task_struct *task);
-extern void fpsimd_sync_to_sve(struct task_struct *task);
 extern void sve_sync_to_fpsimd(struct task_struct *task);
 extern void sve_sync_from_fpsimd_zeropad(struct task_struct *task);
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 9f1890d692dfc..e06e4d8eda49e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -759,21 +759,6 @@ void sve_alloc(struct task_struct *task, bool flush)
 		kzalloc(sve_state_size(task), GFP_KERNEL);
 }
 
-/*
- * Ensure that task->thread.sve_state is up to date with respect to
- * the user task, irrespective of when SVE is in use or not.
- *
- * This should only be called by ptrace.  task must be non-runnable.
- * task->thread.sve_state must point to at least sve_state_size(task)
- * bytes of allocated kernel memory.
- */
-void fpsimd_sync_to_sve(struct task_struct *task)
-{
-	if (!test_tsk_thread_flag(task, TIF_SVE) &&
-	    !thread_sm_enabled(&task->thread))
-		fpsimd_to_sve(task);
-}
-
 /*
  * Ensure that task->thread.uw.fpsimd_state is up to date with respect to
  * the user task, irrespective of whether SVE is in use or not.
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index f79b0d5f71ac9..259d90f70e025 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -910,8 +910,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;
@@ -931,23 +929,20 @@ static int sve_set_common(struct task_struct *target,
 			ret = -EINVAL;
 			goto out;
 		}
-
-		/*
-		 * If we switched then invalidate any existing SVE
-		 * state and ensure there's storage.
-		 */
-		if (target->thread.svcr != old_svcr)
-			sve_alloc(target, true);
 	}
 
+	/* Always zero V regs, FPSR, and FPCR */
+	memset(&current->thread.uw.fpsimd_state, 0,
+	       sizeof(current->thread.uw.fpsimd_state));
+
 	/* Registers: FPSIMD-only case */
 
 	BUILD_BUG_ON(SVE_PT_FPSIMD_OFFSET != sizeof(header));
 	if ((header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD) {
-		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
-				SVE_PT_FPSIMD_OFFSET);
 		clear_tsk_thread_flag(target, TIF_SVE);
 		target->thread.fp_type = FP_STATE_FPSIMD;
+		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
+				SVE_PT_FPSIMD_OFFSET);
 		goto out;
 	}
 
@@ -966,6 +961,7 @@ static int sve_set_common(struct task_struct *target,
 		goto out;
 	}
 
+	/* Always zero SVE state */
 	sve_alloc(target, true);
 	if (!target->thread.sve_state) {
 		ret = -ENOMEM;
@@ -975,13 +971,9 @@ static int sve_set_common(struct task_struct *target,
 	}
 
 	/*
-	 * Ensure target->thread.sve_state is up to date with target's
-	 * FPSIMD regs, so that a short copyin leaves trailing
-	 * registers unmodified.  Only enable SVE if we are
-	 * configuring normal SVE, a system with streaming SVE may not
-	 * have normal SVE.
+	 * Only enable SVE if we are configuring normal SVE, a system with
+	 * streaming SVE may not have normal SVE.
 	 */
-	fpsimd_sync_to_sve(target);
 	if (type == ARM64_VEC_SVE)
 		set_tsk_thread_flag(target, TIF_SVE);
 	target->thread.fp_type = FP_STATE_SVE;
-- 
2.30.2

[PATCH 06/20] arm64/fpsimd: Clarify sve_sync_*() functions

[Mark Rutland]

The sve_sync_{to,from}_fpsimd*() functions are intended to
extract/insert the currently effective FPSIMD state of a task regardless
of whether the task's state is saved in FPSIMD format or SVE format.
Historically they were only used by ptrace, but sve_sync_to_fpsimd() is
now used more widely, and sve_sync_from_fpsimd_zeropad() may be used
more widely in future.

When FPSIMD/SVE state tracking was changed across commits:

  baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
  a0136be443d5 (arm64/fpsimd: Load FP state based on recorded data type")
  bbc6172eefdb ("arm64/fpsimd: SME no longer requires SVE register state")
  8c845e273104 ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")

... sve_sync_to_fpsimd() was updated to consider task->thread.fp_type
rather than the task's TIF_SVE and PSTATE.SM, but (apparently due to an
oversight) sve_sync_from_fpsimd_zeropad() was left as-is, leaving the
two inconsistent.

Due to this, sve_sync_from_fpsimd_zeropad() may copy state from
task->thread.uw.fpsimd_state into task->thread.sve_state when
task->thread.fp_type == FP_STATE_FPSIMD. This is redundant (but benign)
as task->thread.uw.fpsimd_state is the effective state that will be
restored, and task->thread.sve_state will not be consumed. For
consistency, and to avoid the redundant work, it better for
sve_sync_from_fpsimd_zeropad() to consider task->thread.fp_type alone,
matching sve_sync_to_fpsimd().

The naming of both functions is somehat unfortunate, as it is unclear
when and why they copy state. It would be better to describe them in
terms of the effective state.

Considering all of the above, clean this up:

* Adjust sve_sync_from_fpsimd_zeropad() to consider
  task->thread.fp_type.

* Update comments to clarify the intended semantics/usage. I've removed
  the description that task->thread.sve_state must have been allocated,
  as this is only necessary when task->thread.fp_type == FP_STATE_SVE,
  which itself implies that task->thread.sve_state must have been
  allocated.

* Rename the functions to more clearly indicate when/why they copy
  state:

  - sve_sync_to_fpsimd() => fpsimd_sync_from_effective_state()

  - sve_sync_from_fpsimd_zeropad => fpsimd_sync_to_effective_state_zeropad()

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h |  8 ++++----
 arch/arm64/kernel/fpsimd.c      | 30 ++++++++++++------------------
 arch/arm64/kernel/ptrace.c      |  6 +++---
 arch/arm64/kernel/signal.c      |  2 +-
 4 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index da1bd5b9a7e70..c9e17d093fe23 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -198,8 +198,8 @@ struct vl_info {
 
 extern void sve_alloc(struct task_struct *task, bool flush);
 extern void fpsimd_release_task(struct task_struct *task);
-extern void sve_sync_to_fpsimd(struct task_struct *task);
-extern void sve_sync_from_fpsimd_zeropad(struct task_struct *task);
+extern void fpsimd_sync_from_effective_state(struct task_struct *task);
+extern void fpsimd_sync_to_effective_state_zeropad(struct task_struct *task);
 
 extern int vec_set_vector_length(struct task_struct *task, enum vec_type type,
 				 unsigned long vl, unsigned long flags);
@@ -299,8 +299,8 @@ size_t sve_state_size(struct task_struct const *task);
 
 static inline void sve_alloc(struct task_struct *task, bool flush) { }
 static inline void fpsimd_release_task(struct task_struct *task) { }
-static inline void sve_sync_to_fpsimd(struct task_struct *task) { }
-static inline void sve_sync_from_fpsimd_zeropad(struct task_struct *task) { }
+static inline void fpsimd_sync_from_effective_state(struct task_struct *task) { }
+static inline void fpsimd_sync_to_effective_state_zeropad(struct task_struct *task) { }
 
 static inline int sve_max_virtualisable_vl(void)
 {
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index e06e4d8eda49e..dd6480087683c 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -760,39 +760,33 @@ void sve_alloc(struct task_struct *task, bool flush)
 }
 
 /*
- * Ensure that task->thread.uw.fpsimd_state is up to date with respect to
- * the user task, irrespective of whether SVE is in use or not.
+ * Ensure that task->thread.uw.fpsimd_state is up to date with respect to the
+ * task's currently effective FPSIMD/SVE state.
  *
- * This should only be called by ptrace.  task must be non-runnable.
- * task->thread.sve_state must point to at least sve_state_size(task)
- * bytes of allocated kernel memory.
+ * The task's FPSIMD/SVE/SME state must not be subject to concurrent
+ * manipulation.
  */
-void sve_sync_to_fpsimd(struct task_struct *task)
+void fpsimd_sync_from_effective_state(struct task_struct *task)
 {
 	if (task->thread.fp_type == FP_STATE_SVE)
 		sve_to_fpsimd(task);
 }
 
 /*
- * Ensure that task->thread.sve_state is up to date with respect to
- * the task->thread.uw.fpsimd_state.
+ * Ensure that the task's currently effective FPSIMD/SVE state is up to date
+ * with respect to task->thread.uw.fpsimd_state, zeroing any effective
+ * non-FPSIMD (S)SVE state.
  *
- * This should only be called by ptrace to merge new FPSIMD register
- * values into a task for which SVE is currently active.
- * task must be non-runnable.
- * task->thread.sve_state must point to at least sve_state_size(task)
- * bytes of allocated kernel memory.
- * task->thread.uw.fpsimd_state must already have been initialised with
- * the new FPSIMD register values to be merged in.
+ * The task's FPSIMD/SVE/SME state must not be subject to concurrent
+ * manipulation.
  */
-void sve_sync_from_fpsimd_zeropad(struct task_struct *task)
+void fpsimd_sync_to_effective_state_zeropad(struct task_struct *task)
 {
 	unsigned int vq;
 	void *sst = task->thread.sve_state;
 	struct user_fpsimd_state const *fst = &task->thread.uw.fpsimd_state;
 
-	if (!test_tsk_thread_flag(task, TIF_SVE) &&
-	    !thread_sm_enabled(&task->thread))
+	if (task->thread.fp_type != FP_STATE_SVE)
 		return;
 
 	vq = sve_vq_from_vl(thread_get_cur_vl(&task->thread));
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 259d90f70e025..bdba106a4cf29 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -594,7 +594,7 @@ static int __fpr_get(struct task_struct *target,
 {
 	struct user_fpsimd_state *uregs;
 
-	sve_sync_to_fpsimd(target);
+	fpsimd_sync_from_effective_state(target);
 
 	uregs = &target->thread.uw.fpsimd_state;
 
@@ -626,7 +626,7 @@ static int __fpr_set(struct task_struct *target,
 	 * Ensure target->thread.uw.fpsimd_state is up to date, so that a
 	 * short copyin can't resurrect stale data.
 	 */
-	sve_sync_to_fpsimd(target);
+	fpsimd_sync_from_effective_state(target);
 
 	newstate = target->thread.uw.fpsimd_state;
 
@@ -653,7 +653,7 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset,
 	if (ret)
 		return ret;
 
-	sve_sync_from_fpsimd_zeropad(target);
+	fpsimd_sync_to_effective_state_zeropad(target);
 	fpsimd_flush_task_state(target);
 
 	return ret;
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e898948a31b65..57b15f8bc29ff 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -250,7 +250,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 		&current->thread.uw.fpsimd_state;
 	int err;
 
-	sve_sync_to_fpsimd(current);
+	fpsimd_sync_from_effective_state(current);
 
 	/* copy the FP and status/control registers */
 	err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
-- 
2.30.2

[PATCH 07/20] arm64/fpsimd: Factor out {sve,sme}_state_size() helpers

[Mark Rutland]

In subsequent patches we'll need to determine the SVE/SME state size for
a given SVE VL and SME VL regardless of whether a task is currently
configured with those VLs. Split the sizing logic out of
sve_state_size() and sme_state_size() so that we don't need to open-code
this logic elsewhere.

At the same time, apply minor cleanups:

* Move sve_state_size() into fpsimd.h, matching the placement of
  sme_state_size().

* Remove the feature checks from sve_state_size(). We only call
  sve_state_size() when at least one of SVE and SME are supported, and
  when either of the two is not supported, the task's corresponding
  SVE/SME vector length will be zero.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h | 47 ++++++++++++++++++++++++++-------
 arch/arm64/kernel/fpsimd.c      | 16 -----------
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index c9e17d093fe23..b751064bbaaa1 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -293,7 +293,22 @@ static inline bool sve_vq_available(unsigned int vq)
 	return vq_available(ARM64_VEC_SVE, vq);
 }
 
-size_t sve_state_size(struct task_struct const *task);
+static inline size_t __sve_state_size(unsigned int sve_vl, unsigned int sme_vl)
+{
+	unsigned int vl = max(sve_vl, sme_vl);
+	return SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl));
+}
+
+/*
+ * Return how many bytes of memory are required to store the full SVE
+ * state for task, given task's currently configured vector length.
+ */
+static inline size_t sve_state_size(struct task_struct const *task)
+{
+	unsigned int sve_vl = task_get_sve_vl(task);
+	unsigned int sme_vl = task_get_sme_vl(task);
+	return __sve_state_size(sve_vl, sme_vl);
+}
 
 #else /* ! CONFIG_ARM64_SVE */
 
@@ -334,6 +349,11 @@ static inline void vec_update_vq_map(enum vec_type t) { }
 static inline int vec_verify_vq_map(enum vec_type t) { return 0; }
 static inline void sve_setup(void) { }
 
+static inline size_t __sve_state_size(unsigned int sve_vl, unsigned int sme_vl)
+{
+	return 0;
+}
+
 static inline size_t sve_state_size(struct task_struct const *task)
 {
 	return 0;
@@ -386,6 +406,16 @@ extern int sme_set_current_vl(unsigned long arg);
 extern int sme_get_current_vl(void);
 extern void sme_suspend_exit(void);
 
+static inline size_t __sme_state_size(unsigned int sme_vl)
+{
+	size_t size = ZA_SIG_REGS_SIZE(sve_vq_from_vl(sme_vl));
+
+	if (system_supports_sme2())
+		size += ZT_SIG_REG_SIZE;
+
+	return size;
+}
+
 /*
  * Return how many bytes of memory are required to store the full SME
  * specific state for task, given task's currently configured vector
@@ -393,15 +423,7 @@ extern void sme_suspend_exit(void);
  */
 static inline size_t sme_state_size(struct task_struct const *task)
 {
-	unsigned int vl = task_get_sme_vl(task);
-	size_t size;
-
-	size = ZA_SIG_REGS_SIZE(sve_vq_from_vl(vl));
-
-	if (system_supports_sme2())
-		size += ZT_SIG_REG_SIZE;
-
-	return size;
+	return __sme_state_size(task_get_sme_vl(task));
 }
 
 #else
@@ -422,6 +444,11 @@ static inline int sme_set_current_vl(unsigned long arg) { return -EINVAL; }
 static inline int sme_get_current_vl(void) { return -EINVAL; }
 static inline void sme_suspend_exit(void) { }
 
+static inline size_t __sme_state_size(unsigned int sme_vl)
+{
+	return 0;
+}
+
 static inline size_t sme_state_size(struct task_struct const *task)
 {
 	return 0;
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index dd6480087683c..c2603fe8dd243 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -719,22 +719,6 @@ static void sve_free(struct task_struct *task)
 	__sve_free(task);
 }
 
-/*
- * Return how many bytes of memory are required to store the full SVE
- * state for task, given task's currently configured vector length.
- */
-size_t sve_state_size(struct task_struct const *task)
-{
-	unsigned int vl = 0;
-
-	if (system_supports_sve())
-		vl = task_get_sve_vl(task);
-	if (system_supports_sme())
-		vl = max(vl, task_get_sme_vl(task));
-
-	return SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl));
-}
-
 /*
  * Ensure that task->thread.sve_state is allocated and sufficiently large.
  *
-- 
2.30.2

[PATCH 08/20] arm64/fpsimd: Add task_smstop_sm()

[Mark Rutland]

In a few places we want to transition a task from streaming mode to
non-streaming mode, e.g. signal delivery where we historically tried to
use an SMSTOP SM instruction.

Add a new helper to manipulate a task's state in the same way as an
SMSTOP SM instruction. I have not added a corresponding helper to
simulate the effects of SMSTART SM. Only ptrace transitions a task into
streaming mode, and ptrace has distinct semantics for such transitions.

Per ARM DDI 0487 L.a, section B1.4.6:

| RRSWFQ
| When the Effective value of PSTATE.SM is changed by any method from 0
| to 1, an entry to Streaming SVE mode is performed, and all implemented
| bits of Streaming SVE register state are set to zero.

| RKFRQZ
| When the Effective value of PSTATE.SM is changed by any method from 1
| to 0, an exit from Streaming SVE mode is performed, and in the
| newly-entered mode, all implemented bits of the SVE scalable vector
| registers, SVE predicate registers, and FFR, are set to zero.

Per ARM DDI 0487 L.a, section C5.2.9:

| On entry to or exit from Streaming SVE mode, FPMR is set to 0

Per ARM DDI 0487 L.a, section C5.2.10:

| On entry to or exit from Streaming SVE mode, FPSR.{IOC, DZC, OFC, UFC,
| IXC, IDC, QC} are set to 1 and the remaining bits are set to 0.

This means bits 0, 1, 2, 3, 4, 7, and 27 respectively, i.e. 0x0800009f

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h |  2 ++
 arch/arm64/kernel/fpsimd.c      | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index b751064bbaaa1..b8cf0ea43cc05 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -111,6 +111,8 @@ static inline bool thread_za_enabled(struct thread_struct *thread)
 	return system_supports_sme() && (thread->svcr & SVCR_ZA_MASK);
 }
 
+extern void task_smstop_sm(struct task_struct *task);
+
 /* Maximum VL that SVE/SME VL-agnostic software can transparently support */
 #define VL_ARCH_MAX 0x100
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index c2603fe8dd243..fe96e018e18c0 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -695,6 +695,28 @@ static inline void sve_to_fpsimd(struct task_struct *task)
 	}
 }
 
+static inline void __fpsimd_zero_vregs(struct user_fpsimd_state *fpsimd)
+{
+	memset(&fpsimd->vregs, 0, sizeof(fpsimd->vregs));
+}
+
+/*
+ * Simulate the effects of an SMSTOP SM instruction.
+ */
+void task_smstop_sm(struct task_struct *task)
+{
+	if (!thread_sm_enabled(&task->thread))
+		return;
+
+	__fpsimd_zero_vregs(&task->thread.uw.fpsimd_state);
+	task->thread.uw.fpsimd_state.fpsr = 0x0800009f;
+	if (system_supports_fpmr())
+		task->thread.uw.fpmr = 0;
+
+	task->thread.svcr &= ~SVCR_SM_MASK;
+	task->thread.fp_type = FP_STATE_FPSIMD;
+}
+
 void cpu_enable_fpmr(const struct arm64_cpu_capabilities *__always_unused p)
 {
 	write_sysreg_s(read_sysreg_s(SYS_SCTLR_EL1) | SCTLR_EL1_EnFPM_MASK,
-- 
2.30.2

[PATCH 09/20] arm64/fpsimd: signal: Use SMSTOP behaviour in setup_return()

[Mark Rutland]

Historically the behaviour of setup_return() was nondeterministic,
depending on whether the task's FSIMD/SVE/SME state happened to be live.
We fixed most of that in commit:

  929fa99b1215 ("arm64/fpsimd: signal: Always save+flush state early")

... but we didn't decide on how clearing PSTATE.SM should behave, and left a
TODO comment to that effect.

Use the new task_smstop_sm() helper to make this behave as if an SMSTOP
instruction was used to exit streaming mode. This would have been the
most common behaviour prior to the commit above.

Fixes: 40a8e87bb328 ("arm64/sme: Disable ZA and streaming mode when handling signals")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/signal.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 57b15f8bc29ff..bd5fd6ddb4486 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -1475,22 +1475,8 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
 
 	/* 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.
-		 *
-		 * TODO: decide if this should behave as SMSTOP (e.g. reset
-		 * FPSR + FPMR), or whether this should only clear the scalable
-		 * registers + ZA state.
-		 */
-		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);
+		task_smstop_sm(current);
+		current->thread.svcr &= ~SVCR_ZA_MASK;
 		write_sysreg_s(0, SYS_TPIDR2_EL0);
 	}
 
-- 
2.30.2

[PATCH 10/20] arm64/fpsimd: Remove redundant task->mm check

[Mark Rutland]

For historical reasons, arch_dup_task_struct() only calls
fpsimd_preserve_current_state() when current->mm is non-NULL, but this
is no longer necessary.

Historically TIF_FOREIGN_FPSTATE was only managed for user threads, and
was never set for kernel threads. At the time, various functions
attempted to avoid saving/restoring state for kernel threads by checking
task_struct::mm to try to dinstinguish user threads from kernel threads.

We added the current->mm check to arch_dup_task_struct() in commit:

  6eb6c80187c5 ("arm64: kernel thread don't need to save fpsimd context.")

... where the intent was to avoid pointlessly saving state for kernel
threads, which never had live state (and the saved state would never be
consumed).

Subsequently we began setting TIF_FOREIGN_FPSTATE for kernel threads,
and removed most of the task_struct::mm checks in commit:

  df3fb9682045 ("arm64: fpsimd: Eliminate task->mm checks")

... but we missed the check in arch_dup_task_struct(), which is similarly
redundant.

Remove the redundant check from arch_dup_task_struct().

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 42faebb7b7123..885c1adcf54ca 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -344,8 +344,7 @@ void arch_release_task_struct(struct task_struct *tsk)
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
-	if (current->mm)
-		fpsimd_preserve_current_state();
+	fpsimd_preserve_current_state();
 	*dst = *src;
 
 	/*
-- 
2.30.2

[PATCH 11/20] arm64/fpsimd: Consistently preserve FPSIMD state during clone()

[Mark Rutland]

In arch_dup_task_struct() we try to ensure that the child task inherits
the FPSIMD state of its parent, but this depends on the parent task's
saved state being in FPSIMD format, which is not always the case.
Consequently the child task may inherit stale FPSIMD state in some
cases.

This can happen when the parent's state has been modified by ptrace
since syscall entry, as writes to the NT_ARM_SVE regset may save state
in SVE format. This has been possible since commit:

  bc0ee4760364 ("arm64/sve: Core task context handling")

More recently it has been possible for a task's FPSIMD/SVE state to be
saved before lazy discarding was guaranteed to occur, in which case
preemption could cause the effective FPSIMD state to be saved in SVE
format non-deterministically. This has been possible since commit:

  f130ac0ae441 ("arm64: syscall: unmask DAIF earlier for SVCs")

Fix this by saving the parent task's effective FPSIMD state into FPSIMD
format before copying the task_struct. As this requires modifying the
parent's fpsimd_state, we must save+flush the state to avoid racing with
concurrent manipulation.

Similar issues exist when the parent has streaming mode state, and will
be addressed by subsequent patches.

Fixes: bc0ee4760364 ("arm64/sve: Core task context handling")
Fixes: f130ac0ae441 ("arm64: syscall: unmask DAIF earlier for SVCs")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 885c1adcf54ca..3bb7f65bf7b7c 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -344,7 +344,14 @@ void arch_release_task_struct(struct task_struct *tsk)
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
-	fpsimd_preserve_current_state();
+	/*
+	 * The current/src task's FPSIMD state may or may not be live, and may
+	 * have been altered by ptrace after entry to the kernel. Save the
+	 * effective FPSIMD state so that this will be copied into dst.
+	 */
+	fpsimd_save_and_flush_current_state();
+	fpsimd_sync_from_effective_state(src);
+
 	*dst = *src;
 
 	/*
-- 
2.30.2

[PATCH 12/20] arm64/fpsimd: Clear PSTATE.SM during clone()

[Mark Rutland]

Currently arch_dup_task_struct() doesn't handle cases where the parent
task has PSTATE.SM==1. Since syscall entry exits streaming mode, the
parent will usually have PSTATE.SM==0, but this can be change by ptrace
after syscall entry. When this happens, arch_dup_task_struct() will
initialise the new task into an invalid state. The new task inherits the
parent's configuration of PSTATE.SM, but fp_type is set to
FP_STATE_FPSIMD, TIF_SVE and SME may be cleared, and both sve_state and
sme_state may be set to NULL.

This can result in a variety of problems whenever the new task's state
is manipulated, including kernel NULL pointer dereferences and leaking
of streaming mode state between tasks.

When ptrace is not invovled, the parent will have PSTATE.SM==0 as a
result of syscall entry, and the documentation in
Documentation/arch/arm64/sme.rst says:

| On process creation (eg, clone()) the newly created process will have
| PSTATE.SM cleared.

... so make this true by using task_smstop_sm() to exit streaming mode
in the child task, avoiding the problems above.

Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3bb7f65bf7b7c..27a5b0c7ec60b 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -355,16 +355,13 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	*dst = *src;
 
 	/*
-	 * Detach src's sve_state (if any) from dst so that it does not
-	 * get erroneously used or freed prematurely.  dst's copies
-	 * will be allocated on demand later on if dst uses SVE.
-	 * For consistency, also clear TIF_SVE here: this could be done
-	 * later in copy_process(), but to avoid tripping up future
-	 * maintainers it is best not to leave TIF flags and buffers in
-	 * an inconsistent state, even temporarily.
+	 * Drop stale reference to src's sve_state and convert dst to
+	 * non-streaming FPSIMD mode.
 	 */
+	dst->thread.fp_type = FP_STATE_FPSIMD;
 	dst->thread.sve_state = NULL;
 	clear_tsk_thread_flag(dst, TIF_SVE);
+	task_smstop_sm(dst);
 
 	/*
 	 * In the unlikely event that we create a new thread with ZA
@@ -393,8 +390,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 		clear_tsk_thread_flag(dst, TIF_SME);
 	}
 
-	dst->thread.fp_type = FP_STATE_FPSIMD;
-
 	/* clear any pending asynchronous tag fault raised by the parent */
 	clear_tsk_thread_flag(dst, TIF_MTE_ASYNC_FAULT);
 
-- 
2.30.2

[PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Mark Rutland]

Linux is intended to be compatible with userspace written to Arm's
AAPCS64 procedure call standard [1,2]. For the Scalable Matrix Extension
(SME), AAPCS64 was extended with a "ZA lazy saving scheme", where SME's
ZA tile is lazily callee-saved and caller-restored. In this scheme,
TPIDR2_EL0 indicates whether the ZA tile is live or has been saved by
pointing to a "TPIDR2 block" in memory, which has a "za_save_buffer"
pointer. This scheme has been implemented in GCC and LLVM, with
necessary runtime support implemented in glibc and bionic.

AAPCS64 does not specify how the ZA lazy saving scheme is expected to
interact with thread creation mechanisms such as fork() and
pthread_create(), which would be implemented in terms of the Linux clone
syscall. The behaviour implemented by Linux and glibc/bionic doesn't
always compose safely, as explained below.

Currently the clone syscall is implemented such that PSTATE.ZA and the
ZA tile are always inherited by the new task, and TPIDR2_EL0 is
inherited unless the 'flags' argument includes CLONE_SETTLS,
in which case TPIDR2_EL0 is set to 0/NULL. This doesn't make much sense:

(a) TPIDR2_EL0 is part of the calling convention, and changes as control
    is passed between functions. It is *NOT* used for thread local
    storage, despite superficial similarity to TPIDR_EL0, which is is
    used as the TLS register.

(b) TPIDR2_EL0 and PSTATE.ZA are tightly coupled in the procedure call
    standard, and some combinations of states are illegal. In general,
    manipulating the two independently is not guaranteed to be safe.

In practice, code which is compliant with the procedure call standard
may issue a clone syscall while in the "ZA dormant" state, where
PSTATE.ZA==1 and TPIDR2_EL0 is non-null and indicates that ZA needs to
be saved. This can cause a variety of problems, including:

* If the implementation of pthread_create() passes CLONE_SETTLS, the
  new thread will start with PSTATE.ZA==1 and TPIDR2==NULL. Per the
  procedure call standard this is not a legitimate state for most
  functions. This can cause data corruption (e.g. as code may rely on
  PSTATE.ZA being 0 to guarantee that an SMSTART ZA instruction will
  zero the ZA tile contents), and may result in other undefined
  behaviour.

* If the implementation of pthread_create() does not pass CLONE_SETTLS, the
  new thread will start with PSTATE.ZA==1 and TPIDR2 pointing to a
  TPIDR2 block on the parent thread's stack. This can result in a
  variety of problems, e.g.

  - The child may write back to the parent's za_save_buffer, corrupting
    its contents.

  - The child may read from the TPIDR2 block after the parent has reused
    this memory for something else, and consequently the child may abort
    or clobber arbitrary memory.

Ideally we'd require that userspace ensures that a task is in the "ZA
off" state (with PSTATE.ZA==0 and TPIDR2_EL0==NULL) prior to issuing a
clone syscall, and have the kernel force this state for new threads.
Unfortunately, contemporary C libraries do not do this, and simply
forcing this state within the implementation of clone would break
fork().

Instead, we can bodge around this by considering the CLONE_VM flag, and
mainpulate PSTATE.ZA and TPIDR2_EL0 as a pair. CLONE_VM indicates that
the new task will run in the same address space as its parent, and in
that case it doesn't make sense to inherit a stale pointer to the
parent's TPIDR2 block:

* For fork(), CLONE_VM will not be set, and it is safe to inherit both
  PSTATE.ZA and TPIDR2_EL0 as the new task will have its own copy of the
  address space, and cannot clobber its parent's stack.

* For pthread_create() and vfork(), CLONE_VM will be set, and discarding
  PSTATE.ZA and TPIDR2_EL0 for the new task doesn't break any existing
  assumptions in userspace.

Implement this behaviour for clone(). We currently inherit PSTATE.ZA in
arch_dup_task_struct(), but this does not have access to the clone
flags, so move this logic under copy_thread(). Documentation and tests
are updated to conform with the new behaviour.

[1] https://github.com/ARM-software/abi-aa/releases/download/2025Q1/aapcs64.pdf
[2] https://github.com/ARM-software/abi-aa/blob/c51addc3dc03e73a016a1e4edf25440bcac76431/aapcs64/aapcs64.rst

Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Richard Sandiford <richard.sandiford@arm.com>
Cc: Sander De Smalen <sander.desmalen@arm.com>
Cc: Tamas Petz <tamas.petz@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Yury Khrustalev <yury.khrustalev@arm.com>
---
 Documentation/arch/arm64/sme.rst           |  4 +-
 arch/arm64/kernel/process.c                | 89 +++++++++++++++-------
 tools/testing/selftests/arm64/abi/tpidr2.c | 14 +++-
 3 files changed, 75 insertions(+), 32 deletions(-)

diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
index 3a98aed92732e..1c1e48d8bd1a5 100644
--- a/Documentation/arch/arm64/sme.rst
+++ b/Documentation/arch/arm64/sme.rst
@@ -69,8 +69,8 @@ model features for SME is included in Appendix A.
   vectors from 0 to VL/8-1 stored in the same endianness invariant format as is
   used for SVE vectors.
 
-* On thread creation TPIDR2_EL0 is preserved unless CLONE_SETTLS is specified,
-  in which case it is set to 0.
+* On thread creation PSTATE.ZA and TPIDR2_EL0 are preserved unless CLONE_VM
+  is specified, in which case PSTATE.ZA is set to 0 and TPIDR2_EL0 is set to 0.
 
 2.  Vector lengths
 ------------------
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 27a5b0c7ec60b..cab9e80cb587f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -364,31 +364,14 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	task_smstop_sm(dst);
 
 	/*
-	 * In the unlikely event that we create a new thread with ZA
-	 * enabled we should retain the ZA and ZT state so duplicate
-	 * it here.  This may be shortly freed if we exec() or if
-	 * CLONE_SETTLS but it's simpler to do it here. To avoid
-	 * confusing the rest of the code ensure that we have a
-	 * sve_state allocated whenever sme_state is allocated.
+	 * Drop stale reference to src's sme_state and ensure dst has ZA
+	 * disabled.
+	 *
+	 * When necessary, ZA will be inherited later in copy_thread_za().
 	 */
-	if (thread_za_enabled(&src->thread)) {
-		dst->thread.sve_state = kzalloc(sve_state_size(src),
-						GFP_KERNEL);
-		if (!dst->thread.sve_state)
-			return -ENOMEM;
-
-		dst->thread.sme_state = kmemdup(src->thread.sme_state,
-						sme_state_size(src),
-						GFP_KERNEL);
-		if (!dst->thread.sme_state) {
-			kfree(dst->thread.sve_state);
-			dst->thread.sve_state = NULL;
-			return -ENOMEM;
-		}
-	} else {
-		dst->thread.sme_state = NULL;
-		clear_tsk_thread_flag(dst, TIF_SME);
-	}
+	dst->thread.sme_state = NULL;
+	clear_tsk_thread_flag(dst, TIF_SME);
+	dst->thread.svcr &= ~SVCR_ZA_MASK;
 
 	/* clear any pending asynchronous tag fault raised by the parent */
 	clear_tsk_thread_flag(dst, TIF_MTE_ASYNC_FAULT);
@@ -396,6 +379,31 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	return 0;
 }
 
+static int copy_thread_za(struct task_struct *dst, struct task_struct *src)
+{
+	if (!thread_za_enabled(&src->thread))
+		return 0;
+
+	dst->thread.sve_state = kzalloc(sve_state_size(src),
+					GFP_KERNEL);
+	if (!dst->thread.sve_state)
+		return -ENOMEM;
+
+	dst->thread.sme_state = kmemdup(src->thread.sme_state,
+					sme_state_size(src),
+					GFP_KERNEL);
+	if (!dst->thread.sme_state) {
+		kfree(dst->thread.sve_state);
+		dst->thread.sve_state = NULL;
+		return -ENOMEM;
+	}
+
+	set_tsk_thread_flag(dst, TIF_SME);
+	dst->thread.svcr |= SVCR_ZA_MASK;
+
+	return 0;
+}
+
 asmlinkage void ret_from_fork(void) asm("ret_from_fork");
 
 int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
@@ -441,14 +449,39 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
 				childregs->sp = stack_start;
 		}
 
+		/*
+		 * Due to the AAPCS64 "ZA lazy saving scheme", PSTATE.ZA and
+		 * TPIDR2 need to be manipulated as a pair, and either both
+		 * need to be inherited or both need to be reset.
+		 *
+		 * Within a process, child threads must not inherit their
+		 * parent's TPIDR2 value or they may clobber their parent's
+		 * stack at some later point.
+		 *
+		 * When a process is fork()'d, the child must inherit ZA and
+		 * TPIDR2 from its parent in case there was dormant ZA state.
+		 *
+		 * Use CLONE_VM to determine when the child will share the
+		 * address space with the parent, and cannot safely inherit the
+		 * state.
+		 */
+		if (system_supports_sme()) {
+			if (!(clone_flags & CLONE_VM)) {
+				p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
+				ret = copy_thread_za(p, current);
+				if (ret)
+					return ret;
+			} else {
+				p->thread.tpidr2_el0 = 0;
+			}
+		}
+
 		/*
 		 * If a TLS pointer was passed to clone, use it for the new
-		 * thread.  We also reset TPIDR2 if it's in use.
+		 * thread.
 		 */
-		if (clone_flags & CLONE_SETTLS) {
+		if (clone_flags & CLONE_SETTLS)
 			p->thread.uw.tp_value = tls;
-			p->thread.tpidr2_el0 = 0;
-		}
 
 		ret = copy_thread_gcs(p, args);
 		if (ret != 0)
diff --git a/tools/testing/selftests/arm64/abi/tpidr2.c b/tools/testing/selftests/arm64/abi/tpidr2.c
index 285c47dd42f63..eb19dcc37a755 100644
--- a/tools/testing/selftests/arm64/abi/tpidr2.c
+++ b/tools/testing/selftests/arm64/abi/tpidr2.c
@@ -169,8 +169,10 @@ static int sys_clone(unsigned long clone_flags, unsigned long newsp,
 			   child_tidptr);
 }
 
+#define __STACK_SIZE (8 * 1024 * 1024)
+
 /*
- * If we clone with CLONE_SETTLS then the value in the parent should
+ * If we clone with CLONE_VM then the value in the parent should
  * be unchanged and the child should start with zero and be able to
  * set its own value.
  */
@@ -179,11 +181,19 @@ static int write_clone_read(void)
 	int parent_tid, child_tid;
 	pid_t parent, waiting;
 	int ret, status;
+	void *stack;
 
 	parent = getpid();
 	set_tpidr2(parent);
 
-	ret = sys_clone(CLONE_SETTLS, 0, &parent_tid, 0, &child_tid);
+	stack = malloc(__STACK_SIZE);
+	if (!stack) {
+		putstr("# malloc() failed\n");
+		return 0;
+	}
+
+	ret = sys_clone(CLONE_VM, (unsigned long)stack + __STACK_SIZE,
+			&parent_tid, 0, &child_tid);
 	if (ret == -1) {
 		putstr("# clone() failed\n");
 		putnum(errno);
-- 
2.30.2

Re: [PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Will Deacon]

On Tue, May 06, 2025 at 04:25:16PM +0100, Mark Rutland wrote:
> @@ -441,14 +449,39 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>  				childregs->sp = stack_start;
>  		}
>  
> +		/*
> +		 * Due to the AAPCS64 "ZA lazy saving scheme", PSTATE.ZA and
> +		 * TPIDR2 need to be manipulated as a pair, and either both
> +		 * need to be inherited or both need to be reset.
> +		 *
> +		 * Within a process, child threads must not inherit their
> +		 * parent's TPIDR2 value or they may clobber their parent's
> +		 * stack at some later point.
> +		 *
> +		 * When a process is fork()'d, the child must inherit ZA and
> +		 * TPIDR2 from its parent in case there was dormant ZA state.
> +		 *
> +		 * Use CLONE_VM to determine when the child will share the
> +		 * address space with the parent, and cannot safely inherit the
> +		 * state.
> +		 */
> +		if (system_supports_sme()) {
> +			if (!(clone_flags & CLONE_VM)) {
> +				p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);

Why do we need to re-read this register given that we did this just a few
lines earlier?

> diff --git a/tools/testing/selftests/arm64/abi/tpidr2.c b/tools/testing/selftests/arm64/abi/tpidr2.c
> index 285c47dd42f63..eb19dcc37a755 100644
> --- a/tools/testing/selftests/arm64/abi/tpidr2.c
> +++ b/tools/testing/selftests/arm64/abi/tpidr2.c
> @@ -169,8 +169,10 @@ static int sys_clone(unsigned long clone_flags, unsigned long newsp,
>  			   child_tidptr);
>  }
>  
> +#define __STACK_SIZE (8 * 1024 * 1024)
> +
>  /*
> - * If we clone with CLONE_SETTLS then the value in the parent should
> + * If we clone with CLONE_VM then the value in the parent should
>   * be unchanged and the child should start with zero and be able to
>   * set its own value.
>   */
> @@ -179,11 +181,19 @@ static int write_clone_read(void)
>  	int parent_tid, child_tid;
>  	pid_t parent, waiting;
>  	int ret, status;
> +	void *stack;
>  
>  	parent = getpid();
>  	set_tpidr2(parent);
>  
> -	ret = sys_clone(CLONE_SETTLS, 0, &parent_tid, 0, &child_tid);
> +	stack = malloc(__STACK_SIZE);
> +	if (!stack) {
> +		putstr("# malloc() failed\n");
> +		return 0;
> +	}
> +
> +	ret = sys_clone(CLONE_VM, (unsigned long)stack + __STACK_SIZE,
> +			&parent_tid, 0, &child_tid);
>  	if (ret == -1) {
>  		putstr("# clone() failed\n");
>  		putnum(errno);

Thank you for updating the selftest, but please can you put it in a
separate patch?

Cheers,

Will

Re: [PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Mark Rutland]

On Wed, May 07, 2025 at 03:58:01PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:16PM +0100, Mark Rutland wrote:
> > @@ -441,14 +449,39 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> >  				childregs->sp = stack_start;
> >  		}
> >  
> > +		/*
> > +		 * Due to the AAPCS64 "ZA lazy saving scheme", PSTATE.ZA and
> > +		 * TPIDR2 need to be manipulated as a pair, and either both
> > +		 * need to be inherited or both need to be reset.
> > +		 *
> > +		 * Within a process, child threads must not inherit their
> > +		 * parent's TPIDR2 value or they may clobber their parent's
> > +		 * stack at some later point.
> > +		 *
> > +		 * When a process is fork()'d, the child must inherit ZA and
> > +		 * TPIDR2 from its parent in case there was dormant ZA state.
> > +		 *
> > +		 * Use CLONE_VM to determine when the child will share the
> > +		 * address space with the parent, and cannot safely inherit the
> > +		 * state.
> > +		 */
> > +		if (system_supports_sme()) {
> > +			if (!(clone_flags & CLONE_VM)) {
> > +				p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> 
> Why do we need to re-read this register given that we did this just a few
> lines earlier?

Sorry -- I had meant to delete the earlier read. My intent was to centralise
manipulation of TPIDR2 (and ZA) in this block so that it was clear that they
were manipulated as a pair.

I will delete the earlier read, and make this:

| if (system_supports_sme()) {
| 	if (!(clone_flags & CLONE_VM)) {
| 		p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
| 		ret = copy_thread_za(p, current);
| 		if (ret)
| 			return ret;
| 	} else {
| 		p->thread.tpidr2_el0 = 0;
| 		WARN_ON_ONCE(p->thread.svcr & SVCR_ZA_MASK);
| 	}
| }

... or I can clear TPIDR2 in arch_dup_task_struct() along with ZA, delete the
earlier read here, and make this:

| if (system_supports_sme() && !(clone_flags & CLONE_VM)) {
| 	p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
| 	ret = copy_thread_za(p, current);
| 	if (ret)
| 		return ret;
| }

Any preference?

[...]

> > diff --git a/tools/testing/selftests/arm64/abi/tpidr2.c b/tools/testing/selftests/arm64/abi/tpidr2.c

> Thank you for updating the selftest, but please can you put it in a
> separate patch?

Sure; I'll factor all the selftest changes out into separate patches.

Mark.

Re: [PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Will Deacon]

On Wed, May 07, 2025 at 04:22:06PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 03:58:01PM +0100, Will Deacon wrote:
> > On Tue, May 06, 2025 at 04:25:16PM +0100, Mark Rutland wrote:
> > > @@ -441,14 +449,39 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> > >  				childregs->sp = stack_start;
> > >  		}
> > >  
> > > +		/*
> > > +		 * Due to the AAPCS64 "ZA lazy saving scheme", PSTATE.ZA and
> > > +		 * TPIDR2 need to be manipulated as a pair, and either both
> > > +		 * need to be inherited or both need to be reset.
> > > +		 *
> > > +		 * Within a process, child threads must not inherit their
> > > +		 * parent's TPIDR2 value or they may clobber their parent's
> > > +		 * stack at some later point.
> > > +		 *
> > > +		 * When a process is fork()'d, the child must inherit ZA and
> > > +		 * TPIDR2 from its parent in case there was dormant ZA state.
> > > +		 *
> > > +		 * Use CLONE_VM to determine when the child will share the
> > > +		 * address space with the parent, and cannot safely inherit the
> > > +		 * state.
> > > +		 */
> > > +		if (system_supports_sme()) {
> > > +			if (!(clone_flags & CLONE_VM)) {
> > > +				p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> > 
> > Why do we need to re-read this register given that we did this just a few
> > lines earlier?
> 
> Sorry -- I had meant to delete the earlier read. My intent was to centralise
> manipulation of TPIDR2 (and ZA) in this block so that it was clear that they
> were manipulated as a pair.
> 
> I will delete the earlier read, and make this:
> 
> | if (system_supports_sme()) {
> | 	if (!(clone_flags & CLONE_VM)) {
> | 		p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> | 		ret = copy_thread_za(p, current);
> | 		if (ret)
> | 			return ret;
> | 	} else {
> | 		p->thread.tpidr2_el0 = 0;

If we context-switch here, can we end up reading the register value
back into the thread structure?

> | 		WARN_ON_ONCE(p->thread.svcr & SVCR_ZA_MASK);
> | 	}
> | }
> 
> ... or I can clear TPIDR2 in arch_dup_task_struct() along with ZA, delete the
> earlier read here, and make this:
> 
> | if (system_supports_sme() && !(clone_flags & CLONE_VM)) {
> | 	p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> | 	ret = copy_thread_za(p, current);
> | 	if (ret)
> | 		return ret;
> | }
> 
> Any preference?

I don't mind, assuming they both work :)

Will

Re: [PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Mark Rutland]

On Wed, May 07, 2025 at 05:11:38PM +0100, Will Deacon wrote:
> On Wed, May 07, 2025 at 04:22:06PM +0100, Mark Rutland wrote:
> > On Wed, May 07, 2025 at 03:58:01PM +0100, Will Deacon wrote:
> > > On Tue, May 06, 2025 at 04:25:16PM +0100, Mark Rutland wrote:
> > > > @@ -441,14 +449,39 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> > > >  				childregs->sp = stack_start;
> > > >  		}
> > > >  
> > > > +		/*
> > > > +		 * Due to the AAPCS64 "ZA lazy saving scheme", PSTATE.ZA and
> > > > +		 * TPIDR2 need to be manipulated as a pair, and either both
> > > > +		 * need to be inherited or both need to be reset.
> > > > +		 *
> > > > +		 * Within a process, child threads must not inherit their
> > > > +		 * parent's TPIDR2 value or they may clobber their parent's
> > > > +		 * stack at some later point.
> > > > +		 *
> > > > +		 * When a process is fork()'d, the child must inherit ZA and
> > > > +		 * TPIDR2 from its parent in case there was dormant ZA state.
> > > > +		 *
> > > > +		 * Use CLONE_VM to determine when the child will share the
> > > > +		 * address space with the parent, and cannot safely inherit the
> > > > +		 * state.
> > > > +		 */
> > > > +		if (system_supports_sme()) {
> > > > +			if (!(clone_flags & CLONE_VM)) {
> > > > +				p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> > > 
> > > Why do we need to re-read this register given that we did this just a few
> > > lines earlier?
> > 
> > Sorry -- I had meant to delete the earlier read. My intent was to centralise
> > manipulation of TPIDR2 (and ZA) in this block so that it was clear that they
> > were manipulated as a pair.
> > 
> > I will delete the earlier read, and make this:
> > 
> > | if (system_supports_sme()) {
> > | 	if (!(clone_flags & CLONE_VM)) {
> > | 		p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> > | 		ret = copy_thread_za(p, current);
> > | 		if (ret)
> > | 			return ret;
> > | 	} else {
> > | 		p->thread.tpidr2_el0 = 0;
> 
> If we context-switch here, can we end up reading the register value
> back into the thread structure?

No; this is running in the context of the parent, and writing to the
child's task_struct, before the child is runnable. Nothing else is
concurrently reading or writing p->thread.tpidr2_el0.

In the case where we read the parent's TPIDR2 value, we ready the live
CPU register since that's switched eagerly in __switch_to() ->
tls_thread_switch(), and will not change under our feet.

> 
> > | 		WARN_ON_ONCE(p->thread.svcr & SVCR_ZA_MASK);
> > | 	}
> > | }
> > 
> > ... or I can clear TPIDR2 in arch_dup_task_struct() along with ZA, delete the
> > earlier read here, and make this:
> > 
> > | if (system_supports_sme() && !(clone_flags & CLONE_VM)) {
> > | 	p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> > | 	ret = copy_thread_za(p, current);
> > | 	if (ret)
> > | 		return ret;
> > | }
> > 
> > Any preference?
> 
> I don't mind, assuming they both work :)

Cool; I'll go with the first option for now.

Mark.

Re: [PATCH 13/20] arm64/fpsimd: Make clone() compatible with ZA lazy saving

[Yury Khrustalev]

On Tue, May 06, 2025 at 04:25:16PM +0100, Mark Rutland wrote:
> Linux is intended to be compatible with userspace written to Arm's
> AAPCS64 procedure call standard [1,2]. For the Scalable Matrix Extension
> (SME), AAPCS64 was extended with a "ZA lazy saving scheme", where SME's
> ZA tile is lazily callee-saved and caller-restored. In this scheme,
> TPIDR2_EL0 indicates whether the ZA tile is live or has been saved by
> pointing to a "TPIDR2 block" in memory, which has a "za_save_buffer"
> pointer. This scheme has been implemented in GCC and LLVM, with
> necessary runtime support implemented in glibc and bionic.
> 
> AAPCS64 does not specify how the ZA lazy saving scheme is expected to
> interact with thread creation mechanisms such as fork() and
> pthread_create(), which would be implemented in terms of the Linux clone
> syscall. The behaviour implemented by Linux and glibc/bionic doesn't
> always compose safely, as explained below.
> 
> Currently the clone syscall is implemented such that PSTATE.ZA and the
> ZA tile are always inherited by the new task, and TPIDR2_EL0 is
> inherited unless the 'flags' argument includes CLONE_SETTLS,
> in which case TPIDR2_EL0 is set to 0/NULL. This doesn't make much sense:
> 
> (a) TPIDR2_EL0 is part of the calling convention, and changes as control
>     is passed between functions. It is *NOT* used for thread local
>     storage, despite superficial similarity to TPIDR_EL0, which is is
>     used as the TLS register.
> 
> (b) TPIDR2_EL0 and PSTATE.ZA are tightly coupled in the procedure call
>     standard, and some combinations of states are illegal. In general,
>     manipulating the two independently is not guaranteed to be safe.
> 
> In practice, code which is compliant with the procedure call standard
> may issue a clone syscall while in the "ZA dormant" state, where
> PSTATE.ZA==1 and TPIDR2_EL0 is non-null and indicates that ZA needs to
> be saved. This can cause a variety of problems, including:
> 
> * If the implementation of pthread_create() passes CLONE_SETTLS, the
>   new thread will start with PSTATE.ZA==1 and TPIDR2==NULL. Per the
>   procedure call standard this is not a legitimate state for most
>   functions. This can cause data corruption (e.g. as code may rely on
>   PSTATE.ZA being 0 to guarantee that an SMSTART ZA instruction will
>   zero the ZA tile contents), and may result in other undefined
>   behaviour.
> 
> * If the implementation of pthread_create() does not pass CLONE_SETTLS, the
>   new thread will start with PSTATE.ZA==1 and TPIDR2 pointing to a
>   TPIDR2 block on the parent thread's stack. This can result in a
>   variety of problems, e.g.
> 
>   - The child may write back to the parent's za_save_buffer, corrupting
>     its contents.
> 
>   - The child may read from the TPIDR2 block after the parent has reused
>     this memory for something else, and consequently the child may abort
>     or clobber arbitrary memory.
> 
> Ideally we'd require that userspace ensures that a task is in the "ZA
> off" state (with PSTATE.ZA==0 and TPIDR2_EL0==NULL) prior to issuing a
> clone syscall, and have the kernel force this state for new threads.
> Unfortunately, contemporary C libraries do not do this, and simply
> forcing this state within the implementation of clone would break
> fork().
> 
> Instead, we can bodge around this by considering the CLONE_VM flag, and
> mainpulate PSTATE.ZA and TPIDR2_EL0 as a pair. CLONE_VM indicates that
> the new task will run in the same address space as its parent, and in
> that case it doesn't make sense to inherit a stale pointer to the
> parent's TPIDR2 block:
> 
> * For fork(), CLONE_VM will not be set, and it is safe to inherit both
>   PSTATE.ZA and TPIDR2_EL0 as the new task will have its own copy of the
>   address space, and cannot clobber its parent's stack.
> 
> * For pthread_create() and vfork(), CLONE_VM will be set, and discarding
>   PSTATE.ZA and TPIDR2_EL0 for the new task doesn't break any existing
>   assumptions in userspace.

Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>

Kind regards,
Yury

[PATCH 14/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes do not resurrect stale data

[Mark Rutland]

The SVE/SME vector lengths can be changed via prctl/ptrace syscalls.
Changes to the SVE/SME vector lengths are documented as preserving the
lower 128 bits of the Z registers (i.e. the bits shared with the FPSIMD
V registers). To ensure this, vec_set_vector_length() explicitly copies
register values from a task's saved SVE state to its saved FPSIMD state
when dropping the task to FPSIMD-only.

The logic for this was not updated when when FPSIMD/SVE state tracking
was changed across commits:

  baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
  a0136be443d5 (arm64/fpsimd: Load FP state based on recorded data type")
  bbc6172eefdb ("arm64/fpsimd: SME no longer requires SVE register state")
  8c845e273104 ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")

Since the last commit above, a task's FPSIMD/SVE state may be stored in
FPSIMD format while TIF_SVE is set, and the stored SVE state is stale.
When vec_set_vector_length() encounters this case, it will erroneously
clobber the live FPSIMD state with stale SVE state by using
sve_to_fpsimd().

Fix this by using fpsimd_sync_from_effective_state() instead.

Related issues with streaming mode state will be addressed in subsequent
patches.

Fixes: 8c845e273104 ("arm64/sve: Leave SVE enabled on syscall if we don't context switch")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index fe96e018e18c0..faeedaab0558e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -852,7 +852,7 @@ int vec_set_vector_length(struct task_struct *task, enum vec_type type,
 	fpsimd_flush_task_state(task);
 	if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
 	    thread_sm_enabled(&task->thread)) {
-		sve_to_fpsimd(task);
+		fpsimd_sync_from_effective_state(task);
 		task->thread.fp_type = FP_STATE_FPSIMD;
 	}
 
-- 
2.30.2

[PATCH 15/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid state

[Mark Rutland]

Currently, vec_set_vector_length() can manipulate a task into an invalid
state as a result of a prctl/ptrace syscall which changes the SVE/SME
vector length, resulting in several problems:

(1) When changing the SVE vector length, if the task initially has
    PSTATE.ZA==1, and sve_alloc() fails to allocate memory, the task
    will be left with PSTATE.ZA==1 and sve_state==NULL. This is not a
    legitimate state, and could result in a subsequent null pointer
    dereference.

(2) When changing the SVE vector length, if the task initially has
    PSTATE.SM==1, the task will be left with PSTATE.SM==1 and
    fp_type==FP_STATE_FPSIMD. Streaming mode state always needs to be
    saved in SVE format, so this is not a legitimate state.

    Attempting to restore this state may cause a task to erroneously
    inherit stale streaming mode predicate registers and FFR contents,
    behaving non-deterministically and potentially leaving information
    from another task.

    While in this state, reads of the NT_ARM_SSVE regset will indicate
    that the registers are not stored in SVE format. For the NT_ARM_SSVE
    regset specifically, debuggers interpret this as meaning that
    PSTATE.SM==0.

(3) When changing the SME vector length, if the task initially has
    PSTATE.SM==1, the lower 128 bits of task's streaming mode vector
    state will be migrated to non-streaming mode, rather than these bits
    being zeroed as is usually the case for changes to PSTATE.SM.

To fix the first issue, we can eagerly allocate the new sve_state and
sme_state before modifying the task. This makes it possible to handle
memory allocation failure without modifying the task state at all, and
removes the need to clear TIF_SVE and TIF_SME.

To fix the second issue, we either need to clear PSTATE.SM or not change
the saved fp_type. Given we're going to eagerly allocate sve_state and
sme_state, the simplest option is to preserve PSTATE.SM and the saves
fp_type, and consistently truncate the SVE state. This ensures that the
task always stays in a valid state.

I believe these changes should not be problematic for realistic usage:

* When the SVE/SME vector length is changed via prctl(), syscall entry
  will have cleared PSTATE.SM. Unless the task's state has been
  manipulated via ptrace after entry, the task will have PSTATE.SM==0.

* When the SVE/SME vector length is changed via a write to the
  NT_ARM_SVE or NT_ARM_SSVE regsets, PSTATE.SM will be forced
  immediately after the length change, and new vector state will be
  copied from userspace.

* When the SME vector length is changed via a write to the NT_ARM_ZA
  regset, the (S)SVE state is clobbered today, so anyone who cares about
  the specific state would need to install this after writing to the
  NT_ARM_ZA regset.

The fp-ptrace test checks for this last behaviour, so I've updated it to
match the change to the kernel.

As we need to free the old SVE state while TIF_SVE may still be set, we
cannot use sve_free(), and using kfree() directly makes it clear that
the free pairs with the subsequent assignment. As this leaves sve_free()
unused, I've removed the existing sve_free() and renamed __sve_free() to
mirror sme_free().

Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")
Fixes: baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 Documentation/arch/arm64/sme.rst             |   2 +-
 arch/arm64/kernel/fpsimd.c                   | 137 ++++++++++---------
 tools/testing/selftests/arm64/fp/fp-ptrace.c |  18 +--
 3 files changed, 76 insertions(+), 81 deletions(-)

diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
index 1c1e48d8bd1a5..4cb38330e7046 100644
--- a/Documentation/arch/arm64/sme.rst
+++ b/Documentation/arch/arm64/sme.rst
@@ -241,7 +241,7 @@ prctl(PR_SME_SET_VL, unsigned long arg)
       length, or calling PR_SME_SET_VL with the PR_SME_SET_VL_ONEXEC flag,
       does not constitute a change to the vector length for this purpose.
 
-    * Changing the vector length causes PSTATE.ZA and PSTATE.SM to be cleared.
+    * Changing the vector length causes PSTATE.ZA to be cleared.
       Calling PR_SME_SET_VL with vl equal to the thread's current vector
       length, or calling PR_SME_SET_VL with the PR_SME_SET_VL_ONEXEC flag,
       does not constitute a change to the vector length for this purpose.
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index faeedaab0558e..b868c33795ba5 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -724,23 +724,12 @@ void cpu_enable_fpmr(const struct arm64_cpu_capabilities *__always_unused p)
 }
 
 #ifdef CONFIG_ARM64_SVE
-/*
- * Call __sve_free() directly only if you know task can't be scheduled
- * or preempted.
- */
-static void __sve_free(struct task_struct *task)
+static void sve_free(struct task_struct *task)
 {
 	kfree(task->thread.sve_state);
 	task->thread.sve_state = NULL;
 }
 
-static void sve_free(struct task_struct *task)
-{
-	WARN_ON(test_tsk_thread_flag(task, TIF_SVE));
-
-	__sve_free(task);
-}
-
 /*
  * Ensure that task->thread.sve_state is allocated and sufficiently large.
  *
@@ -801,10 +790,73 @@ void fpsimd_sync_to_effective_state_zeropad(struct task_struct *task)
 	__fpsimd_to_sve(sst, fst, vq);
 }
 
+static int change_live_vector_length(struct task_struct *task,
+				     enum vec_type type,
+				     unsigned long vl)
+{
+	unsigned int sve_vl = task_get_sve_vl(task);
+	unsigned int sme_vl = task_get_sme_vl(task);
+	void *sve_state = NULL, *sme_state = NULL;
+
+	if (type == ARM64_VEC_SME)
+		sme_vl = vl;
+	else
+		sve_vl = vl;
+
+	/*
+	 * Allocate the new sve_state and sme_state before freeing the old
+	 * copies so that allocation failure can be handled without needing to
+	 * mutate the task's state in any way.
+	 *
+	 * Changes to the SVE vector length must not discard live ZA state or
+	 * clear PSTATE.ZA, as userspace code which is unaware of the AAPCS64
+	 * ZA lazy saving scheme may attempt to change the SVE vector length
+	 * while unsaved/dormant ZA state exists.
+	 */
+	sve_state = kzalloc(__sve_state_size(sve_vl, sme_vl), GFP_KERNEL);
+	if (!sve_state)
+		goto out_mem;
+
+	if (type == ARM64_VEC_SME) {
+		sme_state = kzalloc(__sme_state_size(sme_vl), GFP_KERNEL);
+		if (!sve_state)
+			goto out_mem;
+	}
+
+	if (task == current)
+		fpsimd_save_and_flush_current_state();
+	else
+		fpsimd_flush_task_state(task);
+
+	/*
+	 * Always preserve PSTATE.SM and the effective FPSIMD state, zeroing
+	 * other SVE state.
+	 */
+	fpsimd_sync_from_effective_state(task);
+	task_set_vl(task, type, vl);
+	kfree(task->thread.sve_state);
+	task->thread.sve_state = sve_state;
+	fpsimd_sync_to_effective_state_zeropad(task);
+
+	if (type == ARM64_VEC_SME) {
+		task->thread.svcr &= ~SVCR_ZA_MASK;
+		kfree(task->thread.sme_state);
+		task->thread.sme_state = sme_state;
+	}
+
+	return 0;
+
+out_mem:
+	kfree(sve_state);
+	kfree(sme_state);
+	return -ENOMEM;
+}
+
 int vec_set_vector_length(struct task_struct *task, enum vec_type type,
 			  unsigned long vl, unsigned long flags)
 {
-	bool free_sme = false;
+	bool onexec = flags & PR_SVE_SET_VL_ONEXEC;
+	bool inherit = flags & PR_SVE_VL_INHERIT;
 
 	if (flags & ~(unsigned long)(PR_SVE_VL_INHERIT |
 				     PR_SVE_SET_VL_ONEXEC))
@@ -824,62 +876,17 @@ int vec_set_vector_length(struct task_struct *task, enum vec_type type,
 
 	vl = find_supported_vector_length(type, vl);
 
-	if (flags & (PR_SVE_VL_INHERIT |
-		     PR_SVE_SET_VL_ONEXEC))
+	if (!onexec && vl != task_get_vl(task, type)) {
+		if (change_live_vector_length(task, type, vl))
+			return -ENOMEM;
+	}
+
+	if (onexec || inherit)
 		task_set_vl_onexec(task, type, vl);
 	else
 		/* Reset VL to system default on next exec: */
 		task_set_vl_onexec(task, type, 0);
 
-	/* Only actually set the VL if not deferred: */
-	if (flags & PR_SVE_SET_VL_ONEXEC)
-		goto out;
-
-	if (vl == task_get_vl(task, type))
-		goto out;
-
-	/*
-	 * To ensure the FPSIMD bits of the SVE vector registers are preserved,
-	 * write any live register state back to task_struct, and convert to a
-	 * regular FPSIMD thread.
-	 */
-	if (task == current) {
-		get_cpu_fpsimd_context();
-
-		fpsimd_save_user_state();
-	}
-
-	fpsimd_flush_task_state(task);
-	if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
-	    thread_sm_enabled(&task->thread)) {
-		fpsimd_sync_from_effective_state(task);
-		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_tsk_thread_flag(task, TIF_SME);
-		free_sme = true;
-	}
-
-	if (task == current)
-		put_cpu_fpsimd_context();
-
-	task_set_vl(task, type, vl);
-
-	/*
-	 * 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);
-	sve_alloc(task, true);
-
-	if (free_sme)
-		sme_free(task);
-
-out:
 	update_tsk_thread_flag(task, vec_vl_inherit_flag(type),
 			       flags & PR_SVE_VL_INHERIT);
 
@@ -1175,7 +1182,7 @@ void __init sve_setup(void)
  */
 void fpsimd_release_task(struct task_struct *dead_task)
 {
-	__sve_free(dead_task);
+	sve_free(dead_task);
 	sme_free(dead_task);
 }
 
diff --git a/tools/testing/selftests/arm64/fp/fp-ptrace.c b/tools/testing/selftests/arm64/fp/fp-ptrace.c
index 762048eb354ff..c2882a5a5cc0a 100644
--- a/tools/testing/selftests/arm64/fp/fp-ptrace.c
+++ b/tools/testing/selftests/arm64/fp/fp-ptrace.c
@@ -1183,18 +1183,8 @@ static void sve_write(pid_t child, struct test_config *config)
 
 static bool za_write_supported(struct test_config *config)
 {
-	if (config->sme_vl_in != config->sme_vl_expected) {
-		/* Changing the SME VL exits streaming mode. */
-		if (config->svcr_expected & SVCR_SM) {
-			return false;
-		}
-	} else {
-		/* Otherwise we can't change streaming mode */
-		if ((config->svcr_in & SVCR_SM) !=
-		    (config->svcr_expected & SVCR_SM)) {
-			return false;
-		}
-	}
+	if ((config->svcr_in & SVCR_SM) != (config->svcr_expected & SVCR_SM))
+		return false;
 
 	return true;
 }
@@ -1212,10 +1202,8 @@ static void za_write_expected(struct test_config *config)
 		memset(zt_expected, 0, sizeof(zt_expected));
 	}
 
-	/* Changing the SME VL flushes ZT, SVE state and exits SM */
+	/* Changing the SME VL flushes ZT, SVE state */
 	if (config->sme_vl_in != config->sme_vl_expected) {
-		svcr_expected &= ~SVCR_SM;
-
 		sve_vq = __sve_vq_from_vl(vl_expected(config));
 		memset(z_expected, 0, __SVE_ZREGS_SIZE(sve_vq));
 		memset(p_expected, 0, __SVE_PREGS_SIZE(sve_vq));
-- 
2.30.2

Re: [PATCH 15/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid state

[Will Deacon]

On Tue, May 06, 2025 at 04:25:18PM +0100, Mark Rutland wrote:
> Currently, vec_set_vector_length() can manipulate a task into an invalid
> state as a result of a prctl/ptrace syscall which changes the SVE/SME
> vector length, resulting in several problems:
> 
> (1) When changing the SVE vector length, if the task initially has
>     PSTATE.ZA==1, and sve_alloc() fails to allocate memory, the task
>     will be left with PSTATE.ZA==1 and sve_state==NULL. This is not a
>     legitimate state, and could result in a subsequent null pointer
>     dereference.
> 
> (2) When changing the SVE vector length, if the task initially has
>     PSTATE.SM==1, the task will be left with PSTATE.SM==1 and
>     fp_type==FP_STATE_FPSIMD. Streaming mode state always needs to be
>     saved in SVE format, so this is not a legitimate state.
> 
>     Attempting to restore this state may cause a task to erroneously
>     inherit stale streaming mode predicate registers and FFR contents,
>     behaving non-deterministically and potentially leaving information
>     from another task.
> 
>     While in this state, reads of the NT_ARM_SSVE regset will indicate
>     that the registers are not stored in SVE format. For the NT_ARM_SSVE
>     regset specifically, debuggers interpret this as meaning that
>     PSTATE.SM==0.
> 
> (3) When changing the SME vector length, if the task initially has
>     PSTATE.SM==1, the lower 128 bits of task's streaming mode vector
>     state will be migrated to non-streaming mode, rather than these bits
>     being zeroed as is usually the case for changes to PSTATE.SM.
> 
> To fix the first issue, we can eagerly allocate the new sve_state and
> sme_state before modifying the task. This makes it possible to handle
> memory allocation failure without modifying the task state at all, and
> removes the need to clear TIF_SVE and TIF_SME.
> 
> To fix the second issue, we either need to clear PSTATE.SM or not change
> the saved fp_type. Given we're going to eagerly allocate sve_state and
> sme_state, the simplest option is to preserve PSTATE.SM and the saves
> fp_type, and consistently truncate the SVE state. This ensures that the
> task always stays in a valid state.
> 
> I believe these changes should not be problematic for realistic usage:
> 
> * When the SVE/SME vector length is changed via prctl(), syscall entry
>   will have cleared PSTATE.SM. Unless the task's state has been
>   manipulated via ptrace after entry, the task will have PSTATE.SM==0.
> 
> * When the SVE/SME vector length is changed via a write to the
>   NT_ARM_SVE or NT_ARM_SSVE regsets, PSTATE.SM will be forced
>   immediately after the length change, and new vector state will be
>   copied from userspace.
> 
> * When the SME vector length is changed via a write to the NT_ARM_ZA
>   regset, the (S)SVE state is clobbered today, so anyone who cares about
>   the specific state would need to install this after writing to the
>   NT_ARM_ZA regset.
> 
> The fp-ptrace test checks for this last behaviour, so I've updated it to
> match the change to the kernel.
> 
> As we need to free the old SVE state while TIF_SVE may still be set, we
> cannot use sve_free(), and using kfree() directly makes it clear that
> the free pairs with the subsequent assignment. As this leaves sve_free()
> unused, I've removed the existing sve_free() and renamed __sve_free() to
> mirror sme_free().
> 
> Fixes: 8bd7f91c03d8 ("arm64/sme: Implement traps and syscall handling for SME")
> Fixes: baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David Spickett <david.spickett@arm.com>
> Cc: Luis Machado <luis.machado@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  Documentation/arch/arm64/sme.rst             |   2 +-
>  arch/arm64/kernel/fpsimd.c                   | 137 ++++++++++---------
>  tools/testing/selftests/arm64/fp/fp-ptrace.c |  18 +--
>  3 files changed, 76 insertions(+), 81 deletions(-)

[...]

> +static int change_live_vector_length(struct task_struct *task,
> +				     enum vec_type type,
> +				     unsigned long vl)
> +{
> +	unsigned int sve_vl = task_get_sve_vl(task);
> +	unsigned int sme_vl = task_get_sme_vl(task);
> +	void *sve_state = NULL, *sme_state = NULL;
> +
> +	if (type == ARM64_VEC_SME)
> +		sme_vl = vl;
> +	else
> +		sve_vl = vl;
> +
> +	/*
> +	 * Allocate the new sve_state and sme_state before freeing the old
> +	 * copies so that allocation failure can be handled without needing to
> +	 * mutate the task's state in any way.
> +	 *
> +	 * Changes to the SVE vector length must not discard live ZA state or
> +	 * clear PSTATE.ZA, as userspace code which is unaware of the AAPCS64
> +	 * ZA lazy saving scheme may attempt to change the SVE vector length
> +	 * while unsaved/dormant ZA state exists.
> +	 */
> +	sve_state = kzalloc(__sve_state_size(sve_vl, sme_vl), GFP_KERNEL);
> +	if (!sve_state)
> +		goto out_mem;
> +
> +	if (type == ARM64_VEC_SME) {
> +		sme_state = kzalloc(__sme_state_size(sme_vl), GFP_KERNEL);
> +		if (!sve_state)

This should be '!sme_state'.

> +			goto out_mem;
> +	}
> +
> +	if (task == current)
> +		fpsimd_save_and_flush_current_state();
> +	else
> +		fpsimd_flush_task_state(task);
> +
> +	/*
> +	 * Always preserve PSTATE.SM and the effective FPSIMD state, zeroing
> +	 * other SVE state.
> +	 */
> +	fpsimd_sync_from_effective_state(task);
> +	task_set_vl(task, type, vl);
> +	kfree(task->thread.sve_state);
> +	task->thread.sve_state = sve_state;
> +	fpsimd_sync_to_effective_state_zeropad(task);
> +
> +	if (type == ARM64_VEC_SME) {
> +		task->thread.svcr &= ~SVCR_ZA_MASK;
> +		kfree(task->thread.sme_state);
> +		task->thread.sme_state = sme_state;
> +	}

I'm probably just missing something here, but how does this address
problem three from the commit message where exiting streaming mode
should zero the bottom bits of the vector registers?

Will

Re: [PATCH 15/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid state

[Mark Rutland]

On Wed, May 07, 2025 at 05:12:47PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:18PM +0100, Mark Rutland wrote:
> > Currently, vec_set_vector_length() can manipulate a task into an invalid
> > state as a result of a prctl/ptrace syscall which changes the SVE/SME
> > vector length, resulting in several problems:
> > 
> > (1) When changing the SVE vector length, if the task initially has
> >     PSTATE.ZA==1, and sve_alloc() fails to allocate memory, the task
> >     will be left with PSTATE.ZA==1 and sve_state==NULL. This is not a
> >     legitimate state, and could result in a subsequent null pointer
> >     dereference.
> > 
> > (2) When changing the SVE vector length, if the task initially has
> >     PSTATE.SM==1, the task will be left with PSTATE.SM==1 and
> >     fp_type==FP_STATE_FPSIMD. Streaming mode state always needs to be
> >     saved in SVE format, so this is not a legitimate state.
> > 
> >     Attempting to restore this state may cause a task to erroneously
> >     inherit stale streaming mode predicate registers and FFR contents,
> >     behaving non-deterministically and potentially leaving information
> >     from another task.
> > 
> >     While in this state, reads of the NT_ARM_SSVE regset will indicate
> >     that the registers are not stored in SVE format. For the NT_ARM_SSVE
> >     regset specifically, debuggers interpret this as meaning that
> >     PSTATE.SM==0.
> > 
> > (3) When changing the SME vector length, if the task initially has
> >     PSTATE.SM==1, the lower 128 bits of task's streaming mode vector
> >     state will be migrated to non-streaming mode, rather than these bits
> >     being zeroed as is usually the case for changes to PSTATE.SM.
> > 
> > To fix the first issue, we can eagerly allocate the new sve_state and
> > sme_state before modifying the task. This makes it possible to handle
> > memory allocation failure without modifying the task state at all, and
> > removes the need to clear TIF_SVE and TIF_SME.
> > 
> > To fix the second issue, we either need to clear PSTATE.SM or not change
> > the saved fp_type. Given we're going to eagerly allocate sve_state and
> > sme_state, the simplest option is to preserve PSTATE.SM and the saves
> > fp_type, and consistently truncate the SVE state. This ensures that the
> > task always stays in a valid state.
> > 
> > I believe these changes should not be problematic for realistic usage:
> > 
> > * When the SVE/SME vector length is changed via prctl(), syscall entry
> >   will have cleared PSTATE.SM. Unless the task's state has been
> >   manipulated via ptrace after entry, the task will have PSTATE.SM==0.
> > 
> > * When the SVE/SME vector length is changed via a write to the
> >   NT_ARM_SVE or NT_ARM_SSVE regsets, PSTATE.SM will be forced
> >   immediately after the length change, and new vector state will be
> >   copied from userspace.
> > 
> > * When the SME vector length is changed via a write to the NT_ARM_ZA
> >   regset, the (S)SVE state is clobbered today, so anyone who cares about
> >   the specific state would need to install this after writing to the
> >   NT_ARM_ZA regset.

[...]

> > +static int change_live_vector_length(struct task_struct *task,
> > +				     enum vec_type type,
> > +				     unsigned long vl)
> > +{
> > +	unsigned int sve_vl = task_get_sve_vl(task);
> > +	unsigned int sme_vl = task_get_sme_vl(task);
> > +	void *sve_state = NULL, *sme_state = NULL;
> > +
> > +	if (type == ARM64_VEC_SME)
> > +		sme_vl = vl;
> > +	else
> > +		sve_vl = vl;
> > +
> > +	/*
> > +	 * Allocate the new sve_state and sme_state before freeing the old
> > +	 * copies so that allocation failure can be handled without needing to
> > +	 * mutate the task's state in any way.
> > +	 *
> > +	 * Changes to the SVE vector length must not discard live ZA state or
> > +	 * clear PSTATE.ZA, as userspace code which is unaware of the AAPCS64
> > +	 * ZA lazy saving scheme may attempt to change the SVE vector length
> > +	 * while unsaved/dormant ZA state exists.
> > +	 */
> > +	sve_state = kzalloc(__sve_state_size(sve_vl, sme_vl), GFP_KERNEL);
> > +	if (!sve_state)
> > +		goto out_mem;
> > +
> > +	if (type == ARM64_VEC_SME) {
> > +		sme_state = kzalloc(__sme_state_size(sme_vl), GFP_KERNEL);
> > +		if (!sve_state)
> 
> This should be '!sme_state'.

Ugh, yes. Fixed now, thanks.

[...]

> > +	/*
> > +	 * Always preserve PSTATE.SM and the effective FPSIMD state, zeroing
> > +	 * other SVE state.
> > +	 */
> > +	fpsimd_sync_from_effective_state(task);
> > +	task_set_vl(task, type, vl);
> > +	kfree(task->thread.sve_state);
> > +	task->thread.sve_state = sve_state;
> > +	fpsimd_sync_to_effective_state_zeropad(task);
> > +
> > +	if (type == ARM64_VEC_SME) {
> > +		task->thread.svcr &= ~SVCR_ZA_MASK;
> > +		kfree(task->thread.sme_state);
> > +		task->thread.sme_state = sme_state;
> > +	}
> 
> I'm probably just missing something here, but how does this address
> problem three from the commit message where exiting streaming mode
> should zero the bottom bits of the vector registers?

The idea is that we no longer exit streaming mode, so we don't need to zero the
bottom bits.

Instead, when either the SVE or SME vector length changes, we
consitently truncate the (streaming or non-streaming) SVE registers to
128-bits, but preserve the existing value of PSTATE.SM and the saved
fp_type.

That all happens in:

	fpsimd_sync_from_effective_state(task);
	task_set_vl(task, type, vl);
	kfree(task->thread.sve_state);
	task->thread.sve_state = sve_state;
	fpsimd_sync_to_effective_state_zeropad(task);

... where:

* If the task's state was initially stored in FPSIMD format, the
  fpsimd_sync_from_effective_state() and
  fpsimd_sync_to_effective_state_zeropad() functions do not touch the
  saved FPSIMD state, leaving the low 128 bits intact.

* If the task's state was initially stored in SVE format, whether
  streaming or non-streaming, the fpsimd_sync_from_effective_state() and
  fpsimd_sync_to_effective_state_zeropad() functions copy the low 128
  bits into the FPSIMD storage, then copy that back into the new zeroed
  SVE storage.

That's what I was trying to describe in the commit message where I said:

| To fix the second issue, we either need to clear PSTATE.SM or not change
| the saved fp_type. Given we're going to eagerly allocate sve_state and
| sme_state, the simplest option is to preserve PSTATE.SM and the saves
| fp_type, and consistently truncate the SVE state. This ensures that the
| task always stays in a valid state.

... but I appreciate that mentioned the second issue and not the third.

I can add a note there, if it'd help?

Mark.

Re: [PATCH 15/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid state

[Will Deacon]

On Wed, May 07, 2025 at 06:10:07PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 05:12:47PM +0100, Will Deacon wrote:
> > On Tue, May 06, 2025 at 04:25:18PM +0100, Mark Rutland wrote:
> > > +	/*
> > > +	 * Always preserve PSTATE.SM and the effective FPSIMD state, zeroing
> > > +	 * other SVE state.
> > > +	 */
> > > +	fpsimd_sync_from_effective_state(task);
> > > +	task_set_vl(task, type, vl);
> > > +	kfree(task->thread.sve_state);
> > > +	task->thread.sve_state = sve_state;
> > > +	fpsimd_sync_to_effective_state_zeropad(task);
> > > +
> > > +	if (type == ARM64_VEC_SME) {
> > > +		task->thread.svcr &= ~SVCR_ZA_MASK;
> > > +		kfree(task->thread.sme_state);
> > > +		task->thread.sme_state = sme_state;
> > > +	}
> > 
> > I'm probably just missing something here, but how does this address
> > problem three from the commit message where exiting streaming mode
> > should zero the bottom bits of the vector registers?
> 
> The idea is that we no longer exit streaming mode, so we don't need to zero the
> bottom bits.

Argh, I mis-read the svcr manipulation above as clearing SM rather than
ZA. Now that makes a lot more sense, thanks!

> Instead, when either the SVE or SME vector length changes, we
> consitently truncate the (streaming or non-streaming) SVE registers to
> 128-bits, but preserve the existing value of PSTATE.SM and the saved
> fp_type.
> 
> That all happens in:
> 
> 	fpsimd_sync_from_effective_state(task);
> 	task_set_vl(task, type, vl);
> 	kfree(task->thread.sve_state);
> 	task->thread.sve_state = sve_state;
> 	fpsimd_sync_to_effective_state_zeropad(task);
> 
> ... where:
> 
> * If the task's state was initially stored in FPSIMD format, the
>   fpsimd_sync_from_effective_state() and
>   fpsimd_sync_to_effective_state_zeropad() functions do not touch the
>   saved FPSIMD state, leaving the low 128 bits intact.
> 
> * If the task's state was initially stored in SVE format, whether
>   streaming or non-streaming, the fpsimd_sync_from_effective_state() and
>   fpsimd_sync_to_effective_state_zeropad() functions copy the low 128
>   bits into the FPSIMD storage, then copy that back into the new zeroed
>   SVE storage.
> 
> That's what I was trying to describe in the commit message where I said:
> 
> | To fix the second issue, we either need to clear PSTATE.SM or not change
> | the saved fp_type. Given we're going to eagerly allocate sve_state and
> | sme_state, the simplest option is to preserve PSTATE.SM and the saves
> | fp_type, and consistently truncate the SVE state. This ensures that the
> | task always stays in a valid state.
> 
> ... but I appreciate that mentioned the second issue and not the third.
> 
> I can add a note there, if it'd help?

Nah, I just need to learn to read properly.

Will

[PATCH 16/20] arm64/fpsimd: ptrace: Save task state before generating SVE header

[Mark Rutland]

As sve_init_header_from_task() consumes the saved value of PSTATE.SM and
the saved fp_type, both must be saved before the header is generated.

When generating a coredump for the current task, sve_get_common() calls
sve_init_header_from_task() before saving the task's state. Consequently
the header may be bogus, and the contents of the regset may be
misleading.

Fix this by saving the task's state before generting the header.

Fixes: e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")
Fixes: b017a0cea627 ("arm64/ptrace: Use saved floating point state type to determine SVE layout")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ptrace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index bdba106a4cf29..67f3843de51f5 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -820,15 +820,15 @@ static int sve_get_common(struct task_struct *target,
 	unsigned int vq;
 	unsigned long start, end;
 
+	if (target == current)
+		fpsimd_preserve_current_state();
+
 	/* Header */
 	sve_init_header_from_task(&header, target, type);
 	vq = sve_vq_from_vl(header.vl);
 
 	membuf_write(&to, &header, sizeof(header));
 
-	if (target == current)
-		fpsimd_preserve_current_state();
-
 	BUILD_BUG_ON(SVE_PT_FPSIMD_OFFSET != sizeof(header));
 	BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header));
 
-- 
2.30.2

[PATCH 17/20] arm64/fpsimd: ptrace: Do not present register data for inactive mode

[Mark Rutland]

The SME ptrace ABI is written around the incorrect assumption that
SVE_PT_REGS_FPSIMD and SVE_PT_REGS_SVE are independent bit flags, where
it is possible for both to be clear. In reality they are different
values for bit 0 of the header flags, where SVE_PT_REGS_FPSIMD is 0 and
SVE_PT_REGS_SVE is 1. In cases where code was writtten expecting that
neither bit flag would be set, the value is equivalent to
SVE_PT_REGS_FPSIMD.

One consequence of this is that reads of the NT_ARM_SVE or NT_ARM_SSVE
will erroneously present data from the other mode:

* When PSTATE.SM==1, reads of NT_ARM_SVE will present a header with
  SVE_PT_REGS_FPSIMD, and FPSIMD-formatted data from streaming mode.

* When PSTATE.SM==0, reads of NT_ARM_SSVE will present a header with
  SVE_PT_REGS_FPSIMD, and FPSIMD-formatted data from non-streaming mode.

The original intent was that no register data would be provided in these
cases, as described in commit:

  e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")

Luckily, debuggers do not consume the bogus register data. Both GDB and
LLDB read the NT_ARM_SSVE regset before the NT_ARM_SVE regset, and
assume that when the NT_ARM_SSVE header presents SVE_PT_REGS_FPSIMD, it
is necessary to read register contents from the NT_ARM_SVE regset,
regardless of whether the NT_ARM_SSVE regset porivded bogus register
data.

Fix the code to stop presenting register data from the inactive mode,
and update the fp-ptrace test to match. At the same time, make the
manipulation of the flag clearer, and remove the bogus comment from
sve_set_common(). I've given this a quick spin with GDB and LLDB, and
both seem happy.

Fixes: e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ptrace.c                   | 32 +++++++++++---------
 tools/testing/selftests/arm64/fp/fp-ptrace.c | 30 +++++++++++++-----
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 67f3843de51f5..a2075e1df27c6 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -775,6 +775,11 @@ static void sve_init_header_from_task(struct user_sve_header *header,
 		task_type = ARM64_VEC_SVE;
 	active = (task_type == type);
 
+	if (active && target->thread.fp_type == FP_STATE_SVE)
+		header->flags = SVE_PT_REGS_SVE;
+	else
+		header->flags = SVE_PT_REGS_FPSIMD;
+
 	switch (type) {
 	case ARM64_VEC_SVE:
 		if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT))
@@ -789,19 +794,14 @@ static void sve_init_header_from_task(struct user_sve_header *header,
 		return;
 	}
 
-	if (active) {
-		if (target->thread.fp_type == FP_STATE_FPSIMD) {
-			header->flags |= SVE_PT_REGS_FPSIMD;
-		} else {
-			header->flags |= SVE_PT_REGS_SVE;
-		}
-	}
-
 	header->vl = task_get_vl(target, type);
 	vq = sve_vq_from_vl(header->vl);
 
 	header->max_vl = vec_max_vl(type);
-	header->size = SVE_PT_SIZE(vq, header->flags);
+	if (active)
+		header->size = SVE_PT_SIZE(vq, header->flags);
+	else
+		header->size = sizeof(header);
 	header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
 				      SVE_PT_REGS_SVE);
 }
@@ -832,6 +832,13 @@ static int sve_get_common(struct task_struct *target,
 	BUILD_BUG_ON(SVE_PT_FPSIMD_OFFSET != sizeof(header));
 	BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header));
 
+	/*
+	 * When the requested vector type is not active, do not present data
+	 * from the other mode to userspace.
+	 */
+	if (header.size == sizeof(header))
+		return 0;
+
 	switch ((header.flags & SVE_PT_REGS_MASK)) {
 	case SVE_PT_REGS_FPSIMD:
 		return __fpr_get(target, regset, to);
@@ -859,7 +866,7 @@ static int sve_get_common(struct task_struct *target,
 		return membuf_zero(&to, end - start);
 
 	default:
-		return 0;
+		BUILD_BUG();
 	}
 }
 
@@ -946,10 +953,7 @@ static int sve_set_common(struct task_struct *target,
 		goto out;
 	}
 
-	/*
-	 * Otherwise: no registers or full SVE case.  For backwards
-	 * compatibility reasons we treat empty flags as SVE registers.
-	 */
+	/* Otherwise: no registers or full SVE case. */
 
 	/*
 	 * If setting a different VL from the requested VL and there is
diff --git a/tools/testing/selftests/arm64/fp/fp-ptrace.c b/tools/testing/selftests/arm64/fp/fp-ptrace.c
index c2882a5a5cc0a..191c47ca0ed80 100644
--- a/tools/testing/selftests/arm64/fp/fp-ptrace.c
+++ b/tools/testing/selftests/arm64/fp/fp-ptrace.c
@@ -439,10 +439,17 @@ static bool check_ptrace_values_sve(pid_t child, struct test_config *config)
 		pass = false;
 	}
 
-	if (sve->size != SVE_PT_SIZE(vq, sve->flags)) {
-		ksft_print_msg("Mismatch in SVE header size: %d != %lu\n",
-			       sve->size, SVE_PT_SIZE(vq, sve->flags));
-		pass = false;
+	if (svcr_in & SVCR_SM) {
+		if (sve->size != sizeof(sve)) {
+			ksft_print_msg("NT_ARM_SVE reports data with PSTATE.SM\n");
+			pass = false;
+		}
+	} else {
+		if (sve->size != SVE_PT_SIZE(vq, sve->flags)) {
+			ksft_print_msg("Mismatch in SVE header size: %d != %lu\n",
+				       sve->size, SVE_PT_SIZE(vq, sve->flags));
+			pass = false;
+		}
 	}
 
 	/* The registers might be in completely different formats! */
@@ -515,10 +522,17 @@ static bool check_ptrace_values_ssve(pid_t child, struct test_config *config)
 		pass = false;
 	}
 
-	if (sve->size != SVE_PT_SIZE(vq, sve->flags)) {
-		ksft_print_msg("Mismatch in SSVE header size: %d != %lu\n",
-			       sve->size, SVE_PT_SIZE(vq, sve->flags));
-		pass = false;
+	if (!(svcr_in & SVCR_SM)) {
+		if (sve->size != sizeof(sve)) {
+			ksft_print_msg("NT_ARM_SSVE reports data without PSTATE.SM\n");
+			pass = false;
+		}
+	} else {
+		if (sve->size != SVE_PT_SIZE(vq, sve->flags)) {
+			ksft_print_msg("Mismatch in SSVE header size: %d != %lu\n",
+				       sve->size, SVE_PT_SIZE(vq, sve->flags));
+			pass = false;
+		}
 	}
 
 	/* The registers might be in completely different formats! */
-- 
2.30.2

[PATCH 18/20] arm64/fpsimd: ptrace: Mandate SVE payload for streaming-mode state

[Mark Rutland]

When a task has PSTATE.SM==1, reads of NT_ARM_SSVE are required to
always present a header with SVE_PT_REGS_SVE, and register data in SVE
format. Reads of NT_ARM_SSVE must never present register data in FPSIMD
format. Within the kernel, we always expect streaming SVE data to be
stored in SVE format.

Currently a user can write to NT_ARM_SSVE with a header presenting
SVE_PT_REGS_FPSIMD rather than SVE_PT_REGS_SVE, placing the task's
FPSIMD/SVE data into an invalid state.

To fix this we can either:

(a) Forbid such writes.
(b) Accept such writes, and immediately convert data into SVE format.

Take the simple option and forbid such writes.

Fixes: e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ptrace.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index a2075e1df27c6..cff72b420eab8 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -890,6 +890,7 @@ static int sve_set_common(struct task_struct *target,
 	struct user_sve_header header;
 	unsigned int vq;
 	unsigned long start, end;
+	bool fpsimd;
 
 	/* Header */
 	if (count < sizeof(header))
@@ -899,6 +900,15 @@ static int sve_set_common(struct task_struct *target,
 	if (ret)
 		goto out;
 
+	/*
+	 * Streaming SVE data is always stored and presented in SVE format.
+	 * Require the user to provide SVE formatted data for consistency, and
+	 * to avoid the risk that we configure the task into an invalid state.
+	 */
+	fpsimd = (header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD;
+	if (fpsimd && type == ARM64_VEC_SME)
+		return -EINVAL;
+
 	/*
 	 * Apart from SVE_PT_REGS_MASK, all SVE_PT_* flags are consumed by
 	 * vec_set_vector_length(), which will also validate them for us:
@@ -945,7 +955,7 @@ static int sve_set_common(struct task_struct *target,
 	/* Registers: FPSIMD-only case */
 
 	BUILD_BUG_ON(SVE_PT_FPSIMD_OFFSET != sizeof(header));
-	if ((header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD) {
+	if (fpsimd) {
 		clear_tsk_thread_flag(target, TIF_SVE);
 		target->thread.fp_type = FP_STATE_FPSIMD;
 		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
-- 
2.30.2

Re: [PATCH 18/20] arm64/fpsimd: ptrace: Mandate SVE payload for streaming-mode state

[Mark Brown]

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

On Tue, May 06, 2025 at 04:25:21PM +0100, Mark Rutland wrote:
> When a task has PSTATE.SM==1, reads of NT_ARM_SSVE are required to
> always present a header with SVE_PT_REGS_SVE, and register data in SVE
> format. Reads of NT_ARM_SSVE must never present register data in FPSIMD
> format. Within the kernel, we always expect streaming SVE data to be
> stored in SVE format.

Reviewed-by: Mark Brown <broonie@kernel.org>

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

[PATCH 19/20] arm64/fpsimd: ptrace: Gracefully handle errors

[Mark Rutland]

Within sve_set_common() we do not handle error conditions correctly:

* When writing to NT_ARM_SSVE, if sme_alloc() fails, the task will be
  left with task->thread.sme_state==NULL, but TIF_SME will be set and
  task->thread.fp_type==FP_STATE_SVE. This will result in a subsequent
  null pointer dereference when the task's state is loaded or otherwise
  manipulated.

* When writing to NT_ARM_SSVE, if sve_alloc() fails, the task will be
  left with task->thread.sve_state==NULL, but TIF_SME will be set,
  PSTATE.SM will be set, and task->thread.fp_type==FP_STATE_FPSIMD.
  This is not a legitimate state, and can result in various problems,
  including a subsequent null pointer dereference and/or the task
  inheriting stale streaming mode register state the next time its state
  is loaded into hardware.

* When writing to NT_ARM_SSVE, if the VL is changed but the resultign VL
  differs from that in the header, the task will be left with TIF_SME
  set, PSTATE.SM set, but task->thread.fp_type==FP_STATE_FPSIMD. This is
  not a legitimate state, and can result in various problems as
  described above.

Avoid these problems by allocating memory earlier, and by changing the
task's saved fp_type to FP_STATE_SVE before skipping register writes due
to a change of VL. To make this simpler I've pulled the flushing of task
state earlier and moved the setting of TIF_SVE earlier -- this will be
cleared when loading FPSIMD-only state, and so moving this has no
resulting functional change. When changlnig the

Note that we only allocate the memory for SVE state when SVE register
contents are provided, avoiding unnecessary memory allocations for tasks
which only use FPSIMD.

Fixes: e12310a0d30f ("arm64/sme: Implement ptrace support for streaming mode SVE registers")
Fixes: baa8515281b3 ("arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE")
Fixes: 5d0a8d2fba50 ("arm64/ptrace: Ensure that SME is set up for target when writing SSVE state")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ptrace.c | 61 ++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index cff72b420eab8..a360e52db02fa 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -892,13 +892,15 @@ static int sve_set_common(struct task_struct *target,
 	unsigned long start, end;
 	bool fpsimd;
 
+	fpsimd_flush_task_state(target);
+
 	/* Header */
 	if (count < sizeof(header))
 		return -EINVAL;
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &header,
 				 0, sizeof(header));
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Streaming SVE data is always stored and presented in SVE format.
@@ -916,7 +918,21 @@ static int sve_set_common(struct task_struct *target,
 	ret = vec_set_vector_length(target, type, header.vl,
 		((unsigned long)header.flags & ~SVE_PT_REGS_MASK) << 16);
 	if (ret)
-		goto out;
+		return ret;
+
+	/* Allocate SME storage if necessary, preserving any existing ZA/ZT state */
+	if (type == ARM64_VEC_SME) {
+		sme_alloc(target, false);
+		if (!target->thread.sme_state)
+			return -ENOMEM;
+	}
+
+	/* Allocate SVE storage if necessary, zeroing any existing SVE state */
+	if (!fpsimd) {
+		sve_alloc(target, true);
+		if (!target->thread.sve_state)
+			return -ENOMEM;
+	}
 
 	/*
 	 * Actual VL set may be different from what the user asked
@@ -930,21 +946,15 @@ static int sve_set_common(struct task_struct *target,
 		switch (type) {
 		case ARM64_VEC_SVE:
 			target->thread.svcr &= ~SVCR_SM_MASK;
+			set_tsk_thread_flag(target, TIF_SVE);
 			break;
 		case ARM64_VEC_SME:
 			target->thread.svcr |= SVCR_SM_MASK;
-
-			/*
-			 * Disable traps and ensure there is SME storage but
-			 * preserve any currently set values in ZA/ZT.
-			 */
-			sme_alloc(target, false);
 			set_tsk_thread_flag(target, TIF_SME);
 			break;
 		default:
 			WARN_ON_ONCE(1);
-			ret = -EINVAL;
-			goto out;
+			return -EINVAL;
 		}
 	}
 
@@ -960,37 +970,20 @@ static int sve_set_common(struct task_struct *target,
 		target->thread.fp_type = FP_STATE_FPSIMD;
 		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
 				SVE_PT_FPSIMD_OFFSET);
-		goto out;
+		return ret;
 	}
 
 	/* Otherwise: no registers or full SVE case. */
 
+	target->thread.fp_type = FP_STATE_SVE;
+
 	/*
 	 * If setting a different VL from the requested VL and there is
 	 * register data, the data layout will be wrong: don't even
 	 * try to set the registers in this case.
 	 */
-	if (count && vq != sve_vq_from_vl(header.vl)) {
-		ret = -EIO;
-		goto out;
-	}
-
-	/* Always zero SVE state */
-	sve_alloc(target, true);
-	if (!target->thread.sve_state) {
-		ret = -ENOMEM;
-		clear_tsk_thread_flag(target, TIF_SVE);
-		target->thread.fp_type = FP_STATE_FPSIMD;
-		goto out;
-	}
-
-	/*
-	 * Only enable SVE if we are configuring normal SVE, a system with
-	 * streaming SVE may not have normal SVE.
-	 */
-	if (type == ARM64_VEC_SVE)
-		set_tsk_thread_flag(target, TIF_SVE);
-	target->thread.fp_type = FP_STATE_SVE;
+	if (count && vq != sve_vq_from_vl(header.vl))
+		return -EIO;
 
 	BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header));
 	start = SVE_PT_SVE_OFFSET;
@@ -999,7 +992,7 @@ static int sve_set_common(struct task_struct *target,
 				 target->thread.sve_state,
 				 start, end);
 	if (ret)
-		goto out;
+		return ret;
 
 	start = end;
 	end = SVE_PT_SVE_FPSR_OFFSET(vq);
@@ -1015,8 +1008,6 @@ static int sve_set_common(struct task_struct *target,
 				 &target->thread.uw.fpsimd_state.fpsr,
 				 start, end);
 
-out:
-	fpsimd_flush_task_state(target);
 	return ret;
 }
 
-- 
2.30.2

Re: [PATCH 19/20] arm64/fpsimd: ptrace: Gracefully handle errors

[Will Deacon]

On Tue, May 06, 2025 at 04:25:22PM +0100, Mark Rutland wrote:
> Within sve_set_common() we do not handle error conditions correctly:
> 
> * When writing to NT_ARM_SSVE, if sme_alloc() fails, the task will be
>   left with task->thread.sme_state==NULL, but TIF_SME will be set and
>   task->thread.fp_type==FP_STATE_SVE. This will result in a subsequent
>   null pointer dereference when the task's state is loaded or otherwise
>   manipulated.
> 
> * When writing to NT_ARM_SSVE, if sve_alloc() fails, the task will be
>   left with task->thread.sve_state==NULL, but TIF_SME will be set,
>   PSTATE.SM will be set, and task->thread.fp_type==FP_STATE_FPSIMD.
>   This is not a legitimate state, and can result in various problems,
>   including a subsequent null pointer dereference and/or the task
>   inheriting stale streaming mode register state the next time its state
>   is loaded into hardware.
> 
> * When writing to NT_ARM_SSVE, if the VL is changed but the resultign VL
>   differs from that in the header, the task will be left with TIF_SME
>   set, PSTATE.SM set, but task->thread.fp_type==FP_STATE_FPSIMD. This is
>   not a legitimate state, and can result in various problems as
>   described above.
> 
> Avoid these problems by allocating memory earlier, and by changing the
> task's saved fp_type to FP_STATE_SVE before skipping register writes due
> to a change of VL. To make this simpler I've pulled the flushing of task
> state earlier and moved the setting of TIF_SVE earlier -- this will be
> cleared when loading FPSIMD-only state, and so moving this has no
> resulting functional change.

Doesn't flushing the state earlier mean that passing a count smaller than
the header size is now potentially destructive to the fpsimd state?

> When changlnig the

This ends mid-sentence and 'changlnig' sounds like a Doors tune.

Will

Re: [PATCH 19/20] arm64/fpsimd: ptrace: Gracefully handle errors

[Mark Rutland]

On Wed, May 07, 2025 at 05:32:43PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:22PM +0100, Mark Rutland wrote:
> > Within sve_set_common() we do not handle error conditions correctly:
> > 
> > * When writing to NT_ARM_SSVE, if sme_alloc() fails, the task will be
> >   left with task->thread.sme_state==NULL, but TIF_SME will be set and
> >   task->thread.fp_type==FP_STATE_SVE. This will result in a subsequent
> >   null pointer dereference when the task's state is loaded or otherwise
> >   manipulated.
> > 
> > * When writing to NT_ARM_SSVE, if sve_alloc() fails, the task will be
> >   left with task->thread.sve_state==NULL, but TIF_SME will be set,
> >   PSTATE.SM will be set, and task->thread.fp_type==FP_STATE_FPSIMD.
> >   This is not a legitimate state, and can result in various problems,
> >   including a subsequent null pointer dereference and/or the task
> >   inheriting stale streaming mode register state the next time its state
> >   is loaded into hardware.
> > 
> > * When writing to NT_ARM_SSVE, if the VL is changed but the resultign VL
> >   differs from that in the header, the task will be left with TIF_SME
> >   set, PSTATE.SM set, but task->thread.fp_type==FP_STATE_FPSIMD. This is
> >   not a legitimate state, and can result in various problems as
> >   described above.
> > 
> > Avoid these problems by allocating memory earlier, and by changing the
> > task's saved fp_type to FP_STATE_SVE before skipping register writes due
> > to a change of VL. To make this simpler I've pulled the flushing of task
> > state earlier and moved the setting of TIF_SVE earlier -- this will be
> > cleared when loading FPSIMD-only state, and so moving this has no
> > resulting functional change.
> 
> Doesn't flushing the state earlier mean that passing a count smaller than
> the header size is now potentially destructive to the fpsimd state?

Sorry, this is poorly worded, and I will go rewrite this for clarity.

There are two key things to bear in mind:

(1) In patch 5 ("arm64/fpsimd: ptrace: Consistently handle partial
    writes to NT_ARM_(S)SVE"), sve_set_common() was changed to always
    zero state first, so that's *always* destructive.

(2) Here, "flush" is referring to the poorly named which detaches a task
    from live/stale state on any CPU, and does not modify the saved
    state in any way. For the ptrace set_*() functions, we know that the
    tracee is blocked (and its state has already been saved), so calling
    that earlier doesn't result in any functional change.

> > When changlnig the
> 
> This ends mid-sentence and 'changlnig' sounds like a Doors tune.

I've deleted this (and fix the "resultign" typo above).

Mark.

[PATCH 20/20] arm64/fpsimd: Allow CONFIG_ARM64_SME to be selected

[Mark Rutland]

Now that the known issues with SME have been addressed, allow SME to be
selected.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: David Spickett <david.spickett@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Luis Machado <luis.machado@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Richard Sandiford <richard.sandiford@arm.com>
Cc: Sander De Smalen <sander.desmalen@arm.com>
Cc: Tamas Petz <tamas.petz@arm.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Will Deacon <will@kernel.org>
Cc: Yury Khrustalev <yury.khrustalev@arm.com>
---
 arch/arm64/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a182295e6f08b..27437f13154e0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2285,7 +2285,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.30.2

Re: [PATCH 00/20] arm64: FPSIMD/SVE/SME fixes + re-eanble SME

[Mark Brown]

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

On Tue, May 06, 2025 at 04:25:03PM +0100, Mark Rutland wrote:
> Hi all,
> 
> These patches fix a number of problems in the FPSIMD/SVE/SME code, and
> finally re-enable SME support.

I've given everything a first pass and aside from previously noted
changelog nit on patch 17 which don't affect the resulting change this
all looks good.  I'm on holiday for a week and a bit more so probably
won't have time to review properly, other than a couple of the smaller
patches.

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

Re: [PATCH 00/20] arm64: FPSIMD/SVE/SME fixes + re-eanble SME

[Mark Rutland]

On Wed, May 07, 2025 at 10:48:43AM +0900, Mark Brown wrote:
> On Tue, May 06, 2025 at 04:25:03PM +0100, Mark Rutland wrote:
> > Hi all,
> > 
> > These patches fix a number of problems in the FPSIMD/SVE/SME code, and
> > finally re-enable SME support.
> 
> I've given everything a first pass and aside from previously noted
> changelog nit on patch 17 which don't affect the resulting change this
> all looks good.

Thanks.

Since you haven't commented on patch 17 here, and this is the first
version of that patch, I assume that by "previously noted" you're
referring to the ptrace ABI discussion we had when I posted the
preparatory fixes, i.e.

  https://lore.kernel.org/linux-arm-kernel/20250404174435.3288106-1-mark.rutland@arm.com/
  https://lore.kernel.org/linux-arm-kernel/45cb5ce9-39af-4c42-b68d-fa506b67bc55@sirena.org.uk/
  https://lore.kernel.org/linux-arm-kernel/Z_VS5wvn0C7tdjbo@J2N7QTR9R3.cambridge.arm.com/
  https://lore.kernel.org/linux-arm-kernel/879811da-d0f0-4b0b-bca4-df68ff9bf394@sirena.org.uk/#t

If you have a specific objection, I can consider massaging the wording,
but as it stands I don't see any problem with the commit message on
patch 17. There clearly was an incorrect assumption during development,
as indicated by the code, the comments, and the documentation. Providing
that context makes review *much* easier.

> I'm on holiday for a week and a bit more so probably
> won't have time to review properly, other than a couple of the smaller
> patches.

No problem. If it's possible to throw any automated testing at this,
that'd be much appreciated.

Mark.

Re: [PATCH 00/20] arm64: FPSIMD/SVE/SME fixes + re-eanble SME

[Mark Brown]

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

On Wed, May 07, 2025 at 10:56:54AM +0100, Mark Rutland wrote:

> If you have a specific objection, I can consider massaging the wording,
> but as it stands I don't see any problem with the commit message on
> patch 17. There clearly was an incorrect assumption during development,
> as indicated by the code, the comments, and the documentation. Providing
> that context makes review *much* easier.

I don't have a specific wording in the style you like right now, and
like I say I'm not sure it's too important since the actual chagne looks
good.  The main thing is just that the original thought was that we'd
not supply any register data unless in streaming mode which does due to
the use of one bit boil down to setting the FPSIMD value for the flag,
but the intent was more that the data format flag was rendered
irrelevant by the lack of data.  I was definitely aware that the two
flags were just a single bit - I distinctly remember seeing that and
thinking how unhelpful it was.  

Life would be a lot simpler and we probably wouldn't loose much if we
didn't support the FPSIMD format in the SVE regset but that ship is far
over the horizon.

> > I'm on holiday for a week and a bit more so probably
> > won't have time to review properly, other than a couple of the smaller
> > patches.

> No problem. If it's possible to throw any automated testing at this,
> that'd be much appreciated.

Yeah, might take a little bit since some other stuff blew up and my CI's
a bit miserable right now.

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