Re: [PATCH] arm64/sme: Move storage of reg_smidr to __cpuinfo_store_cpu()

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Dec 16, 2024 at 02:31:43PM +0000, Marc Zyngier wrote:
> On Mon, 16 Dec 2024 12:38:17 +0000,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > I think that what we did in commit:
> > 
> >    892f7237b3ff ("arm64: Delay initialisation of cpuinfo_arm64::reg_{zcr,smcr}")
> > 
> > ... introduces an anti-pattern that'd be nice to avoid. That broke the
> > existing split of __cpuinfo_store_cpu() and init_cpu_features(), where
> > the former read the ID regs, and the latter set up the features
> > *without* altering the copy of the ID regs that was read. i.e.
> > init_cpu_features() shouldn't write to its info argument at all.
> > 
> > I understand that we have to do something as a bodge for broken FW which
> > traps SME, but I'd much rather we did that within __cpuinfo_store_cpu().
> 
> Honestly, I'd rather revert that patch, together with b3000e2133d8
> ("arm64: Add the arm64.nosme command line option"). I'm getting tired
> of the FW nonsense, and we are only allowing vendors to ship untested
> crap.
> 
> Furthermore, given the state of SME in the kernel, I don't think this
> is makes any difference. So maybe this is the right time to reset
> everything to a sane state.

Looking again, a revert does look to be the best option.

We removed reg_zcr and reg_smcr in v6.7 in commits:

  abef0695f9665c3d ("arm64/sve: Remove ZCR pseudo register from cpufeature code")
  391208485c3ad50f ("arm64/sve: Remove SMCR pseudo register from cpufeature code")

As of those commits, ZCR and SCMR no longer matter to
__cpuinfo_store_cpu(), and only SMIDR_EL1 remains...

Per ARM DDI 0487 L.a, accesses to SMIDR_EL1 never trap to EL3, so we can
read that safely as long as ID_AA64PFR1_EL1.SME indicates that SME is
implemented.

Which is to say that if we revert the remaining portion of 892f7237b3ff
and restore the read of SMIDR, that should be good as far back as v6.7,
which sounds good to me.

Mark.

> > Can we add something to check whether SME was disabled on the command
> > line, and use that in __cpuinfo_store_cpu(), effectively reverting
> > 892f7237b3ff?
> 
> Maybe, but that'd be before any sanitisation of the overrides, so it
> would have to severely limit its scope. Something like this, which I
> haven't tested:
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index d79e88fccdfce..9e9295e045009 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -492,10 +492,22 @@ void cpuinfo_store_cpu(void)
>  	update_cpu_features(smp_processor_id(), info, &boot_cpu_data);
>  }
>  
> +static void cpuinfo_apply_overrides(struct cpuinfo_arm64 *info)
> +{
> +	if (FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.mask) &&
> +	    !FIELD_GET(ID_AA64PFR0_EL1_SVE, id_aa64pfr0_override.val))
> +		info->reg_id_aa64pfr0 &= ~ID_AA64PFR0_EL1_SVE;
> +
> +	if (FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.mask) &&
> +	    !FIELD_GET(ID_AA64PFR1_EL1_SME, id_aa64pfr1_override.val))
> +		info->reg_id_aa64pfr1 &= ~ID_AA64PFR1_EL1_SME;
> +}
> +
>  void __init cpuinfo_store_boot_cpu(void)
>  {
>  	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
>  	__cpuinfo_store_cpu(info);
> +	cpuinfo_apply_overrides(info);
>  
>  	boot_cpu_data = *info;
>  	init_cpu_features(&boot_cpu_data);
> 
> But this will have ripple effects on the rest of the override code
> (the kernel messages are likely to be wrong).
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.