[v12,6/7] arm64: mte: Save/Restore TFSR_EL1 during suspend
diff mbox series

Message ID 20210208165617.9977-7-vincenzo.frascino@arm.com
State New, archived
Headers show
Series
  • arm64: ARMv8.5-A: MTE: Add async mode support
Related show

Commit Message

Vincenzo Frascino Feb. 8, 2021, 4:56 p.m. UTC
When MTE async mode is enabled TFSR_EL1 contains the accumulative
asynchronous tag check faults for EL1 and EL0.

During the suspend/resume operations the firmware might perform some
operations that could change the state of the register resulting in
a spurious tag check fault report.

Save/restore the state of the TFSR_EL1 register during the
suspend/resume operations to prevent this to happen.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h |  4 ++++
 arch/arm64/kernel/mte.c      | 22 ++++++++++++++++++++++
 arch/arm64/kernel/suspend.c  |  3 +++
 3 files changed, 29 insertions(+)

Comments

Lorenzo Pieralisi Feb. 8, 2021, 6:56 p.m. UTC | #1
On Mon, Feb 08, 2021 at 04:56:16PM +0000, Vincenzo Frascino wrote:
> When MTE async mode is enabled TFSR_EL1 contains the accumulative
> asynchronous tag check faults for EL1 and EL0.
> 
> During the suspend/resume operations the firmware might perform some
> operations that could change the state of the register resulting in
> a spurious tag check fault report.
> 
> Save/restore the state of the TFSR_EL1 register during the
> suspend/resume operations to prevent this to happen.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/mte.h |  4 ++++
>  arch/arm64/kernel/mte.c      | 22 ++++++++++++++++++++++
>  arch/arm64/kernel/suspend.c  |  3 +++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 237bb2f7309d..2d79bcaaeb30 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -43,6 +43,7 @@ void mte_sync_tags(pte_t *ptep, pte_t pte);
>  void mte_copy_page_tags(void *kto, const void *kfrom);
>  void flush_mte_state(void);
>  void mte_thread_switch(struct task_struct *next);
> +void mte_suspend_enter(void);
>  void mte_suspend_exit(void);
>  long set_mte_ctrl(struct task_struct *task, unsigned long arg);
>  long get_mte_ctrl(struct task_struct *task);
> @@ -68,6 +69,9 @@ static inline void flush_mte_state(void)
>  static inline void mte_thread_switch(struct task_struct *next)
>  {
>  }
> +static inline void mte_suspend_enter(void)
> +{
> +}
>  static inline void mte_suspend_exit(void)
>  {
>  }
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 3332aabda466..5c440967721b 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -25,6 +25,7 @@
>  
>  u64 gcr_kernel_excl __ro_after_init;
>  
> +static u64 mte_suspend_tfsr_el1;

IIUC you need this per-CPU (core loses context on suspend-to-RAM but also
CPUidle, S2R is single threaded but CPUidle runs on every core idle
thread).

Unless you sync/report it on enter/exit (please note: I am not familiar
with MTE so it is just a, perhaps silly, suggestion to avoid
saving/restoring it).

Lorenzo

>  static bool report_fault_once = true;
>  
>  /* Whether the MTE asynchronous mode is enabled. */
> @@ -295,12 +296,33 @@ void mte_thread_switch(struct task_struct *next)
>  	mte_check_tfsr_el1();
>  }
>  
> +void mte_suspend_enter(void)
> +{
> +	if (!system_supports_mte())
> +		return;
> +
> +	/*
> +	 * The barriers are required to guarantee that the indirect writes
> +	 * to TFSR_EL1 are synchronized before we save the state.
> +	 */
> +	dsb(nsh);
> +	isb();
> +
> +	/* Save SYS_TFSR_EL1 before suspend entry */
> +	mte_suspend_tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
> +}
> +
>  void mte_suspend_exit(void)
>  {
>  	if (!system_supports_mte())
>  		return;
>  
>  	update_gcr_el1_excl(gcr_kernel_excl);
> +
> +	/* Resume SYS_TFSR_EL1 after suspend exit */
> +	write_sysreg_s(mte_suspend_tfsr_el1, SYS_TFSR_EL1);
> +
> +	mte_check_tfsr_el1();
>  }
>  
>  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index a67b37a7a47e..16caa9b32dae 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -91,6 +91,9 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  	unsigned long flags;
>  	struct sleep_stack_data state;
>  
> +	/* Report any MTE async fault before going to suspend. */
> +	mte_suspend_enter();
> +
>  	/*
>  	 * From this point debug exceptions are disabled to prevent
>  	 * updates to mdscr register (saved and restored along with
> -- 
> 2.30.0
>
Vincenzo Frascino Feb. 9, 2021, 10:41 a.m. UTC | #2
Hi Lorenzo,

thank you for your review.

On 2/8/21 6:56 PM, Lorenzo Pieralisi wrote:
>>  u64 gcr_kernel_excl __ro_after_init;
>>  
>> +static u64 mte_suspend_tfsr_el1;
> IIUC you need this per-CPU (core loses context on suspend-to-RAM but also
> CPUidle, S2R is single threaded but CPUidle runs on every core idle
> thread).
> 
> Unless you sync/report it on enter/exit (please note: I am not familiar
> with MTE so it is just a, perhaps silly, suggestion to avoid
> saving/restoring it).
> 

I thought about making it per cpu, but I concluded that since it is an
asynchronous tag fault it wasn't necessary.

But thinking at it from the statistical point of view what you are saying is
completely right, because we might end up in scenario in which we report the
fault on multiple cores when it happens on one or in a scenario in which we do
not report the potential fault at all.

I am going to update my code accordingly in the next version.

Thanks!

> Lorenzo
>
Catalin Marinas Feb. 9, 2021, 11:55 a.m. UTC | #3
On Mon, Feb 08, 2021 at 04:56:16PM +0000, Vincenzo Frascino wrote:
> When MTE async mode is enabled TFSR_EL1 contains the accumulative
> asynchronous tag check faults for EL1 and EL0.
> 
> During the suspend/resume operations the firmware might perform some
> operations that could change the state of the register resulting in
> a spurious tag check fault report.
> 
> Save/restore the state of the TFSR_EL1 register during the
> suspend/resume operations to prevent this to happen.

Do we need a similar fix for TFSRE0_EL1? We get away with this if
suspend is only entered on the idle (kernel) thread but I recall we
could also enter suspend on behalf of a user process (I may be wrong
though).

If that's the case, it would make more sense to store the TFSR* regs in
the thread_struct alongside sctlr_tcf0. If we did that, we'd not need
the per-cpu mte_suspend_tfsr_el1 variable.
Lorenzo Pieralisi Feb. 9, 2021, 2:33 p.m. UTC | #4
On Tue, Feb 09, 2021 at 11:55:33AM +0000, Catalin Marinas wrote:
> On Mon, Feb 08, 2021 at 04:56:16PM +0000, Vincenzo Frascino wrote:
> > When MTE async mode is enabled TFSR_EL1 contains the accumulative
> > asynchronous tag check faults for EL1 and EL0.
> > 
> > During the suspend/resume operations the firmware might perform some
> > operations that could change the state of the register resulting in
> > a spurious tag check fault report.
> > 
> > Save/restore the state of the TFSR_EL1 register during the
> > suspend/resume operations to prevent this to happen.
> 
> Do we need a similar fix for TFSRE0_EL1? We get away with this if
> suspend is only entered on the idle (kernel) thread but I recall we
> could also enter suspend on behalf of a user process (I may be wrong
> though).

Yes, when we suspend the machine to RAM, we execute suspend on behalf
on a userspace process (but that's only running on 1 cpu, the others
are hotplugged out).

IIUC (and that's an if) TFSRE0_EL1 is checked on kernel entry so I don't
think there is a need to save/restore it (just reset it on suspend
exit).

TFSR_EL1, I don't see a point in saving/restoring it (it is a bit
per-CPU AFAICS) either, IMO we should "check" it on suspend (if it is
possible in that context) and reset it on resume.

I don't think though you can "check" with IRQs disabled so I suspect
that TFSR_EL1 has to be saved/restored (which means that there is a
black out period where we run kernel code without being able to detect
faults but there is no solution to that other than delaying saving the
value to just before calling into PSCI). Likewise on resume from low
power.

Thanks,
Lorenzo

> If that's the case, it would make more sense to store the TFSR* regs in
> the thread_struct alongside sctlr_tcf0. If we did that, we'd not need
> the per-cpu mte_suspend_tfsr_el1 variable.
> 
> -- 
> Catalin
Vincenzo Frascino Feb. 9, 2021, 2:54 p.m. UTC | #5
On 2/9/21 2:33 PM, Lorenzo Pieralisi wrote:
>> Do we need a similar fix for TFSRE0_EL1? We get away with this if
>> suspend is only entered on the idle (kernel) thread but I recall we
>> could also enter suspend on behalf of a user process (I may be wrong
>> though).
> Yes, when we suspend the machine to RAM, we execute suspend on behalf
> on a userspace process (but that's only running on 1 cpu, the others
> are hotplugged out).
> 
> IIUC (and that's an if) TFSRE0_EL1 is checked on kernel entry so I don't
> think there is a need to save/restore it (just reset it on suspend
> exit).
> 
> TFSR_EL1, I don't see a point in saving/restoring it (it is a bit
> per-CPU AFAICS) either, IMO we should "check" it on suspend (if it is
> possible in that context) and reset it on resume.
> 
> I don't think though you can "check" with IRQs disabled so I suspect
> that TFSR_EL1 has to be saved/restored (which means that there is a
> black out period where we run kernel code without being able to detect
> faults but there is no solution to that other than delaying saving the
> value to just before calling into PSCI). Likewise on resume from low
> power.
> 

Ok, based on what you are saying it seems that the most viable solution here is
to save and restore TFSR_EL1. I will update my code accordingly.

> Thanks,
> Lorenzo
>
Catalin Marinas Feb. 9, 2021, 5:28 p.m. UTC | #6
On Tue, Feb 09, 2021 at 02:33:28PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Feb 09, 2021 at 11:55:33AM +0000, Catalin Marinas wrote:
> > On Mon, Feb 08, 2021 at 04:56:16PM +0000, Vincenzo Frascino wrote:
> > > When MTE async mode is enabled TFSR_EL1 contains the accumulative
> > > asynchronous tag check faults for EL1 and EL0.
> > > 
> > > During the suspend/resume operations the firmware might perform some
> > > operations that could change the state of the register resulting in
> > > a spurious tag check fault report.
> > > 
> > > Save/restore the state of the TFSR_EL1 register during the
> > > suspend/resume operations to prevent this to happen.
> > 
> > Do we need a similar fix for TFSRE0_EL1? We get away with this if
> > suspend is only entered on the idle (kernel) thread but I recall we
> > could also enter suspend on behalf of a user process (I may be wrong
> > though).
> 
> Yes, when we suspend the machine to RAM, we execute suspend on behalf
> on a userspace process (but that's only running on 1 cpu, the others
> are hotplugged out).
> 
> IIUC (and that's an if) TFSRE0_EL1 is checked on kernel entry so I don't
> think there is a need to save/restore it (just reset it on suspend
> exit).

You are right, we don't check TFSRE0_EL1 on return to user, only
clear it, so no need to do anything on suspend/resume.

> TFSR_EL1, I don't see a point in saving/restoring it (it is a bit
> per-CPU AFAICS) either, IMO we should "check" it on suspend (if it is
> possible in that context) and reset it on resume.

I think this should work.

> I don't think though you can "check" with IRQs disabled so I suspect
> that TFSR_EL1 has to be saved/restored (which means that there is a
> black out period where we run kernel code without being able to detect
> faults but there is no solution to that other than delaying saving the
> value to just before calling into PSCI). Likewise on resume from low
> power.

It depends on whether kasan_report can be called with IRQs disabled. I
don't see why not, so if this works I'd rather just call mte_check_async
(or whatever it's called) on the suspend path and zero the register on
resume (mte_suspend_exit). We avoid any saving of the state.
Vincenzo Frascino Feb. 9, 2021, 6:25 p.m. UTC | #7
On 2/9/21 5:28 PM, Catalin Marinas wrote:
>> I don't think though you can "check" with IRQs disabled so I suspect
>> that TFSR_EL1 has to be saved/restored (which means that there is a
>> black out period where we run kernel code without being able to detect
>> faults but there is no solution to that other than delaying saving the
>> value to just before calling into PSCI). Likewise on resume from low
>> power.
> It depends on whether kasan_report can be called with IRQs disabled. I
> don't see why not, so if this works I'd rather just call mte_check_async
> (or whatever it's called) on the suspend path and zero the register on
> resume (mte_suspend_exit). We avoid any saving of the state.

Fine by me, I tried a quick test and can confirm that kasan_report can be
invoked with IRQ disabled.

Patch
diff mbox series

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 237bb2f7309d..2d79bcaaeb30 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -43,6 +43,7 @@  void mte_sync_tags(pte_t *ptep, pte_t pte);
 void mte_copy_page_tags(void *kto, const void *kfrom);
 void flush_mte_state(void);
 void mte_thread_switch(struct task_struct *next);
+void mte_suspend_enter(void);
 void mte_suspend_exit(void);
 long set_mte_ctrl(struct task_struct *task, unsigned long arg);
 long get_mte_ctrl(struct task_struct *task);
@@ -68,6 +69,9 @@  static inline void flush_mte_state(void)
 static inline void mte_thread_switch(struct task_struct *next)
 {
 }
+static inline void mte_suspend_enter(void)
+{
+}
 static inline void mte_suspend_exit(void)
 {
 }
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 3332aabda466..5c440967721b 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -25,6 +25,7 @@ 
 
 u64 gcr_kernel_excl __ro_after_init;
 
+static u64 mte_suspend_tfsr_el1;
 static bool report_fault_once = true;
 
 /* Whether the MTE asynchronous mode is enabled. */
@@ -295,12 +296,33 @@  void mte_thread_switch(struct task_struct *next)
 	mte_check_tfsr_el1();
 }
 
+void mte_suspend_enter(void)
+{
+	if (!system_supports_mte())
+		return;
+
+	/*
+	 * The barriers are required to guarantee that the indirect writes
+	 * to TFSR_EL1 are synchronized before we save the state.
+	 */
+	dsb(nsh);
+	isb();
+
+	/* Save SYS_TFSR_EL1 before suspend entry */
+	mte_suspend_tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
+}
+
 void mte_suspend_exit(void)
 {
 	if (!system_supports_mte())
 		return;
 
 	update_gcr_el1_excl(gcr_kernel_excl);
+
+	/* Resume SYS_TFSR_EL1 after suspend exit */
+	write_sysreg_s(mte_suspend_tfsr_el1, SYS_TFSR_EL1);
+
+	mte_check_tfsr_el1();
 }
 
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index a67b37a7a47e..16caa9b32dae 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -91,6 +91,9 @@  int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 	unsigned long flags;
 	struct sleep_stack_data state;
 
+	/* Report any MTE async fault before going to suspend. */
+	mte_suspend_enter();
+
 	/*
 	 * From this point debug exceptions are disabled to prevent
 	 * updates to mdscr register (saved and restored along with