diff mbox series

[v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary

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

Commit Message

Peter Collingbourne Nov. 10, 2021, 10:07 p.m. UTC
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(-)

Comments

Peter Collingbourne Dec. 3, 2021, 6:33 p.m. UTC | #1
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
Catalin Marinas Dec. 10, 2021, 12:06 p.m. UTC | #2
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.
Peter Collingbourne Dec. 16, 2021, 2:44 a.m. UTC | #3
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/
Catalin Marinas Dec. 17, 2021, 6:16 p.m. UTC | #4
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?
Peter Collingbourne Jan. 22, 2022, 1:03 a.m. UTC | #5
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 mbox series

Patch

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)