[1/2] arm64/sme: Restore SMCR on exit from suspend
Commit Message
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(+)
Comments
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
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.
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.
@@ -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)
{
@@ -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)
@@ -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();
}