Query regarding ERRATUM_1418040

[Marc Zyngier]

On 2020-07-06 13:09, Will Deacon wrote:
> 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.

Indeed.

>> +#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?

You wish. Turns out that BIC is labelled as a v8.2 instruction, even
if is just yet another alias for BFM, and gas sends me packing
if I dare using it.

So the above BFI is a poor man's version of BIC. Which objdump happily
disassembles as... BIC! Yes, this is all braindead.

Anyway, I'll fix this up and repost it together with the rest of the
vdso series.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...