Query regarding ERRATUM_1418040

[Will Deacon]

On Thu, Jul 02, 2020 at 09:19:04AM +0100, Marc Zyngier wrote:
> On 2020-07-01 17:27, Will Deacon wrote:
> > On Wed, Jun 17, 2020 at 03:29:46PM +0100, Marc Zyngier wrote:
> > > On 2020-06-17 12:25, Will Deacon wrote:
> > > > Hmm, but in conjunction with the previous point, doesn't this mean if
> > > > some CPUs are affected by an erratum which requires CNTVCT trapping for
> > > > AArch64 and others are affected by 1418040, then the former won't
> > > > actually
> > > > be trapped?
> > > 
> > > Indeed. Having CPUs that require opposite workarounds is one of the
> > > many
> > > fascinating aspects of BL systems... :-/ Does such a system exist
> > > today?
> > 
> > I don't know, but it feels like we should either address the issue of
> > scream
> > loudly if we detect it!
> 
> I have no idea how you plan to detect that.

You'd end up having to have workarounds know about each other and, yes,
it would be awful.

> > I think re-enabling on entry from a 32-bit task would be the easiest
> > thing to
> > do. Since you're playing with 32-bit timer bugs atm, do you fancy taking
> > a
> > look ;)
> 
> I came up with this, tested in a guest only by fudging the detection
> code (I don't have the offending HW at hand). The use of branches vs
> NOPs is debatable, no strong opinion here.
> 
>         M.
> 
> From 81126933d6c990dac7213d0ec66c4a9df21fe8b8 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz at kernel.org>
> Date: Wed, 1 Jul 2020 21:29:24 +0100
> Subject: [PATCH] arm64: Rework ARM_ERRATUM_1414080 handling
> 
> The current handling of erratum 1414080 has the side effect that
> cntkctl_el1 can get changed for both 32 and 64bit tasks.
> 
> This isn't a problem so far, but if we ever need to mitigate another
> of these errata on the 64bit side, we'd better keep the messing with
> cntkctl_el1 local to 32bit tasks.
> 
> For that, make sure that on entering the kernel from a 32bit tasks,
> userspace access to cntvct gets enabled, and disabled returning to
> userspace, while it never gets changed for 64bit tasks.
> 
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>  arch/arm64/kernel/entry.S | 44 +++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 16 deletions(-)

Mostly looks fine, just some small nits below. Looking at it now, though,
I suspect it would make more sense to do this at context-switch. We can
do that later on.

> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 5304d193c79d..357bce62c31e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -167,6 +167,19 @@ alternative_cb_end
>  	stp	x28, x29, [sp, #16 * 14]
> 
>  	.if	\el == 0
> +	.if	\regsize == 32
> +	// If we come back from a 32bit task on a system affected by
> +	// 1418040, let's reenable userspace access to the virtual counter.
> +#ifdef CONFIG_ARM64_ERRATUM_1418040
> +alternative_if_not ARM64_WORKAROUND_1418040
> +	b	.L__entry_wa_1418040\@
> +alternative_else_nop_endif
> +	mrs	x0, cntkctl_el1
> +	orr	x0, x0, #2	// ARCH_TIMER_USR_VCT_ACCESS_EN
> +	msr	cntkctl_el1, x0
> +.L__entry_wa_1418040\@:

nit: naming this .L__entry_skip_wa_1418040\@ would be clearer to me,
as we take the branch to jump over the workaround.

> +#endif
> +	.endif
>  	clear_gp_regs
>  	mrs	x21, sp_el0
>  	ldr_this_cpu	tsk, __entry_task, x20
> @@ -318,7 +331,21 @@ alternative_else_nop_endif
>  	ldr	x23, [sp, #S_SP]		// load return stack pointer
>  	msr	sp_el0, x23
>  	tst	x22, #PSR_MODE32_BIT		// native task?
> -	b.eq	3f
> +	b.eq	4f
> +
> +#ifdef CONFIG_ARM64_ERRATUM_1418040
> +alternative_if_not ARM64_WORKAROUND_1418040
> +	b	3f
> +alternative_else_nop_endif
> +	/*
> +	 * if (x22.mode32 == 1)
> +	 *     cntkctl_el1.el0vcten = 0
> +	 */

I think we can probably drop this comment now, as I find it more confusing
that helpful.

> +	mrs	x1, cntkctl_el1
> +	bfi	x1, xzr, #1, #1	// ARCH_TIMER_USR_VCT_ACCESS_EN

Can you use BIC here?

Will