Re: [PATCH v3] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Jan 21, 2022 at 05:02:50PM -0800, Peter Collingbourne wrote:
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 075539f5f1c8..5352db4c0f45 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -11,7 +11,9 @@
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/bitfield.h>
> +#include <linux/kasan.h>
>  #include <linux/page-flags.h>
> +#include <linux/sched.h>
>  #include <linux/types.h>
>  
>  #include <asm/pgtable-types.h>
> @@ -86,6 +88,23 @@ static inline int mte_ptrace_copy_tags(struct task_struct *child,
>  
>  #endif /* CONFIG_ARM64_MTE */
>  
> +static inline void mte_disable_tco_entry(struct task_struct *task)
> +{
> +	/*
> +	 * 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.
> +	 */

I'd add a note here that the 1ULL << SCTLR_EL1_TCF0_SHIFT is meant to
check for both synchronous and asymmetric modes even if we don't have
the latter supporting the user yet. We do have the definitions already.

> +	if (kasan_hw_tags_enabled() ||
> +	    (system_supports_mte() &&
> +	     (task->thread.sctlr_user & (1UL << SCTLR_EL1_TCF0_SHIFT))))
> +		asm volatile(SET_PSTATE_TCO(0));
> +}

Does it make a difference in code generation if you place a:

	if (!system_supports_mte())
		return;

at the beginning of the function (and remove the subsequent check)? It's
probably also easier to read, though the code generation depends on the
likely/unlikely choices for the static branches involved.

> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index f418ebc65f95..5345587f3384 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -252,6 +252,7 @@ void mte_thread_switch(struct task_struct *next)
>  
>  	mte_update_sctlr_user(next);
>  	mte_update_gcr_excl(next);
> +	mte_disable_tco_entry(next);

Maybe a one-line comment here that TCO may not have been disabled on
exception entry for the current task.

Otherwise it looks good to me:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel