Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
@ 2026-11-02 23:48 Wei Wang
  2026-11-03 11:50 ` Will Deacon
  2026-11-03 13:56 ` Mark Brown
  0 siblings, 2 replies; 6+ messages in thread
From: Wei Wang @ 2026-11-02 23:48 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Mark Brown, Mark Rutland, Rafael J . Wysocki, linux-arm-kernel,
	linux-pm, linux-kernel, stable, wei.vince.wang, Wei Wang

On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
context-switches it eagerly in tls_thread_switch(). While a task is
running, the live register is the only copy of its value.

The architecture defines TPIDR2_EL0 as UNKNOWN after a warm reset, so
a CPU power-down through PSCI CPU_SUSPEND or SYSTEM_SUSPEND can lose it.
cpu_do_suspend() and cpu_do_resume() already preserve TPIDR_EL0 and
TPIDRRO_EL0, but not TPIDR2_EL0.

For CPU idle this is harmless: current is the idle task, and the next
context switch reloads TPIDR2_EL0 from next->thread.tpidr2_el0. For
suspend-to-RAM and hibernation, however, cpu_suspend() runs in the
context of the userspace task that wrote /sys/power/state, and that
task returns to EL0 with an UNKNOWN TPIDR2_EL0. On hardware where the
reset value is non-zero, the next SME ABI lazy-save check in that
thread faults. For example, bionic's fork() -> clone() ->
__arm_za_disable() -> __arm_tpidr2_save() loads from the garbage
pointer:

  # echo mem > /sys/power/state; true
  signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xa3bad11bcaf72e93
      x16 a3bad11bcaf72e89
  #00 pc ... libc.so (__arm_tpidr2_save+24)
  #01 pc ... libc.so (__arm_za_disable+32)
  #02 pc ... libc.so (clone+480)
  #03 pc ... libc.so (fork+60)

Having firmware zero TPIDR2_EL0 on warm reset avoids the crash. It is
not enough on its own, though: a thread that enters the kernel with a
lazy save pending (TPIDR2_EL0 pointing at a valid TPIDR2 block) would
see TPIDR2_EL0 == 0 on return. It would then wrongly conclude that ZA
had been committed and restore stale data from its save buffer. The
kernel owns this state and must preserve it.

Save TPIDR2_EL0 in cpu_do_suspend() and restore it in cpu_do_resume()
when SME is present. Placing this in proc.S covers both cpu_suspend()
and hibernation. The new slot uses ctx_regs[14]; sp moves into what was
previously tail padding, so sizeof(struct cpu_suspend_ctx) stays 128
bytes. sp is only accessed through the CPU_CTX_SP asm-offset.

Fixes: a9d691585950 ("arm64/sme: Implement support for TPIDR2")
Signed-off-by: Wei Wang <wvw@google.com>
---
 arch/arm64/include/asm/suspend.h | 2 +-
 arch/arm64/mm/proc.S             | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
index eb60c9735553..2cc5fb405769 100644
--- a/arch/arm64/include/asm/suspend.h
+++ b/arch/arm64/include/asm/suspend.h
@@ -2,7 +2,7 @@
 #ifndef __ASM_SUSPEND_H
 #define __ASM_SUSPEND_H
 
-#define NR_CTX_REGS 14
+#define NR_CTX_REGS 15
 #define NR_CALLEE_SAVED_REGS 12
 
 /*
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 7afbece885a3..c7b7fa6491c3 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -103,6 +103,10 @@ SYM_FUNC_START(cpu_do_suspend)
 alternative_if ARM64_HAS_TCR2
 	mrs	x2, REG_TCR2_EL1
 	str	x2, [x0, #104]
+alternative_else_nop_endif
+alternative_if ARM64_SME
+	mrs_s	x2, SYS_TPIDR2_EL0
+	str	x2, [x0, #112]
 alternative_else_nop_endif
 	ret
 SYM_FUNC_END(cpu_do_suspend)
@@ -142,6 +146,10 @@ alternative_if ARM64_HAS_TCR2
 	ldr	x2, [x0, #104]
 	msr	REG_TCR2_EL1, x2
 alternative_else_nop_endif
+alternative_if ARM64_SME
+	ldr	x2, [x0, #112]
+	msr_s	SYS_TPIDR2_EL0, x2
+alternative_else_nop_endif
 
 	msr	sctlr_el1, x12
 	set_this_cpu_offset x13
-- 
2.56.0.385.gac671c819a-goog



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
  2026-11-02 23:48 [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend Wei Wang
@ 2026-11-03 11:50 ` Will Deacon
  2026-11-03 15:49   ` Catalin Marinas
  2026-11-03 13:56 ` Mark Brown
  1 sibling, 1 reply; 6+ messages in thread
From: Will Deacon @ 2026-11-03 11:50 UTC (permalink / raw)
  To: Wei Wang
  Cc: Catalin Marinas, Mark Brown, Mark Rutland, Rafael J . Wysocki,
	linux-arm-kernel, linux-pm, linux-kernel, stable, wei.vince.wang

On Mon, Nov 02, 2026 at 11:48:13PM +0000, Wei Wang wrote:
> On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
> the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
> context-switches it eagerly in tls_thread_switch(). While a task is
> running, the live register is the only copy of its value.
> 
> The architecture defines TPIDR2_EL0 as UNKNOWN after a warm reset, so
> a CPU power-down through PSCI CPU_SUSPEND or SYSTEM_SUSPEND can lose it.
> cpu_do_suspend() and cpu_do_resume() already preserve TPIDR_EL0 and
> TPIDRRO_EL0, but not TPIDR2_EL0.
> 
> For CPU idle this is harmless: current is the idle task, and the next
> context switch reloads TPIDR2_EL0 from next->thread.tpidr2_el0. For
> suspend-to-RAM and hibernation, however, cpu_suspend() runs in the
> context of the userspace task that wrote /sys/power/state, and that
> task returns to EL0 with an UNKNOWN TPIDR2_EL0. On hardware where the
> reset value is non-zero, the next SME ABI lazy-save check in that
> thread faults. For example, bionic's fork() -> clone() ->
> __arm_za_disable() -> __arm_tpidr2_save() loads from the garbage
> pointer:
> 
>   # echo mem > /sys/power/state; true
>   signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xa3bad11bcaf72e93
>       x16 a3bad11bcaf72e89
>   #00 pc ... libc.so (__arm_tpidr2_save+24)
>   #01 pc ... libc.so (__arm_za_disable+32)
>   #02 pc ... libc.so (clone+480)
>   #03 pc ... libc.so (fork+60)
> 
> Having firmware zero TPIDR2_EL0 on warm reset avoids the crash. It is
> not enough on its own, though: a thread that enters the kernel with a
> lazy save pending (TPIDR2_EL0 pointing at a valid TPIDR2 block) would
> see TPIDR2_EL0 == 0 on return. It would then wrongly conclude that ZA
> had been committed and restore stale data from its save buffer. The
> kernel owns this state and must preserve it.

There's a lot of unnecessary (AI-generated?) detail in here. Please can
you trim this down a bit? I don't think we care about the firmware part,
the stuff about idle or the bionic crash. The bottom line is that we're
not saving/restoring a live EL0 register across suspend/resume.

> Save TPIDR2_EL0 in cpu_do_suspend() and restore it in cpu_do_resume()
> when SME is present. Placing this in proc.S covers both cpu_suspend()
> and hibernation. The new slot uses ctx_regs[14]; sp moves into what was
> previously tail padding, so sizeof(struct cpu_suspend_ctx) stays 128
> bytes. sp is only accessed through the CPU_CTX_SP asm-offset.
> 
> Fixes: a9d691585950 ("arm64/sme: Implement support for TPIDR2")
> Signed-off-by: Wei Wang <wvw@google.com>
> ---
>  arch/arm64/include/asm/suspend.h | 2 +-
>  arch/arm64/mm/proc.S             | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/suspend.h b/arch/arm64/include/asm/suspend.h
> index eb60c9735553..2cc5fb405769 100644
> --- a/arch/arm64/include/asm/suspend.h
> +++ b/arch/arm64/include/asm/suspend.h
> @@ -2,7 +2,7 @@
>  #ifndef __ASM_SUSPEND_H
>  #define __ASM_SUSPEND_H
>  
> -#define NR_CTX_REGS 14
> +#define NR_CTX_REGS 15
>  #define NR_CALLEE_SAVED_REGS 12
>  
>  /*
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 7afbece885a3..c7b7fa6491c3 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -103,6 +103,10 @@ SYM_FUNC_START(cpu_do_suspend)
>  alternative_if ARM64_HAS_TCR2
>  	mrs	x2, REG_TCR2_EL1
>  	str	x2, [x0, #104]
> +alternative_else_nop_endif
> +alternative_if ARM64_SME
> +	mrs_s	x2, SYS_TPIDR2_EL0
> +	str	x2, [x0, #112]
>  alternative_else_nop_endif
>  	ret
>  SYM_FUNC_END(cpu_do_suspend)
> @@ -142,6 +146,10 @@ alternative_if ARM64_HAS_TCR2
>  	ldr	x2, [x0, #104]
>  	msr	REG_TCR2_EL1, x2
>  alternative_else_nop_endif
> +alternative_if ARM64_SME
> +	ldr	x2, [x0, #112]
> +	msr_s	SYS_TPIDR2_EL0, x2
> +alternative_else_nop_endif

nit: We use msr and REG_* for TCR2, so it would be more consistent to do
the same for TPIDR2_EL0 if the assembler is happy with it.

Otherwise, looks ok to me:

Acked-by: Will Deacon <will@kernel.org>

Will


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
  2026-11-02 23:48 [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend Wei Wang
  2026-11-03 11:50 ` Will Deacon
@ 2026-11-03 13:56 ` Mark Brown
  2026-11-03 17:57   ` Wei Wang
  1 sibling, 1 reply; 6+ messages in thread
From: Mark Brown @ 2026-11-03 13:56 UTC (permalink / raw)
  To: Wei Wang
  Cc: Catalin Marinas, Will Deacon, Mark Rutland, Rafael J . Wysocki,
	linux-arm-kernel, linux-pm, linux-kernel, stable, wei.vince.wang

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

On Mon, Nov 02, 2026 at 11:48:13PM +0000, Wei Wang wrote:

> On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
> the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
> context-switches it eagerly in tls_thread_switch(). While a task is
> running, the live register is the only copy of its value.

Thanks for catching this:

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

but I think we're missing some other registers, I made the same mistake
for GCS with GCSPR_EL0 and GCSCRE0_EL1 and I'm not seeing where POR_EL0
is handled either but possibly it's somewhere I didn't spot.  I can look
into those incrementally once this gets applied (or if you do a v2 and
roll them in that'd also be great!).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
  2026-11-03 11:50 ` Will Deacon
@ 2026-11-03 15:49   ` Catalin Marinas
  2026-11-03 17:50     ` Wei Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2026-11-03 15:49 UTC (permalink / raw)
  To: Will Deacon
  Cc: Wei Wang, Mark Brown, Mark Rutland, Rafael J . Wysocki,
	linux-arm-kernel, linux-pm, linux-kernel, stable, wei.vince.wang

On Tue, Nov 03, 2026 at 11:50:03AM +0000, Will Deacon wrote:
> On Mon, Nov 02, 2026 at 11:48:13PM +0000, Wei Wang wrote:
> > On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
> > the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
> > context-switches it eagerly in tls_thread_switch(). While a task is
> > running, the live register is the only copy of its value.

[...]

> There's a lot of unnecessary (AI-generated?) detail in here. Please can
> you trim this down a bit? I don't think we care about the firmware part,
> the stuff about idle or the bionic crash. The bottom line is that we're
> not saving/restoring a live EL0 register across suspend/resume.

I should get an LLM to trim it down when applying ;).

> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 7afbece885a3..c7b7fa6491c3 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -103,6 +103,10 @@ SYM_FUNC_START(cpu_do_suspend)
> >  alternative_if ARM64_HAS_TCR2
> >  	mrs	x2, REG_TCR2_EL1
> >  	str	x2, [x0, #104]
> > +alternative_else_nop_endif
> > +alternative_if ARM64_SME
> > +	mrs_s	x2, SYS_TPIDR2_EL0
> > +	str	x2, [x0, #112]
> >  alternative_else_nop_endif
> >  	ret
> >  SYM_FUNC_END(cpu_do_suspend)
> > @@ -142,6 +146,10 @@ alternative_if ARM64_HAS_TCR2
> >  	ldr	x2, [x0, #104]
> >  	msr	REG_TCR2_EL1, x2
> >  alternative_else_nop_endif
> > +alternative_if ARM64_SME
> > +	ldr	x2, [x0, #112]
> > +	msr_s	SYS_TPIDR2_EL0, x2
> > +alternative_else_nop_endif
> 
> nit: We use msr and REG_* for TCR2, so it would be more consistent to do
> the same for TPIDR2_EL0 if the assembler is happy with it.

TPIDR2 is not defined in arch/arm64/tools/sysreg yet (for some reason,
only TPIDR_EL1 is). Since this is a cc stable, I think we can leave it
as is and do a TPIDR* cleanup going forward.

-- 
Catalin


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
  2026-11-03 15:49   ` Catalin Marinas
@ 2026-11-03 17:50     ` Wei Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Wei Wang @ 2026-11-03 17:50 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, Mark Brown, Mark Rutland, Rafael J . Wysocki,
	linux-arm-kernel, linux-pm, linux-kernel, stable, wei.vince.wang

On Tue, Nov 3, 2026 at 7:49 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Nov 03, 2026 at 11:50:03AM +0000, Will Deacon wrote:
> > On Mon, Nov 02, 2026 at 11:48:13PM +0000, Wei Wang wrote:
> > > On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
> > > the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
> > > context-switches it eagerly in tls_thread_switch(). While a task is
> > > running, the live register is the only copy of its value.
>
> [...]
>
> > There's a lot of unnecessary (AI-generated?) detail in here. Please can
> > you trim this down a bit? I don't think we care about the firmware part,
> > the stuff about idle or the bionic crash. The bottom line is that we're
> > not saving/restoring a live EL0 register across suspend/resume.
>
> I should get an LLM to trim it down when applying ;).
>

Fair point, I'll send a v2 with a trimmed commit message.

> > > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > > index 7afbece885a3..c7b7fa6491c3 100644
> > > --- a/arch/arm64/mm/proc.S
> > > +++ b/arch/arm64/mm/proc.S
> > > @@ -103,6 +103,10 @@ SYM_FUNC_START(cpu_do_suspend)
> > >  alternative_if ARM64_HAS_TCR2
> > >     mrs     x2, REG_TCR2_EL1
> > >     str     x2, [x0, #104]
> > > +alternative_else_nop_endif
> > > +alternative_if ARM64_SME
> > > +   mrs_s   x2, SYS_TPIDR2_EL0
> > > +   str     x2, [x0, #112]
> > >  alternative_else_nop_endif
> > >     ret
> > >  SYM_FUNC_END(cpu_do_suspend)
> > > @@ -142,6 +146,10 @@ alternative_if ARM64_HAS_TCR2
> > >     ldr     x2, [x0, #104]
> > >     msr     REG_TCR2_EL1, x2
> > >  alternative_else_nop_endif
> > > +alternative_if ARM64_SME
> > > +   ldr     x2, [x0, #112]
> > > +   msr_s   SYS_TPIDR2_EL0, x2
> > > +alternative_else_nop_endif
> >
> > nit: We use msr and REG_* for TCR2, so it would be more consistent to do
> > the same for TPIDR2_EL0 if the assembler is happy with it.
>
> TPIDR2 is not defined in arch/arm64/tools/sysreg yet (for some reason,
> only TPIDR_EL1 is). Since this is a cc stable, I think we can leave it
> as is and do a TPIDR* cleanup going forward.
>
> --
> Catalin


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend
  2026-11-03 13:56 ` Mark Brown
@ 2026-11-03 17:57   ` Wei Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Wei Wang @ 2026-11-03 17:57 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Mark Rutland, Rafael J . Wysocki,
	linux-arm-kernel, linux-pm, linux-kernel, stable, wei.vince.wang

On Tue, Nov 3, 2026 at 5:56 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 02, 2026 at 11:48:13PM +0000, Wei Wang wrote:
>
> > On systems with FEAT_SME, TPIDR2_EL0 is part of userspace thread state:
> > the AAPCS64 SME ABI uses it as the ZA lazy-save pointer, and the kernel
> > context-switches it eagerly in tls_thread_switch(). While a task is
> > running, the live register is the only copy of its value.
>
> Thanks for catching this:
>
> Reviewed-by: Mark Brown <broonie@kernel.org>
>

Thanks!

> but I think we're missing some other registers, I made the same mistake
> for GCS with GCSPR_EL0 and GCSCRE0_EL1 and I'm not seeing where POR_EL0
> is handled either but possibly it's somewhere I didn't spot.  I can look
> into those incrementally once this gets applied (or if you do a v2 and
> roll them in that'd also be great!).

I think you're right. As far as I can see, these are only switched in
process.c and nothing touches them
in the suspend path. Unfortunately the hardware I am testing doesn't
implement GCS or POE per cpuinfo,
so I couldn't test changes there. I'd rather keep this one minimal for
stable and leave those as a follow-up.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-11-03 17:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-02 23:48 [PATCH] arm64: suspend: Save and restore TPIDR2_EL0 across CPU suspend Wei Wang
2026-11-03 11:50 ` Will Deacon
2026-11-03 15:49   ` Catalin Marinas
2026-11-03 17:50     ` Wei Wang
2026-11-03 13:56 ` Mark Brown
2026-11-03 17:57   ` Wei Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox