Re: [PATCH v2 07/23] arm64: cpufeature: Read MPAMIDR_EL1 in __cpuinfo_store_cpu()

[Ben Horgan <ben.horgan@arm.com>]

Hi Will,

On 10/9/26 11:07, Will Deacon wrote:
> From: Fuad Tabba <fuad.tabba@linux.dev>
> 
> MPAMIDR_EL1 is the one ID register __cpuinfo_store_cpu() doesn't read.
> Its read was deferred to init_cpu_features() and update_cpu_features()
> because it traps on firmware that fails to enable MPAM. Only the
> sanitised ID_AA64PFR0_EL1 and ID_AA64PFR1_EL1 carried the arm64.nompam
> override for such firmware.
> 
> Store ID_AA64PFR0_EL1 through read_cpuid_with_overrides() as well. Then
> read MPAMIDR_EL1 in __cpuinfo_store_cpu() again, beside GMID_EL1, gated
> on this CPU's overridden ID_AA64PFR0_EL1 and ID_AA64PFR1_EL1. cpu_data
> is then complete before init_cpu_features() runs.
> 
> Other readers of the stored ID_AA64PFR0_EL1 see the override too. With
> arm64.no32bit_el0, __cpuinfo_store_cpu() no longer reads the AArch32 ID
> registers. Adding allow_mismatched_32bit_el0 no longer enables 32-bit
> EL0 either.
> 
> detect_ftr_has_mpam(), a system-wide test on the sanitised values,
> stays for the ARM64_MPAM capability.
> 
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> Signed-off-by: Will Deacon <will@kernel.org>
> --->  arch/arm64/kernel/cpufeature.c | 10 +++-------
>  arch/arm64/kernel/cpuinfo.c    | 11 ++++-------
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 3161531ad401..9212f10c1d1a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1248,10 +1248,8 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
>  		cpacr_restore(cpacr);
>  	}
>  
> -	if (detect_ftr_has_mpam()) {
> -		info->reg_mpamidr = read_cpuid(MPAMIDR_EL1);
> +	if (id_aa64pfr0_mpam(info->reg_id_aa64pfr0) || id_aa64pfr1_mpamfrac(info->reg_id_aa64pfr1))
>  		init_cpu_ftr_reg(SYS_MPAMIDR_EL1, info->reg_mpamidr);

info->reg_mpamidr isn't useful if we aren't going to use MPAM so I
suppose all the info->reg_mpamidr accesses should be additionally
guarded with IS_ENABLED(CONFIG_ARM64_MPAM) as already done for MTE.
That's a separate change though and what you've got here looks good to
me. So, FWIW,

Reviewed-by: Ben Horgan <ben.horgan@arm.com>

Thanks,

Ben

> -	}
>  
>  	if (IS_ENABLED(CONFIG_ARM64_MTE) && id_aa64pfr1_mte(info->reg_id_aa64pfr1))
>  		init_cpu_ftr_reg(SYS_GMID_EL1, info->reg_gmid);
> @@ -1504,11 +1502,9 @@ void update_cpu_features(int cpu,
>  		cpacr_restore(cpacr);
>  	}
>  
> -	if (detect_ftr_has_mpam()) {
> -		info->reg_mpamidr = read_cpuid(MPAMIDR_EL1);
> +	if (id_aa64pfr0_mpam(info->reg_id_aa64pfr0) || id_aa64pfr1_mpamfrac(info->reg_id_aa64pfr1))
>  		taint |= check_update_ftr_reg(SYS_MPAMIDR_EL1, cpu,
> -					info->reg_mpamidr, boot->reg_mpamidr);
> -	}
> +					      info->reg_mpamidr, boot->reg_mpamidr);
>  
>  	/*
>  	 * The kernel uses the LDGM/STGM instructions and the number of tags
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index d48167fe4218..0ae40b0c7b2f 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -495,7 +495,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
>  	info->reg_id_aa64mmfr2 = read_cpuid(ID_AA64MMFR2_EL1);
>  	info->reg_id_aa64mmfr3 = read_cpuid(ID_AA64MMFR3_EL1);
>  	info->reg_id_aa64mmfr4 = read_cpuid(ID_AA64MMFR4_EL1);
> -	info->reg_id_aa64pfr0 = read_cpuid(ID_AA64PFR0_EL1);
> +	info->reg_id_aa64pfr0 = read_cpuid_with_overrides(ID_AA64PFR0_EL1);
>  	info->reg_id_aa64pfr1 = read_cpuid_with_overrides(ID_AA64PFR1_EL1);
>  	info->reg_id_aa64pfr2 = read_cpuid(ID_AA64PFR2_EL1);
>  	info->reg_id_aa64zfr0 = read_cpuid(ID_AA64ZFR0_EL1);
> @@ -505,15 +505,12 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
>  	if (IS_ENABLED(CONFIG_ARM64_MTE) && id_aa64pfr1_mte(info->reg_id_aa64pfr1))
>  		info->reg_gmid = read_cpuid(GMID_EL1);
>  
> +	if (id_aa64pfr0_mpam(info->reg_id_aa64pfr0) || id_aa64pfr1_mpamfrac(info->reg_id_aa64pfr1))
> +		info->reg_mpamidr = read_cpuid(MPAMIDR_EL1);
> +
>  	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
>  		__cpuinfo_store_cpu_32bit(&info->aarch32);
>  
> -	/*
> -	 * info->reg_mpamidr deferred to {init,update}_cpu_features because we
> -	 * don't want to read it (and trigger a trap on buggy firmware) if
> -	 * using an aa64pfr0_el1 override to unconditionally disable MPAM.
> -	 */
> -
>  	if (IS_ENABLED(CONFIG_ARM64_SME) &&
>  	    id_aa64pfr1_sme(info->reg_id_aa64pfr1)) {
>  		/*