| Message ID | 20211110220735.3937127-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary | expand |
On Wed, Nov 10, 2021 at 2:07 PM Peter Collingbourne <pcc@google.com> wrote: > > On some microarchitectures, clearing PSTATE.TCO is expensive. Clearing > TCO is only necessary if in-kernel MTE is enabled, or if MTE is > enabled in the userspace process in synchronous (or, soon, asymmetric) > mode, because we do not report uaccess faults to userspace in none > or asynchronous modes. Therefore, adjust the kernel entry code to > clear TCO only if necessary. > > Because it is now possible to switch to a task in which TCO needs to > be clear from a task in which TCO is set, we also need to do the same > thing on task switch. > > Signed-off-by: Peter Collingbourne <pcc@google.com> > Link: https://linux-review.googlesource.com/id/I52d82a580bd0500d420be501af2c35fa8c90729e > --- > v2: > - do the same thing in cpu_switch_to() Ping. Peter
On Wed, Nov 10, 2021 at 02:07:35PM -0800, Peter Collingbourne wrote: > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 2f69ae43941d..a78ec15f5bbc 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -189,6 +189,27 @@ alternative_cb_end > #endif > .endm > > + .macro mte_clear_tco, sctlr > + /* > + * Re-enable tag checking (TCO set on exception entry). This is only > + * necessary if MTE is enabled in either the kernel or the userspace > + * task in synchronous mode. With MTE disabled in the kernel and > + * disabled or asynchronous in userspace, tag check faults (including in > + * uaccesses) are not reported, therefore there is no need to re-enable > + * checking. This is beneficial on microarchitectures where re-enabling > + * TCO is expensive. > + */ > +#ifdef CONFIG_ARM64_MTE > +alternative_cb kasan_hw_tags_enable > + tbz \sctlr, #SCTLR_EL1_TCF0_SHIFT, 1f > +alternative_cb_end > +alternative_if ARM64_MTE > + SET_PSTATE_TCO(0) > +alternative_else_nop_endif > +1: > +#endif > + .endm The patch looks fine to me but I recall in a private conversation with Mark he proposed the idea of moving this to entry-common.c (unless it was about something completely different). The downside is that we run with the TCO set for slightly longer. There shouldn't be a major drawback currently as we don't have stack tagging anyway.
On Fri, Dec 10, 2021 at 4:06 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Nov 10, 2021 at 02:07:35PM -0800, Peter Collingbourne wrote: > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > index 2f69ae43941d..a78ec15f5bbc 100644 > > --- a/arch/arm64/kernel/entry.S > > +++ b/arch/arm64/kernel/entry.S > > @@ -189,6 +189,27 @@ alternative_cb_end > > #endif > > .endm > > > > + .macro mte_clear_tco, sctlr > > + /* > > + * Re-enable tag checking (TCO set on exception entry). This is only > > + * necessary if MTE is enabled in either the kernel or the userspace > > + * task in synchronous mode. With MTE disabled in the kernel and > > + * disabled or asynchronous in userspace, tag check faults (including in > > + * uaccesses) are not reported, therefore there is no need to re-enable > > + * checking. This is beneficial on microarchitectures where re-enabling > > + * TCO is expensive. > > + */ > > +#ifdef CONFIG_ARM64_MTE > > +alternative_cb kasan_hw_tags_enable > > + tbz \sctlr, #SCTLR_EL1_TCF0_SHIFT, 1f > > +alternative_cb_end > > +alternative_if ARM64_MTE > > + SET_PSTATE_TCO(0) > > +alternative_else_nop_endif > > +1: > > +#endif > > + .endm > > The patch looks fine to me but I recall in a private conversation with > Mark he proposed the idea of moving this to entry-common.c (unless it > was about something completely different). The downside is that we run > with the TCO set for slightly longer. There shouldn't be a major > drawback currently as we don't have stack tagging anyway. Yes, Mark made that suggestion on the list. I tried it and found that it led to a performance regression relative to baseline [1]. Peter [1] https://lore.kernel.org/all/CAMn1gO51k1Dqts=THYq28nVMSvO6ZQB5sEG1wuzEVpAXBTfFjg@mail.gmail.com/
On Wed, Dec 15, 2021 at 06:44:03PM -0800, Peter Collingbourne wrote: > On Fri, Dec 10, 2021 at 4:06 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Nov 10, 2021 at 02:07:35PM -0800, Peter Collingbourne wrote: > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > > index 2f69ae43941d..a78ec15f5bbc 100644 > > > --- a/arch/arm64/kernel/entry.S > > > +++ b/arch/arm64/kernel/entry.S > > > @@ -189,6 +189,27 @@ alternative_cb_end > > > #endif > > > .endm > > > > > > + .macro mte_clear_tco, sctlr > > > + /* > > > + * Re-enable tag checking (TCO set on exception entry). This is only > > > + * necessary if MTE is enabled in either the kernel or the userspace > > > + * task in synchronous mode. With MTE disabled in the kernel and > > > + * disabled or asynchronous in userspace, tag check faults (including in > > > + * uaccesses) are not reported, therefore there is no need to re-enable > > > + * checking. This is beneficial on microarchitectures where re-enabling > > > + * TCO is expensive. > > > + */ > > > +#ifdef CONFIG_ARM64_MTE > > > +alternative_cb kasan_hw_tags_enable > > > + tbz \sctlr, #SCTLR_EL1_TCF0_SHIFT, 1f > > > +alternative_cb_end > > > +alternative_if ARM64_MTE > > > + SET_PSTATE_TCO(0) > > > +alternative_else_nop_endif > > > +1: > > > +#endif > > > + .endm > > > > The patch looks fine to me but I recall in a private conversation with > > Mark he proposed the idea of moving this to entry-common.c (unless it > > was about something completely different). The downside is that we run > > with the TCO set for slightly longer. There shouldn't be a major > > drawback currently as we don't have stack tagging anyway. > > Yes, Mark made that suggestion on the list. I tried it and found that > it led to a performance regression relative to baseline [1]. [...] > [1] https://lore.kernel.org/all/CAMn1gO51k1Dqts=THYq28nVMSvO6ZQB5sEG1wuzEVpAXBTfFjg@mail.gmail.com/ That's weird since there should be the same number of instructions executed on entry with your diff above. Could it be that mte_disable_tco_entry() is not inlined?
On Fri, Dec 17, 2021 at 10:17 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Dec 15, 2021 at 06:44:03PM -0800, Peter Collingbourne wrote: > > On Fri, Dec 10, 2021 at 4:06 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Wed, Nov 10, 2021 at 02:07:35PM -0800, Peter Collingbourne wrote: > > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > > > > index 2f69ae43941d..a78ec15f5bbc 100644 > > > > --- a/arch/arm64/kernel/entry.S > > > > +++ b/arch/arm64/kernel/entry.S > > > > @@ -189,6 +189,27 @@ alternative_cb_end > > > > #endif > > > > .endm > > > > > > > > + .macro mte_clear_tco, sctlr > > > > + /* > > > > + * Re-enable tag checking (TCO set on exception entry). This is only > > > > + * necessary if MTE is enabled in either the kernel or the userspace > > > > + * task in synchronous mode. With MTE disabled in the kernel and > > > > + * disabled or asynchronous in userspace, tag check faults (including in > > > > + * uaccesses) are not reported, therefore there is no need to re-enable > > > > + * checking. This is beneficial on microarchitectures where re-enabling > > > > + * TCO is expensive. > > > > + */ > > > > +#ifdef CONFIG_ARM64_MTE > > > > +alternative_cb kasan_hw_tags_enable > > > > + tbz \sctlr, #SCTLR_EL1_TCF0_SHIFT, 1f > > > > +alternative_cb_end > > > > +alternative_if ARM64_MTE > > > > + SET_PSTATE_TCO(0) > > > > +alternative_else_nop_endif > > > > +1: > > > > +#endif > > > > + .endm > > > > > > The patch looks fine to me but I recall in a private conversation with > > > Mark he proposed the idea of moving this to entry-common.c (unless it > > > was about something completely different). The downside is that we run > > > with the TCO set for slightly longer. There shouldn't be a major > > > drawback currently as we don't have stack tagging anyway. > > > > Yes, Mark made that suggestion on the list. I tried it and found that > > it led to a performance regression relative to baseline [1]. > [...] > > [1] https://lore.kernel.org/all/CAMn1gO51k1Dqts=THYq28nVMSvO6ZQB5sEG1wuzEVpAXBTfFjg@mail.gmail.com/ > > That's weird since there should be the same number of instructions > executed on entry with your diff above. Could it be that > mte_disable_tco_entry() is not inlined? Looks like it was something like that. In the kernel without the patch enter_from_user_mode() was empty which allowed the compiler to remove the call from its callers (el0_svc etc). In the kernel with the patch it contained one instruction (the MSR) and it wasn't inlined, apparently because of the noinstr annotation (which includes noinline; I guess removing a call to an empty function doesn't count as inlining). This was with a kernel version based on 5.10 (don't currently have a way to run a newer kernel version on this hardware unfortunately); looks like some refactoring patches have landed since then that allow enter_from_user_mode to be inlined. With a similar refactoring patch applied to the 5.10 kernel, things look fairly reasonable with the patch that I'm sending as v3. Looking at all combinations of {kasan off, kasan on} x {none, async, sync} x {big, medium, little} there are some small regressions but they are mostly on the little cores. I also tested the patch on non-MTE hardware (DragonBoard 845c) running the mainline kernel. This revealed that the added call to system_supports_mte() was necessary to avoid regressing things by too much on non-MTE hardware (avoids the load from current). With the v3 patch the syscall latency goes from 245.1ns to 247.4ns on the little cores and 151.4ns to 145.2ns on the big cores. Peter
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 2f69ae43941d..a78ec15f5bbc 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -189,6 +189,27 @@ alternative_cb_end #endif .endm + .macro mte_clear_tco, sctlr + /* + * Re-enable tag checking (TCO set on exception entry). This is only + * necessary if MTE is enabled in either the kernel or the userspace + * task in synchronous mode. With MTE disabled in the kernel and + * disabled or asynchronous in userspace, tag check faults (including in + * uaccesses) are not reported, therefore there is no need to re-enable + * checking. This is beneficial on microarchitectures where re-enabling + * TCO is expensive. + */ +#ifdef CONFIG_ARM64_MTE +alternative_cb kasan_hw_tags_enable + tbz \sctlr, #SCTLR_EL1_TCF0_SHIFT, 1f +alternative_cb_end +alternative_if ARM64_MTE + SET_PSTATE_TCO(0) +alternative_else_nop_endif +1: +#endif + .endm + .macro kernel_entry, el, regsize = 64 .if \regsize == 32 mov w0, w0 // zero upper 32 bits of x0 @@ -269,7 +290,11 @@ alternative_else_nop_endif .else add x21, sp, #PT_REGS_SIZE get_current_task tsk + ldr x0, [tsk, THREAD_SCTLR_USER] .endif /* \el == 0 */ + + mte_clear_tco x0 + mrs x22, elr_el1 mrs x23, spsr_el1 stp lr, x21, [sp, #S_LR] @@ -308,13 +333,6 @@ alternative_if ARM64_HAS_IRQ_PRIO_MASKING msr_s SYS_ICC_PMR_EL1, x20 alternative_else_nop_endif - /* Re-enable tag checking (TCO set on exception entry) */ -#ifdef CONFIG_ARM64_MTE -alternative_if ARM64_MTE - SET_PSTATE_TCO(0) -alternative_else_nop_endif -#endif - /* * Registers that may be useful after this macro is invoked: * @@ -742,6 +760,8 @@ SYM_FUNC_START(cpu_switch_to) ptrauth_keys_install_kernel x1, x8, x9, x10 scs_save x0 scs_load x1 + ldr x8, [x1, THREAD_SCTLR_USER] + mte_clear_tco x8 ret SYM_FUNC_END(cpu_switch_to) NOKPROBE(cpu_switch_to)
On some microarchitectures, clearing PSTATE.TCO is expensive. Clearing TCO is only necessary if in-kernel MTE is enabled, or if MTE is enabled in the userspace process in synchronous (or, soon, asymmetric) mode, because we do not report uaccess faults to userspace in none or asynchronous modes. Therefore, adjust the kernel entry code to clear TCO only if necessary. Because it is now possible to switch to a task in which TCO needs to be clear from a task in which TCO is set, we also need to do the same thing on task switch. Signed-off-by: Peter Collingbourne <pcc@google.com> Link: https://linux-review.googlesource.com/id/I52d82a580bd0500d420be501af2c35fa8c90729e --- v2: - do the same thing in cpu_switch_to() arch/arm64/kernel/entry.S | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)