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

[Marc Zyngier]

On Mon, 16 Dec 2024 12:38:17 +0000,
Mark Rutland <mark.rutland@xxxxxxx> wrote:
> 
> On Sat, Dec 14, 2024 at 10:56:13AM +0000, Marc Zyngier wrote:
>
> > Why isn't the following a good enough fix? It makes it plain that
> > boot_cpu_data is only a copy of CPU0's initial boot state.
> > 
> > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> > index d79e88fccdfce..0cbb42fd48850 100644
> > --- a/arch/arm64/kernel/cpuinfo.c
> > +++ b/arch/arm64/kernel/cpuinfo.c
> > @@ -497,6 +497,6 @@ void __init cpuinfo_store_boot_cpu(void)
> >  	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
> >  	__cpuinfo_store_cpu(info);
> >  
> > +	init_cpu_features(info);
> >  	boot_cpu_data = *info;
> > -	init_cpu_features(&boot_cpu_data);
> >  }
> 
> I think that change in isolation is fine, but I don't think that's the
> right fix.
> 
> 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.

> 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.