Re: [PATCH v2 1/2] arm64: Move handling of erratum 1418040 into C code

[Will Deacon <will@kernel.org>]

On Fri, Jul 31, 2020 at 09:33:57AM +0100, Marc Zyngier wrote:
> Instead of dealing with erratum 1418040 on each entry and exit,
> let's move the handling to __switch_to() instead, which has
> several advantages:
> 
> - It can be applied when it matters (switching between 32 and 64
>   bit tasks).
> - It is written in C (yay!)
> - It can rely on static keys rather than alternatives
> 
> Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kernel/entry.S   | 21 ---------------------
>  arch/arm64/kernel/process.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 21 deletions(-)

[...]

> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 6089638c7d43..8bbf066224ab 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -515,6 +515,40 @@ static void entry_task_switch(struct task_struct *next)
>  	__this_cpu_write(__entry_task, next);
>  }
>  
> +/*
> + * ARM erratum 1418040 handling, affecting the 32bit view of CNTVCT.
> + * Assuming the virtual counter is enabled at the beginning of times:
> + *
> + * - disable access when switching from a 64bit task to a 32bit task
> + * - enable access when switching from a 32bit task to a 64bit task
> + */
> +static __always_inline

Do we need the __always_inline? None of the other calls from __switch_to()
have it.

> +void erratum_1418040_thread_switch(struct task_struct *prev,
> +				   struct task_struct *next)
> +{
> +	bool prev32, next32;
> +	u64 val;
> +
> +	if (!(IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&
> +	      cpus_have_const_cap(ARM64_WORKAROUND_1418040)))
> +		return;
> +
> +	prev32 = is_compat_thread(task_thread_info(prev));
> +	next32 = is_compat_thread(task_thread_info(next));
> +
> +	if (prev32 == next32)
> +		return;
> +
> +	val = read_sysreg(cntkctl_el1);
> +
> +	if (prev32 & !next32)

I know they're bools but this is perverse!

Why can't it just be:

	if (next32)
		val &= ~ARCH_TIMER_USR_VCT_ACCESS_EN;
	else
		val |= ARCH_TIMER_USR_VCT_ACCESS_EN;

?

Will

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