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...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
