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

[Suzuki K Poulose]

On 20/03/2025 17:09, Mikołaj Lenczewski wrote:
> On Thu, Mar 20, 2025 at 01:24:25PM +0000, Suzuki K Poulose wrote:
> > On 19/03/2025 15:05, Mikołaj Lenczewski wrote:
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index fb8752b42ec8..3e4cc917a07e 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -453,6 +453,9 @@
> > >    	arm64.no32bit_el0 [ARM64] Unconditionally disable the execution of
> > >    			32 bit applications.
> > > +	arm64.nobbml2	[ARM64] Unconditionally disable Break-Before-Make Level
> > > +			2 support
> > > +
> > >    	arm64.nobti	[ARM64] Unconditionally disable Branch Target
> > >    			Identification support
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index 940343beb3d4..49deda2b22ae 100644
> > > --- a/arch/arm64/Kconfig
> > > +++ b/arch/arm64/Kconfig
> > > @@ -2057,6 +2057,17 @@ config ARM64_TLB_RANGE
> > >    	  The feature introduces new assembly instructions, and they were
> > >    	  support when binutils >= 2.30.
> > > +config ARM64_BBML2_NOABORT
> > > +	bool "Enable support for Break-Before-Make Level 2 detection and usage"
> > > +	default y
> > > +	help
> > > +	  FEAT_BBM provides detection of support levels for break-before-make
> > > +	  sequences. If BBM level 2 is supported, some TLB maintenance requirements
> > > +	  can be relaxed to improve performance. We additonally require the
> > > +	  property that the implementation cannot ever raise TLB Conflict Aborts.
> > > +	  Selecting N causes the kernel to fallback to BBM level 0 behaviour
> > > +	  even if the system supports BBM level 2.
> >
> > minor nit: Should we mention that the feature can be disabled at runtime
> > using a kernel parameter ?
>
> Yes, this sounds very reasonable, I should have thought of that. Will
> mention the commandline parameter in the kconfig option documentation.

And also may be mention this in the patch description and may be also
add the rationale for providing this tunable.

Cheers
Suzuki


> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index d561cf3b8ac7..1a4adcda267b 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -2176,6 +2176,67 @@ static bool hvhe_possible(const struct arm64_cpu_capabilities *entry,
> > >    	return arm64_test_sw_feature_override(ARM64_SW_FEATURE_OVERRIDE_HVHE);
> > >    }
> > > +static bool cpu_has_bbml2_noabort(unsigned int cpu_midr)
> > > +{
> > > +	/* 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, fulfill 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).
> > > +	 *
> > > +	 * Note that implementations can only be considered "known-good" if their
> > > +	 * implementors attest to the fact that the implementation never raises
> > > +	 * TLBI conflict aborts for bbml2 mapping granularity changes.
> > > +	 */
> > > +	static const struct midr_range supports_bbml2_noabort_list[] = {
> > > +		MIDR_REV_RANGE(MIDR_CORTEX_X4, 0, 3, 0xf),
> > > +		MIDR_REV_RANGE(MIDR_NEOVERSE_V3, 0, 2, 0xf),
> > > +		{}
> > > +	};
> > > +
> > > +	return is_midr_in_range_list(cpu_midr, supports_bbml2_noabort_list);
> > > +}
> > > +
> > > +static inline unsigned int cpu_read_midr(int cpu)
> > > +{
> > > +	WARN_ON_ONCE(!cpu_online(cpu));
> > > +
> > > +	return per_cpu(cpu_data, cpu).reg_midr;
> > > +}
> > > +
> > > +static bool has_bbml2_noabort(const struct arm64_cpu_capabilities *caps, int scope)
> > > +{
> > > +	if (!IS_ENABLED(CONFIG_ARM64_BBML2_NOABORT))
> > > +		return false;
> > > +
> > > +	if (scope & SCOPE_SYSTEM) {
> > > +		int cpu;
> > > +
> > > +		/* We are a boot CPU, and must verify that all enumerated boot
> >
> > minor nit: See Documentation/process/coding-style.rst,
> > Section 8 Commenting.
>
> My bad, had skimmed the coding style at one point but clearly missed or
> forgot the section on comments. Will fix up this and all other instances
> of the same issue.