* [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
@ 2021-11-10 22:07 Peter Collingbourne
2021-12-03 18:33 ` Peter Collingbourne
2021-12-10 12:06 ` Catalin Marinas
0 siblings, 2 replies; 6+ messages in thread
From: Peter Collingbourne @ 2021-11-10 22:07 UTC (permalink / raw)
To: Catalin Marinas, Vincenzo Frascino, Will Deacon,
Andrey Konovalov, Mark Rutland
Cc: Peter Collingbourne, Evgenii Stepanov, linux-arm-kernel
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(-)
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)
--
2.34.0.rc0.344.g81b53c2807-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
2021-11-10 22:07 [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary Peter Collingbourne
@ 2021-12-03 18:33 ` Peter Collingbourne
2021-12-10 12:06 ` Catalin Marinas
1 sibling, 0 replies; 6+ messages in thread
From: Peter Collingbourne @ 2021-12-03 18:33 UTC (permalink / raw)
To: Catalin Marinas, Vincenzo Frascino, Will Deacon,
Andrey Konovalov, Mark Rutland
Cc: Evgenii Stepanov, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
2021-11-10 22:07 [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary Peter Collingbourne
2021-12-03 18:33 ` Peter Collingbourne
@ 2021-12-10 12:06 ` Catalin Marinas
2021-12-16 2:44 ` Peter Collingbourne
1 sibling, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-10 12:06 UTC (permalink / raw)
To: Peter Collingbourne
Cc: Vincenzo Frascino, Will Deacon, Andrey Konovalov, Mark Rutland,
Evgenii Stepanov, linux-arm-kernel
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.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
2021-12-10 12:06 ` Catalin Marinas
@ 2021-12-16 2:44 ` Peter Collingbourne
2021-12-17 18:16 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: Peter Collingbourne @ 2021-12-16 2:44 UTC (permalink / raw)
To: Catalin Marinas
Cc: Vincenzo Frascino, Will Deacon, Andrey Konovalov, Mark Rutland,
Evgenii Stepanov, linux-arm-kernel
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/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
2021-12-16 2:44 ` Peter Collingbourne
@ 2021-12-17 18:16 ` Catalin Marinas
2022-01-22 1:03 ` Peter Collingbourne
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2021-12-17 18:16 UTC (permalink / raw)
To: Peter Collingbourne
Cc: Vincenzo Frascino, Will Deacon, Andrey Konovalov, Mark Rutland,
Evgenii Stepanov, linux-arm-kernel
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?
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary
2021-12-17 18:16 ` Catalin Marinas
@ 2022-01-22 1:03 ` Peter Collingbourne
0 siblings, 0 replies; 6+ messages in thread
From: Peter Collingbourne @ 2022-01-22 1:03 UTC (permalink / raw)
To: Catalin Marinas
Cc: Vincenzo Frascino, Will Deacon, Andrey Konovalov, Mark Rutland,
Evgenii Stepanov, Linux ARM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-22 1:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 22:07 [PATCH v2] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary Peter Collingbourne
2021-12-03 18:33 ` Peter Collingbourne
2021-12-10 12:06 ` Catalin Marinas
2021-12-16 2:44 ` Peter Collingbourne
2021-12-17 18:16 ` Catalin Marinas
2022-01-22 1:03 ` Peter Collingbourne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.