Re: [PATCH v3 09/21] arm64: cpufeature: Add global feature override facility

[Marc Zyngier]

Hi Catalin,

On 2021-01-11 18:41, Catalin Marinas wrote:
> Hi Marc,
>
> On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
> > Add a facility to globally override a feature, no matter what
> > the HW says. Yes, this is dangerous.
>
> Yeah, it's dangerous. We can make it less so if we only allow safe
> values (e.g. lower if FTR_UNSIGNED).

My plan was also to allow non-safe values in order to trigger features
that are not advertised by the HW. But I can understand if you are
reluctant to allow such thing! :D

> > diff --git a/arch/arm64/include/asm/cpufeature.h 
> > b/arch/arm64/include/asm/cpufeature.h
> > index 9a555809b89c..465d2cb63bfc 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
> >  	u64				sys_val;
> >  	u64				user_val;
> >  	const struct arm64_ftr_bits	*ftr_bits;
> > +	u64				*override_val;
> > +	u64				*override_mask;
> >  };
>
> At the arm64_ftr_reg level, we don't have any information about the 
> safe
> values for a feature. Could we instead move this to arm64_ftr_bits? We
> probably only need a single field. When populating the feature values,
> we can make sure it doesn't go above the hardware one.
>
> I attempted a feature modification for MTE here, though I dropped the
> entire series in the meantime as we clarified the ARM ARM:
>
> https://lore.kernel.org/linux-arm-kernel/20200515171612.1020-24-catalin.marinas@xxxxxxx/
>
> Srinivas copied it in his patch (but forgot to give credit ;)):
>
> https://lore.kernel.org/linux-arm-msm/1610152163-16554-3-git-send-email-sramana@xxxxxxxxxxxxxx/
>
> The above adds a filter function but, instead, just use your mechanism 
> in
> this series for idreg.feature setting via cmdline. The 
> arm64_ftr_value()
> function extracts the hardware value and lowers it if a cmdline 
> argument
> was passed.

One thing is that it is not always possible to sanitise the value passed
if it is required very early on, as I do with VHE. But in that case
I actually check that we are VHE capable before starting to poke at
VHE-specific state.

I came up with the following patch on top, which preserves the current
global approach (no per arm64_ftr_bits state), but checks (and alters)
the override as it iterates through the various fields.

For example, if I pass "arm64.nopauth kvm-arm.mode=nvhe 
id_aa64pfr1.bt=5"
to the FVP, I get the following output:

[    0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[31:28]: forced from 1 
to 0
[    0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[11:8]: forced from 1 
to 0
[    0.000000] CPU features: SYS_ID_AA64MMFR1_EL1[11:8]: forced from 1 
to 0
[    0.000000] CPU features: SYS_ID_AA64PFR1_EL1[3:0]: not forcing 1 to 
5
[    0.000000] CPU features: detected: GIC system register CPU interface
[    0.000000] CPU features: detected: Hardware dirty bit management
[    0.000000] CPU features: detected: Spectre-v4
[    0.000000] CPU features: detected: Branch Target Identification

showing that the PAC features have been downgraded, together with VHE,
but that BTI is still detected as value 5 was obviously bogus.

Thoughts?

        M.

diff --git a/arch/arm64/kernel/cpufeature.c 
b/arch/arm64/kernel/cpufeature.c
index 894af60b9669..00d99e593b65 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 
new)
 	u64 strict_mask = ~0x0ULL;
 	u64 user_mask = 0;
 	u64 valid_mask = 0;
+	u64 override_val = 0, override_mask = 0;

 	const struct arm64_ftr_bits *ftrp;
 	struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
@@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, 
u64 new)
 	if (!reg)
 		return;

+	if (reg->override_mask && reg->override_val) {
+		override_mask = *reg->override_mask;
+		override_val = *reg->override_val;
+	}
+
 	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
 		u64 ftr_mask = arm64_ftr_mask(ftrp);
 		s64 ftr_new = arm64_ftr_value(ftrp, new);
+		s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
+
+		if ((ftr_mask & override_mask) == ftr_mask) {
+			if (ftr_ovr < ftr_new) {
+				pr_warn("%s[%d:%d]: forced from %llx to %llx\n",
+					reg->name,
+					ftrp->shift + ftrp->width - 1,
+					ftrp->shift, ftr_new, ftr_ovr);
+
+				ftr_new = ftr_ovr;
+			} else if (ftr_ovr != ftr_new) {
+				pr_warn("%s[%d:%d]: not forcing %llx to %llx\n",
+					reg->name,
+					ftrp->shift + ftrp->width - 1,
+					ftrp->shift, ftr_new, ftr_ovr);
+
+				/* Remove the override */
+				*reg->override_mask &= ~ftr_mask;
+				*reg->override_val &= ~ftr_mask;
+			}
+		}

 		val = arm64_ftr_set_value(ftrp, val, ftr_new);

@@ -800,18 +827,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, 
u64 new)

 	val &= valid_mask;

-	if (reg->override_mask && reg->override_val) {
-		u64 override = val;
-		override &= ~*reg->override_mask;
-		override |= (*reg->override_val & *reg->override_mask);
-
-		if (val != override)
-			pr_warn("%s: forced from %016llx to %016llx\n",
-				reg->name, val, override);
-
-		val = override;
-	}
-
 	reg->sys_val = val;
 	reg->strict_mask = strict_mask;
 	reg->user_mask = user_mask;

--
Jazz is not dead. It just smells funny...