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

["Mikołaj Lenczewski" <miko.lenczewski@arm.com>]

On Wed, Feb 19, 2025 at 05:25:00PM -0800, Yang Shi wrote: 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index d561cf3b8ac7..8c337bd95ef7 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -2176,6 +2176,31 @@ static bool hvhe_possible(const struct arm64_cpu_capabilities *entry,
> >   	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
> >   }
> >   
> > +static bool has_bbml2_noconflict(const struct arm64_cpu_capabilities *entry,
> > +				 int scope)
> > +{
> > +	if (!IS_ENABLED(CONFIG_ARM64_ENABLE_BBML2))
> > +		return false;
> > +
> > +	/* We want to allow usage of bbml2 in as wide a range of kernel contexts
> > +	 * as possible. This list is therefore an allow-list of known-good
> > +	 * implementations that both support bbml2 and additionally, fulfil the
> > +	 * extra constraint of never generating TLB conflict aborts when using
> > +	 * the relaxed bbml2 semantics (such aborts make use of bbml2 in certain
> > +	 * kernel contexts difficult to prove safe against recursive aborts).
> > +	 */
> > +	static const struct midr_range supports_bbml2_without_abort_list[] = {
> > +		MIDR_REV_RANGE(MIDR_CORTEX_X4, 0, 3, 0xf),
> > +		MIDR_REV_RANGE(MIDR_NEOVERSE_V3, 0, 2, 0xf),
> > +		{}
> > +	};
> > +
> > +	if (!is_midr_in_range_list(read_cpuid_id(), supports_bbml2_without_abort_list))
> > +		return false;
> > +
> > +	return true;
> > +}
> Hi Miko,
> 
> Thanks for cc'ing me this series. I and Ryan discussed about how to 
> advertise BBML2 properly in my thread 
> (https://lore.kernel.org/linux-arm-kernel/4c44cf6e-98de-47bb-b430-2b1331114904@os.amperecomputing.com/). 
> IIUC, this may not work as expected.
> 
> The boot cpu initializes the boot_cpu_data, then the secondary cpus need 
> to update it, the "sanitized" register value will be generated. For 
> example, TLB range capability is determined by ISAR0_EL1. If all the 
> cpus have this feature, the "sanitized" register value will show true 
> otherwise it will show false.
> 
> BBML2 can be determined by MMFR2_EL1. If we can rely on it then system 
> feature does work. But the problem is some implementations may have 
> MMFR2_EL1 set, but they may not be able to handle TLB conflict. We can't 
> rely on it solely so we check MIDR in .matches callback instead of 
> MMFR2_EL1. But system feature .matches callback is just called once on 
> boot CPU because it is supposed to read the sanitized register value. So 
> you actually just checked the MIDR on boot CPU in .matches callback if I 
> read the code correctly.
> 
> I'm not quite familiar with cpufeature details, if I'm wrong please feel 
> free to correct me.
> 
> Yang

Hi Yang,

Thank you for taking the time to review this patch series.

Thank you also for the spot. I am very much not an expert on
cpufeatures, but I think you are correct. IIUC, currently the .matches
check would go through as long as the the boot CPU executing the
.matches function has the correct MIDR value, and as long as
CONFIG_ARM64_ENABLE_BBML2 is set.

If, as you point out, another CPU has a MIDR that is not on this list
and which was not checked (because .matches only executes on a single
boot CPU), then .matches should still go through (and we could run into
problems when said other CPU executes any BBML2 aware code).

Please let me know if I have understood what you are saying correctly.

From re-reading `include/asm/cpufeature.h`, making it a SCOPE_LOCAL_CPU
feature seems to be close to what we want. We want to have each CPU test
its cpuid against the allowed MIDR list, and the feature overall to
only be considered present if *all* boot cpus returned true (not as
SCOPE_LOCAL_CPU puts it "... detect if at least one matches.").

I will see if this can be hacked around with the current system, and if
not, we might have to extend the current cpucaps scopes / machinery like
you suggest in your patch series comments.

-- 
Kind regards,
Mikołaj Lenczewski