| Message ID | 20210208165617.9977-7-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: MTE: Add async mode support | expand |
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 >
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 >
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.
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
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 >
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.
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.
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
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(+)