Re: [PATCH v3 1/3] arm64: Add BBM Level 2 cpu feature

[Marc Zyngier <maz@kernel.org>]

On Thu, 13 Mar 2025 18:20:26 +0000,
Mikołaj Lenczewski <miko.lenczewski@arm.com> wrote:
> 
> On Thu, Mar 13, 2025 at 05:34:46PM +0000, Marc Zyngier wrote:
> > On Thu, 13 Mar 2025 10:41:10 +0000,
> > Mikołaj Lenczewski <miko.lenczewski@arm.com> 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:
> > 
> > diff --git a/arch/arm64/kernel/pi/idreg-override.c b/arch/arm64/kernel/pi/idreg-override.c
> > index c6b185b885f70..803a0c99f7b46 100644
> > --- a/arch/arm64/kernel/pi/idreg-override.c
> > +++ b/arch/arm64/kernel/pi/idreg-override.c
> > @@ -102,6 +102,7 @@ static const struct ftr_set_desc mmfr2 __prel64_initconst = {
> >  	.override	= &id_aa64mmfr2_override,
> >  	.fields		= {
> >  		FIELD("varange", ID_AA64MMFR2_EL1_VARange_SHIFT, mmfr2_varange_filter),
> > +		FIELD("bbm", ID_AA64MMFR2_EL1_BBM_SHIFT, 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",		"id_aa64mmfr2.bbm=0" },
> >  };
> >  
> >  static int __init parse_hexdigit(const char *p, u64 *v)
> > 
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.
> 
> Thanks for the review.
> 
> I think part of this confusion is due to me not including a changelog
> (definitely something for the next respin!), but the discussion this
> change is based on is found here:
> 
> https://lore.kernel.org/all/b46dc626-edc9-4d20-99d2-6cd08a01346c@os.amperecomputing.com/
> 
> Essentially, this is a SW feature because we do not check the
> id_aa64mmfr2.bbm register as part of the has_bbml2_noabort() cpucap
> matches filter. This is because certain hardware implementations
> do not actually declare bbml2 via the hardware feature register, despite
> implementing our bbml2_noabort feature, and certain hypervisor setups
> might result in issues so we want to have an override to allow
> potentially disabling the feature for generic kernels.

I replied to Ryan on the same subject: not advertising a feature that
is actually supported is very much an erratum, and we should not
conflate feature control of an architecture feature (which is what the
ID override horror is doing) with implementation-specific workarounds.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.