[PATCH 0/2] arm64/sme: Fix handling of traps on resume

[PATCH 0/2] arm64/sme: Fix handling of traps on resume

[Mark Brown]

The fast model was recently changed to reset system registers to 0 on
resume, exposing the fact that for SME we do not restore the
configuration of traps for extensions that add state.  Fix this.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (2):
      arm64/sme: Restore SMCR on exit from suspend
      arm64/sme: Restore SMCR_EL1.EZT0 on exit from suspend

 arch/arm64/include/asm/fpsimd.h |  2 ++
 arch/arm64/kernel/fpsimd.c      | 15 +++++++++++++++
 arch/arm64/kernel/suspend.c     |  3 +++
 3 files changed, 20 insertions(+)
---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240129-arm64-sme-resume-3266150292b6

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

[PATCH 1/2] arm64/sme: Restore SMCR on exit from suspend

[Mark Brown]

The fields in SMCR_EL1 reset to an architecturally UNKNOWN value. Since we
do not otherwise manage the traps configured in this register at runtime we
need to reconfigure them after a suspend in case nothing else was kind
enough to preserve them for us.

The vector length will be restored as part of restoring the SME state for
the next SME using task.

Fixes: a1f4ccd25cc2 (arm64/sme: Provide Kconfig for SME)
Reported-by: Jackson Cooper-Driver <Jackson.Cooper-Driver@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h |  2 ++
 arch/arm64/kernel/fpsimd.c      | 13 +++++++++++++
 arch/arm64/kernel/suspend.c     |  3 +++
 3 files changed, 18 insertions(+)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 50e5f25d3024..7780d343ef08 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -386,6 +386,7 @@ extern void sme_alloc(struct task_struct *task, bool flush);
 extern unsigned int sme_get_vl(void);
 extern int sme_set_current_vl(unsigned long arg);
 extern int sme_get_current_vl(void);
+extern void sme_suspend_exit(void);
 
 /*
  * Return how many bytes of memory are required to store the full SME
@@ -421,6 +422,7 @@ static inline int sme_max_vl(void) { return 0; }
 static inline int sme_max_virtualisable_vl(void) { return 0; }
 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(struct task_struct const *task)
 {
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index a5dc6f764195..69201208bb13 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1311,6 +1311,19 @@ void __init sme_setup(void)
 		get_sme_default_vl());
 }
 
+void sme_suspend_exit(void)
+{
+	u64 smcr = 0;
+
+	if (!system_supports_sme())
+		return;
+
+	if (system_supports_fa64())
+		smcr |= SMCR_ELx_FA64;
+
+	write_sysreg_s(smcr, SYS_SMCR_EL1);
+}
+
 #endif /* CONFIG_ARM64_SME */
 
 static void sve_init_regs(void)
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index eca4d0435211..eaaff94329cd 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -12,6 +12,7 @@
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
 #include <asm/exec.h>
+#include <asm/fpsimd.h>
 #include <asm/mte.h>
 #include <asm/memory.h>
 #include <asm/mmu_context.h>
@@ -80,6 +81,8 @@ void notrace __cpu_suspend_exit(void)
 	 */
 	spectre_v4_enable_mitigation(NULL);
 
+	sme_suspend_exit();
+
 	/* Restore additional feature-specific configuration */
 	ptrauth_suspend_exit();
 }

-- 
2.30.2

Re: [PATCH 1/2] arm64/sme: Restore SMCR on exit from suspend

[Dave Martin]

On Tue, Jan 30, 2024 at 12:02:48AM +0000, Mark Brown wrote:
> The fields in SMCR_EL1 reset to an architecturally UNKNOWN value. Since we
> do not otherwise manage the traps configured in this register at runtime we
> need to reconfigure them after a suspend in case nothing else was kind
> enough to preserve them for us.

Are any other regs affected?  

What about SMPRI_EL1?  That seems to be initialised once and for all in
cpufeatures, so I'd guess it might be affected.

Also, what about the _EL2 regs if the kernel is resuming at EL2
(without VHE -- or if SME && !VHE not a thing?)


> The vector length will be restored as part of restoring the SME state for
> the next SME using task.
> 
> Fixes: a1f4ccd25cc2 (arm64/sme: Provide Kconfig for SME)
> Reported-by: Jackson Cooper-Driver <Jackson.Cooper-Driver@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/fpsimd.h |  2 ++
>  arch/arm64/kernel/fpsimd.c      | 13 +++++++++++++
>  arch/arm64/kernel/suspend.c     |  3 +++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 50e5f25d3024..7780d343ef08 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -386,6 +386,7 @@ extern void sme_alloc(struct task_struct *task, bool flush);
>  extern unsigned int sme_get_vl(void);
>  extern int sme_set_current_vl(unsigned long arg);
>  extern int sme_get_current_vl(void);
> +extern void sme_suspend_exit(void);
>  
>  /*
>   * Return how many bytes of memory are required to store the full SME
> @@ -421,6 +422,7 @@ static inline int sme_max_vl(void) { return 0; }
>  static inline int sme_max_virtualisable_vl(void) { return 0; }
>  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(struct task_struct const *task)
>  {
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index a5dc6f764195..69201208bb13 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1311,6 +1311,19 @@ void __init sme_setup(void)
>  		get_sme_default_vl());
>  }
>  
> +void sme_suspend_exit(void)
> +{
> +	u64 smcr = 0;
> +
> +	if (!system_supports_sme())
> +		return;
> +
> +	if (system_supports_fa64())
> +		smcr |= SMCR_ELx_FA64;

This seems to silently duplicate logic present in cpufeatures.c.
Would it be cleaner to save/restore this register explicitly across
suspend, once cpufeatures has initialised it?

Or this could be factored somehow, but dumbly saving/restoring it is
probably simpler (?)

> +	write_sysreg_s(smcr, SYS_SMCR_EL1);

Is there an ISB or equivalent somewhere on this path?

Can we blow up when trying to restore SME state (e.g., ZT0) before we
enter userspace for the first time, if the firmware left the SME regs
inaccessible?

> +}
> +

[...]

Cheers
---Dave

Re: [PATCH 1/2] arm64/sme: Restore SMCR on exit from suspend

[Mark Brown]

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

On Tue, Jan 30, 2024 at 10:53:42AM +0000, Dave Martin wrote:
> On Tue, Jan 30, 2024 at 12:02:48AM +0000, Mark Brown wrote:
> > The fields in SMCR_EL1 reset to an architecturally UNKNOWN value. Since we
> > do not otherwise manage the traps configured in this register at runtime we
> > need to reconfigure them after a suspend in case nothing else was kind
> > enough to preserve them for us.

> Are any other regs affected?  

> What about SMPRI_EL1?  That seems to be initialised once and for all in
> cpufeatures, so I'd guess it might be affected.

Ah, yes - we should do that too, thanks.  At present we map SMPRI_EL1
out using EL2 controls and just set it to 0 on init so I keep forgetting
about it, I wrote a few lines of code years ago.

> Also, what about the _EL2 regs if the kernel is resuming at EL2
> (without VHE -- or if SME && !VHE not a thing?)

Yeah, I was somewhat confused about where the EL2 handling was in the
resume path and was hoping that if we weren't just rerunning the initial
setup someone would tell me what I'm missing (which appeared to be what
was happening).

The hardware will always have VHE but we could be running nVHE (eg, for
pKVM) so not using it.

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

Re: [PATCH 1/2] arm64/sme: Restore SMCR on exit from suspend

[Mark Brown]

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

On Tue, Jan 30, 2024 at 10:53:42AM +0000, Dave Martin wrote:
> On Tue, Jan 30, 2024 at 12:02:48AM +0000, Mark Brown wrote:

Sorry, didn't see these bits due to the large block of quoted text.

> > +	if (system_supports_fa64())
> > +		smcr |= SMCR_ELx_FA64;

> This seems to silently duplicate logic present in cpufeatures.c.
> Would it be cleaner to save/restore this register explicitly across
> suspend, once cpufeatures has initialised it?

I was unsure about that, I could go either way.  All the register save
and restore is currently done in assembler which felt like it was doing
things too early so I went with this instead.

> Or this could be factored somehow, but dumbly saving/restoring it is
> probably simpler (?)

Yes, I keep thinking about doing that but factoring out is annoying
since there's also the KVM case where we don't always want to base the
decision about the settings on the cpufeature detection.

> > +	write_sysreg_s(smcr, SYS_SMCR_EL1);

> Is there an ISB or equivalent somewhere on this path?

> Can we blow up when trying to restore SME state (e.g., ZT0) before we
> enter userspace for the first time, if the firmware left the SME regs
> inaccessible?

I concluded when I wrote this that there was but I confess I can't
remember where exactly now.

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

[PATCH 2/2] arm64/sme: Restore SMCR_EL1.EZT0 on exit from suspend

[Mark Brown]

The fields in SMCR_EL1 reset to an architecturally UNKNOWN value. Since we
do not otherwise manage the traps configured in this register at runtime we
need to reconfigure them after a suspend in case nothing else was kind
enough to preserve them for us. Do so for SMCR_EL1.EZT0.

Fixes: d4913eee152d (arm64/sme: Add basic enumeration for SME2)
Reported-by: Jackson Cooper-Driver <Jackson.Cooper-Driver@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 69201208bb13..329782fe39c5 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1320,6 +1320,8 @@ void sme_suspend_exit(void)
 
 	if (system_supports_fa64())
 		smcr |= SMCR_ELx_FA64;
+	if (system_supports_sme2())
+		smcr |= SMCR_ELx_EZT0;
 
 	write_sysreg_s(smcr, SYS_SMCR_EL1);
 }

-- 
2.30.2

Re: [PATCH 2/2] arm64/sme: Restore SMCR_EL1.EZT0 on exit from suspend

[Dave Martin]

On Tue, Jan 30, 2024 at 12:02:49AM +0000, Mark Brown wrote:
> The fields in SMCR_EL1 reset to an architecturally UNKNOWN value. Since we
> do not otherwise manage the traps configured in this register at runtime we
> need to reconfigure them after a suspend in case nothing else was kind
> enough to preserve them for us. Do so for SMCR_EL1.EZT0.
> 
> Fixes: d4913eee152d (arm64/sme: Add basic enumeration for SME2)
> Reported-by: Jackson Cooper-Driver <Jackson.Cooper-Driver@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 69201208bb13..329782fe39c5 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1320,6 +1320,8 @@ void sme_suspend_exit(void)
>  
>  	if (system_supports_fa64())
>  		smcr |= SMCR_ELx_FA64;

Ditto comments on patch 1.

Unrelated to this patch, it is worth having a prctl for this?  The
architecture seems to discourage software written for the SME ISA to
implicitly rely on FA64, so it would useful to be able to run with it
disabled even on hardware that supports it.

> +	if (system_supports_sme2())
> +		smcr |= SMCR_ELx_EZT0;

Side question: since ZT0 is likely to be sporadically used, maybe it
is worth having separate lazy restore for it versus the main SME state?
(Not relevant for this series though, and probably best deferred until
there is hardware to benchmark on.  Also, ZT0 is small compared with
the SME state proper...)

[...]

Cheers
---Dave

Re: [PATCH 2/2] arm64/sme: Restore SMCR_EL1.EZT0 on exit from suspend

[Mark Brown]

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

On Tue, Jan 30, 2024 at 10:54:06AM +0000, Dave Martin wrote:
> On Tue, Jan 30, 2024 at 12:02:49AM +0000, Mark Brown wrote:

> > +	if (system_supports_sme2())
> > +		smcr |= SMCR_ELx_EZT0;

> Side question: since ZT0 is likely to be sporadically used, maybe it
> is worth having separate lazy restore for it versus the main SME state?
> (Not relevant for this series though, and probably best deferred until
> there is hardware to benchmark on.  Also, ZT0 is small compared with
> the SME state proper...)

One of the advantages SME has here is that we've got a clear indication
if userspace is actively using the registers through SMSTART and SMSTOP.
We only restore ZT0 at all whenever PSTATE.ZA is set and the strong
recommendation is that should only be set when either ZA or ZT0 are in
active use for power and performance reasons.  While it is likely that
there will be code that uses ZA but doesn't touch ZT0 I would expect
that the overhead of entering the kernel to do a lazy restore will be
sufficiently high for it to be an unreasonable penalty on code that does
touch it, as you say it's not *that* big compared to likely ZA sizes.

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

Re: [PATCH 2/2] arm64/sme: Restore SMCR_EL1.EZT0 on exit from suspend

[Dave Martin]

On Tue, Jan 30, 2024 at 02:34:23PM +0000, Mark Brown wrote:
> On Tue, Jan 30, 2024 at 10:54:06AM +0000, Dave Martin wrote:
> > On Tue, Jan 30, 2024 at 12:02:49AM +0000, Mark Brown wrote:
> 
> > > +	if (system_supports_sme2())
> > > +		smcr |= SMCR_ELx_EZT0;
> 
> > Side question: since ZT0 is likely to be sporadically used, maybe it
> > is worth having separate lazy restore for it versus the main SME state?
> > (Not relevant for this series though, and probably best deferred until
> > there is hardware to benchmark on.  Also, ZT0 is small compared with
> > the SME state proper...)
> 
> One of the advantages SME has here is that we've got a clear indication
> if userspace is actively using the registers through SMSTART and SMSTOP.
> We only restore ZT0 at all whenever PSTATE.ZA is set and the strong

Good point.  I was still thinking in SVE mode there.

> recommendation is that should only be set when either ZA or ZT0 are in
> active use for power and performance reasons.  While it is likely that
> there will be code that uses ZA but doesn't touch ZT0 I would expect
> that the overhead of entering the kernel to do a lazy restore will be
> sufficiently high for it to be an unreasonable penalty on code that does
> touch it, as you say it's not *that* big compared to likely ZA sizes.

Agreed.

Cheers
---Dave