Re: [PATCH v2 01/11] arm64: Skip update of an idreg field affected by an override

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Mar 25, 2026 at 02:54:28PM +0000, Suzuki K Poulose wrote:
> On 19/03/2026 15:34, Catalin Marinas wrote:
> > On Mon, Mar 02, 2026 at 11:56:42AM +0000, Marc Zyngier wrote:
> > > When computing the new value od an idreg that contains a field
> > > affected by an override, do not update that particular field.
> > > 
> > > The value computed at init-time must be kept as-is, as that's
> > > what the user has asked for, for better or worse.
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >   arch/arm64/kernel/cpufeature.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index c31f8e17732a3..28fc77443ccd3 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1224,6 +1224,13 @@ static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
> > >   		s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
> > >   		s64 ftr_new = arm64_ftr_value(ftrp, new);
> > > +		/*
> > > +		 * Don't alter the initial value that has been forced
> > > +		 * by an override.
> > > +		 */
> > > +		if ((reg->override->mask & arm64_ftr_mask(ftrp)) == arm64_ftr_mask(ftrp))
> > > +			continue;
> > 
> > I got lost in the in the cpufeature framework, so I may be missing
> > something.
> > 
> > Let's say the primary CPU has a feature field with value 2 and we want
> > to override it to value 1. For e.g. a LOWER_SAFE feature, boot_cpu_data
> > will stored the overridden value of 1.
> > 
> > A secondary CPU comes online with the same feature missing, so value 0.
> > With the above change, we no longer update the system-wide feature
> > value, leave it as 1. Later on, for a system feature we may turn it on
> > even though the secondary CPU does not support it.
> > 
> > In summary, this makes the overridden field sticky for secondary CPUs
> > even if they don't support it.
> 
> That is true. I think we should let the secondary CPUs alter the values,
> with initial CPU feature value with the override value set, the system
> could then choose the safest among the override and the others.

It works for me. We should add a comment somewhere that the override is
not expected to work for features where we allow differences (some
FTR_NONSTRICT).

> > Unrelated to your patch, I think we can similarly fail to reject
> > secondary CPUs in check_early_cpu_features() -> verify_local_cpu_caps()
> > because of __read_sysreg_by_encoding() which uses the override value
> > unconditionally. From this perspective, we are now consistent with your
> > patch above.
> 
> This is true as well and the override takes the priority and with the
> wrong level of override value the system could be made to think that
> some features are available even when it is unsafe to do so.
> We should sanitise the values read by __read_sysreg_by_encoding() with
> the "overrides". I can cook something up.

Or remove this check if we expect the override to only work on the
resulting sanitised value, not individual checks.

-- 
Catalin