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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 25/03/2026 17:51, Catalin Marinas wrote:
> 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.

True, but if some capabilities are PERCPU local features, then there is
no way to override them with the controls. I have the following patch,
that could do the trick :

--8>--

arm64: Apply overrides to CPU local capabilities

If an override has been applied, make sure we apply that for the
secondary CPUs too, to limit the features.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
  arch/arm64/kernel/cpufeature.c | 40 +++++++++++++++++++++++++++-------
  1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2e1e4de9a2cd..2b494302b767 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1217,10 +1217,41 @@ void __init init_cpu_features(struct 
cpuinfo_arm64 *info)
  		init_cpu_ftr_reg(SYS_GMID_EL1, info->reg_gmid);
  }

+/*
+ * Sanitise the register fields to clamp the values to the overrides that
+ * has been applied.
+ */
+static u64 override_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 val)
+{
+	const struct arm64_ftr_bits *ftrp;
+
+	if (!reg || !reg->override->mask)
+		return val;
+
+	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
+		u64 ftr_mask = arm64_ftr_mask(ftrp);
+		s64 ftr_val, ftr_ovr, ftr_safe;
+
+		/* Skip the fields not overridden */
+		if ((ftr_mask & reg->override->mask) != ftr_mask)
+			continue;
+
+		ftr_val = arm64_ftr_value(ftrp, val);
+		ftr_ovr = arm64_ftr_value(ftrp, reg->override->val);
+		ftr_safe = arm64_ftr_safe_value(ftrp, ftr_ovr, ftr_val);
+
+		if (ftr_safe != ftr_val)
+			val = arm64_ftr_set_value(ftrp, val, ftr_safe);
+	}
+	return val;
+}
+
  static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
  {
  	const struct arm64_ftr_bits *ftrp;

+	/* Apply the overrides */
+	new = override_cpu_ftr_reg(reg, new);
  	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
  		s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
  		s64 ftr_new = arm64_ftr_value(ftrp, new);
@@ -1524,7 +1555,6 @@ EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg);
   */
  u64 __read_sysreg_by_encoding(u32 sys_id)
  {
-	struct arm64_ftr_reg *regp;
  	u64 val;

  	switch (sys_id) {
@@ -1577,13 +1607,7 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
  		return 0;
  	}

-	regp  = get_arm64_ftr_reg(sys_id);
-	if (regp) {
-		val &= ~regp->override->mask;
-		val |= (regp->override->val & regp->override->mask);
-	}
-
-	return val;
+	return override_cpu_ftr_reg(get_arm64_ftr_reg(sys_id), val);
  }

  #include <linux/irqchip/arm-gic-v3.h>
-- 
2.43.0



>