On Mon, Dec 16, 2024 at 01:23:55PM +0000, Mark Brown wrote:
> On Mon, Dec 16, 2024 at 12:44:14PM +0000, Mark Rutland wrote:
>
> > ... didn't matter either way, and using '&boot_cpu_data' was intended to
> > make it clear that the features were based on the boot CPU's info, even
> > if you just grepped for that and didn't see the surrounding context.
>
> Right, that was my best guess as to what was supposed to be going on
> but it wasn't super clear. The code could use some more comments.
>
> > I think the real fix here is to move the reading back into
> > __cpuinfo_store_cpu(), but to have an explicit check that SME has been
> > disabled on the commandline, with a comment explaining that this is a
> > bodge for broken FW which traps the SME ID regs.
>
> That should be doable.
>
> There's a few other similar ID registers (eg, we already read GMID_EL1
> and MPAMIDR_EL1) make me a bit nervous that we might need to generalise
> it a bit, but we can deal with that if it comes up. Even for SME the
> disable was added speculatively, the factors that made this come up for
> SVE are less likely to be an issue with SME.
FWIW, I had a quick go (with only the SME case), and I think the shape
that we want is roughly as below, which I think is easy to generalise to
those other cases.
MarcZ, thoughts?
Mark.
---->8----
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 8b4e5a3cd24c8..f16eb99c10723 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -91,6 +91,16 @@ struct arm64_ftr_override {
u64 mask;
};
+static inline u64
+arm64_ftr_override_apply(const struct arm64_ftr_override *override,
+ u64 val)
+{
+ val &= ~override->mask;
+ val |= override->val & override->mask;
+
+ return val;
+}
+
/*
* @arm64_ftr_reg - Feature register
* @strict_mask Bits which should match across all CPUs for sanity.
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 6ce71f444ed84..faad7d3e4cf5f 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1167,12 +1167,6 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
id_aa64pfr1_sme(read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1))) {
unsigned long cpacr = cpacr_save_enable_kernel_sme();
- /*
- * We mask out SMPS since even if the hardware
- * supports priorities the kernel does not at present
- * and we block access to them.
- */
- info->reg_smidr = read_cpuid(SMIDR_EL1) & ~SMIDR_EL1_SMPS;
vec_init_vq_map(ARM64_VEC_SME);
cpacr_restore(cpacr);
@@ -1550,10 +1544,8 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
}
regp = get_arm64_ftr_reg(sys_id);
- if (regp) {
- val &= ~regp->override->mask;
- val |= (regp->override->val & regp->override->mask);
- }
+ if (regp)
+ val = arm64_ftr_override_apply(regp->override, val);
return val;
}
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index d79e88fccdfce..1460e3541132f 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -439,6 +439,24 @@ static void __cpuinfo_store_cpu_32bit(struct cpuinfo_32bit *info)
info->reg_mvfr2 = read_cpuid(MVFR2_EL1);
}
+static void __cpuinfo_store_cpu_sme(struct cpuinfo_arm64 *info)
+{
+ /*
+ * TODO: explain that this bodge is to avoid trapping.
+ */
+ u64 pfr1 = info->reg_id_aa64pfr1;
+ pfr1 = arm64_ftr_override_apply(&id_aa64pfr1_override, pfr1);
+ if (!id_aa64pfr1_sme(pfr1))
+ return;
+
+ /*
+ * We mask out SMPS since even if the hardware
+ * supports priorities the kernel does not at present
+ * and we block access to them.
+ */
+ info->reg_smidr = read_cpuid(SMIDR_EL1) & ~SMIDR_EL1_SMPS;
+}
+
static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
{
info->reg_cntfrq = arch_timer_get_cntfrq();
@@ -482,6 +500,8 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
if (id_aa64pfr0_mpam(info->reg_id_aa64pfr0))
info->reg_mpamidr = read_cpuid(MPAMIDR_EL1);
+ __cpuinfo_store_cpu_sme(info);
+
cpuinfo_detect_icache_policy(info);
}
