On 14/03/2025 09:18, Ryan Roberts wrote:
On 13/03/2025 18:36, Marc Zyngier wrote:
On Thu, 13 Mar 2025 18:22:00 +0000,
Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
On 13/03/2025 17:34, Marc Zyngier wrote:
On Thu, 13 Mar 2025 10:41:10 +0000,
Mikołaj Lenczewski <miko.lenczewski@xxxxxxx> wrote:
diff --git a/arch/arm64/kernel/pi/idreg-override.c b/arch/arm64/kernel/pi/idreg-override.c
index c6b185b885f7..9728faa10390 100644
--- a/arch/arm64/kernel/pi/idreg-override.c
+++ b/arch/arm64/kernel/pi/idreg-override.c
@@ -209,6 +209,7 @@ static const struct ftr_set_desc sw_features __prel64_initconst = {
FIELD("nokaslr", ARM64_SW_FEATURE_OVERRIDE_NOKASLR, NULL),
FIELD("hvhe", ARM64_SW_FEATURE_OVERRIDE_HVHE, hvhe_filter),
FIELD("rodataoff", ARM64_SW_FEATURE_OVERRIDE_RODATA_OFF, NULL),
+ FIELD("nobbml2", ARM64_SW_FEATURE_OVERRIDE_NOBBML2, NULL),
{}
},
};
@@ -246,6 +247,7 @@ static const struct {
{ "rodata=off", "arm64_sw.rodataoff=1" },
{ "arm64.nolva", "id_aa64mmfr2.varange=0" },
{ "arm64.no32bit_el0", "id_aa64pfr0.el0=1" },
+ { "arm64.nobbml2", "arm64_sw.nobbml2=1" },
Why is that a SW feature? This looks very much like a HW feature to
me, and you should instead mask out ID_AA64MMFR2_EL1.BBM, and be done
with it. Something like:
I think this implies that we would expect the BBM field to be advertising BBML2
support normally and we would check for that as part of the cpufeature
detection. That's how Miko was doing it in v2, but Yang pointed out that
AmpereOne, which supports BBML2+NOABORT semantics, doesn't actually advertise
BBML2 in its MMFR2. So we don't want to check that field, and instead rely
solely on the MIDR allow-list + a command line override. It was me that
suggested putting that in the SW feature register, and I think that still sounds
like the right solution for this situation?
I think this is mixing two different things:
- preventing BBM-L2 from being visible to the kernel: this is what my
suggestion is doing by nuking an architectural feature in the
relevant register
- random HW not correctly advertising what they are doing: this is an
erratum workaround
I'd rather we don't conflate the two things, and make them very
explicitly distinct.
It all sounds so obvious when you put it like that! :)
I'm guessing there is a layer where the workaround can be applied to the
sanitised feature registers on a per-cpu basis and that won't affect this global
override which will remain as an overlay on top? If so then that sounds perfect
(you can probably tell I find the whole feature management framework rather
inpeneterable). That workaround would be added as part of Yang's series anyway.
Unfortunately, there is no easy way to fix this via the normal erratum
workaround "capability". The sanitised feature registers are handled
separately (initialised via init_cpu_features() for boot CPU and
sanitised eachtime via update_cpu_features).
Also we do not "enable" any capability (i.e. calling cpu_enable())
until the very end, after the CPUs are all brought up (except for boot
CPUs).
But it may be possible to "fix up" the BBML2 feature in
cpuinfo_store_*cpu(), without using the "enable" call back.
Something like:
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 285d7d538342..8c23adbe29f8 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -479,7 +479,11 @@ static void __cpuinfo_store_cpu(struct
cpuinfo_arm64 *info)
info->reg_id_aa64mmfr0 = read_cpuid(ID_AA64MMFR0_EL1);
info->reg_id_aa64mmfr1 = read_cpuid(ID_AA64MMFR1_EL1);
info->reg_id_aa64mmfr2 = read_cpuid(ID_AA64MMFR2_EL1);
+ /*
+ * The CPU cap is not detected system wide, but we are able to
+ * check if this CPU is affected by the Erratum.
+ */
+ if (this_cpu_has_cap(AMPERE_ONE_ERRATUM_BBML2))
+ // Fixup info->reg_id_aa64_mmfr2 with BBML2.
+
info->reg_id_aa64mmfr3 = read_cpuid(ID_AA64MMFR3_EL1);
info->reg_id_aa64mmfr4 = read_cpuid(ID_AA64MMFR4_EL1);
info->reg_id_aa64pfr0 = read_cpuid(ID_AA64PFR0_EL1);
info->reg_id_aa64pfr1 = read_cpuid(ID_AA64PFR1_EL1);
Suzuki
So sounds like we are back to testing MMFR2.BBM in the matches function, with
the addition of Maz's proposal above. Sorry for sending you round the houses, Miko.
Thanks,
Ryan
Thanks,
M.
