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

[Robin Murphy <robin.murphy@arm.com>]

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 :)

Robin.

> +1:
> +#endif
>   	apply_ssbd 0, x0, x1
>   	.endif
>   
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5fcccc467868..27acc9eb0f7c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -339,13 +339,6 @@ static u64 notrace arm64_858921_read_cntvct_el0(void)
>   }
>   #endif
>   
> -#ifdef CONFIG_ARM64_ERRATUM_1188873
> -static u64 notrace arm64_1188873_read_cntvct_el0(void)
> -{
> -	return read_sysreg(cntvct_el0);
> -}
> -#endif
> -
>   #ifdef CONFIG_SUN50I_ERRATUM_UNKNOWN1
>   /*
>    * The low bits of the counter registers are indeterminate while bit 10 or
> @@ -476,14 +469,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
>   		.read_cntvct_el0 = arm64_858921_read_cntvct_el0,
>   	},
>   #endif
> -#ifdef CONFIG_ARM64_ERRATUM_1188873
> -	{
> -		.match_type = ate_match_local_cap_id,
> -		.id = (void *)ARM64_WORKAROUND_1188873,
> -		.desc = "ARM erratum 1188873",
> -		.read_cntvct_el0 = arm64_1188873_read_cntvct_el0,
> -	},
> -#endif
>   #ifdef CONFIG_SUN50I_ERRATUM_UNKNOWN1
>   	{
>   		.match_type = ate_match_dt,
> 

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