Re: [PATCH 1/4] arm64: Restrict ARM64_ERRATUM_1188873 mitigation to AArch32

[Marc Zyngier <marc.zyngier@arm.com>]

On 08/04/2019 19:34, Robin Murphy wrote:
> Hi Marc,
> 
> On 08/04/2019 17:02, Marc Zyngier wrote:
>> We currently deal with ARM64_ERRATUM_1188873 by always trapping EL0
>> accesses for both instruction sets. Although nothing wrong comes out
>> of that, people trying to squeeze the last drop of performance from
>> buggy HW find this over the top. Oh well.
>>
>> Let's change the mitigation by flipping the counter enable bit
>> on return to userspace. Non-broken HW gets an extra branch on
>> the fast path, which is hopefully not the end of the world.
>> The arch timer workaround it also removed.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm64/kernel/entry.S            | 23 +++++++++++++++++++++++
>>   drivers/clocksource/arm_arch_timer.c | 15 ---------------
>>   2 files changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index c50a7a75f2e0..e0aed21ab402 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -336,6 +336,29 @@ alternative_if ARM64_WORKAROUND_845719
>>   alternative_else_nop_endif
>>   #endif
>>   3:
>> +#ifdef CONFIG_ARM64_ERRATUM_1188873
>> +alternative_if_not ARM64_WORKAROUND_1188873
>> +	b	1f
>> +alternative_else_nop_endif
>> +
>> +	ubfx	x0, x22, #4, #1			// Extract PSR_MODE32
>> +	eor	x0, x0, #1			// Negate it
>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> +	mrs	x1, cntkctl_el1
>> +alternative_else
>> +	mrs	x1, cnthctl_el2
>> +alternative_endif
>> +	ubfx	x2, x1, #1, #1			// Extract EL0VCTEN
>> +	cmp	x2, x0
>> +	b.eq	1f				// Matches, nothing to do
>> +	bfi	x1, x0, #1, #1			// Move EL0VCTEN in place
>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> +	msr	cntkctl_el1, x1
>> +alternative_else
>> +	msr	cnthctl_el2, x1
>> +alternative_endif
> 
> Unless I've gone horribly wrong in my reasoning somewhere, I reckon this 
> could be a fair bit shorter...
> 
> 
> alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> 	mrs	x1, cntkctl_el1
> alternative_else
> 	mrs	x1, cnthctl_el2
> alternative_endif
> 	eon	x0, x1, x22, lsr #3
> 	eor	x1, x1, #1
> 	tbz	x0, #1, 1f
> alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> 	msr	cntkctl_el1, x1
> alternative_else
> 	msr	cnthctl_el2, x1
> alternative_endif
> 
> 
> ...although whether that's a good idea at all probably depends mostly on 
> how many comments you think it needs :)

Neat! Totally impossible to decipher, and thus perfect! ;-) The way I
understand it that pstate.m32 and cntkctl_el1.el0vcten must always have
opposite values. If they don't, we flip el0vcten.

We could move the eor after the tbz, I think. And we can also get rid of
the VHE alternative, as cntkctl_el1 is a valid accessor for cnthctl_el2
in that case.

Shal we settle for this?

+#ifdef CONFIG_ARM64_ERRATUM_1188873
+alternative_if_not ARM64_WORKAROUND_1188873
+       b       1f
+alternative_else_nop_endif
+
+       // if (!x22.mode32 != cntkctl_el1.el0vcten)
+       //         cntkctl_el1.el0vcten = !cntkctl_el1.el0vcten
+
+       mrs     x1, cntkctl_el1
+
+       eon     x0, x1, x22, lsr #3
+       tbz     x0, #1, 1f
+
+       eor     x1, x1, #1
+       msr     cntkctl_el1, x1
+1:
+#endif

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

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