Re: [PATCH] clocksource/drivers/arm_arch_timer: limit XGene-1 workaround

[Marc Zyngier <maz@kernel.org>]

On Mon, 16 Oct 2023 16:31:27 +0100,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> The AppliedMicro XGene-1 CPU has an erratum where the timer condition
> would only consider TVAL, not CVAL. We currently apply a workaround when
> seeing the PartNum field of MIDR_EL1 being 0x000, under the assumption
> that this would match only the XGene-1 CPU model.
> However even the Ampere eMAG (aka XGene-3) uses that same part number, and
> only differs in the "Variant" and "Revision" fields: XGene-1's MIDR is
> 0x500f0000, our eMAG reports 0x503f0002. Experiments show the latter
> doesn't show the faulty behaviour.
> 
> Increase the specificity of the check to only consider partnum 0x000 and
> variant 0x00, to exclude the Ampere eMAG.
> 
> Fixes: 012f18850452 ("clocksource/drivers/arm_arch_timer: Work around broken CVAL implementations")
> Reported-by: Ross Burton <ross.burton@arm.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi,
> 
> I don't have access to an XGene-2 machine, so would be grateful if
> anyone from Ampere could either shed some light on the applicability or
> give it a test on such a machine.

I think these boxes are essentially "abandonware".

However, you can infer a couple of things from commit 0d4429301c4a and
xgene_edac_pmd_l2c_version1(), which seems to cover Variant==1 as well
as Variant==0 (and some other REVIDR shenanigans). I'm tempted to say
that XGene-2 is this Variant==1, but I have no idea wither it is
affected or not.

Given that nobody has ever seen this HW in the wild, I don't think
breaking the HW is a problem. Someone will shout if their unicorn is
broken.

> 
> And please let me know if the KVM bits require this to be split up in
> two patches.

Nah, it's just fine. This code path is now mostly unused.

> 
> Cheers,
> Andre
> 
>  arch/arm64/include/asm/cputype.h     | 3 ++-
>  arch/arm64/kvm/guest.c               | 2 +-
>  drivers/clocksource/arm_arch_timer.c | 5 +++--
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
> index 74d00feb62f03..7c7493cb571f9 100644
> --- a/arch/arm64/include/asm/cputype.h
> +++ b/arch/arm64/include/asm/cputype.h
> @@ -86,7 +86,8 @@
>  #define ARM_CPU_PART_NEOVERSE_N2	0xD49
>  #define ARM_CPU_PART_CORTEX_A78C	0xD4B
>  
> -#define APM_CPU_PART_POTENZA		0x000
> +#define APM_CPU_PART_XGENE		0x000
> +#define APM_CPU_VAR_POTENZA		0x00
>  
>  #define CAVIUM_CPU_PART_THUNDERX	0x0A1
>  #define CAVIUM_CPU_PART_THUNDERX_81XX	0x0A2
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 95f6945c44325..a1710e5fa72b6 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -874,7 +874,7 @@ u32 __attribute_const__ kvm_target_cpu(void)
>  		break;
>  	case ARM_CPU_IMP_APM:
>  		switch (part_number) {
> -		case APM_CPU_PART_POTENZA:
> +		case APM_CPU_PART_XGENE:
>  			return KVM_ARM_TARGET_XGENE_POTENZA;
>  		}
>  		break;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 7dd2c615bce23..071b04f1ee730 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -836,8 +836,9 @@ static u64 __arch_timer_check_delta(void)
>  		 * Note that TVAL is signed, thus has only 31 of its
>  		 * 32 bits to express magnitude.
>  		 */
> -		MIDR_ALL_VERSIONS(MIDR_CPU_MODEL(ARM_CPU_IMP_APM,
> -						 APM_CPU_PART_POTENZA)),
> +		MIDR_REV_RANGE(MIDR_CPU_MODEL(ARM_CPU_IMP_APM,
> +					      APM_CPU_PART_XGENE),
> +			       APM_CPU_VAR_POTENZA, 0x0, 0xf),
>  		{},
>  	};

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

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