Re: [PATCH v2 1/4] arm64: Add BBM Level 2 cpu feature

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

On Fri, Feb 28, 2025 at 06:45:38PM -0800, Yang Shi wrote:
> 
> 
> 
> On 2/28/25 5:29 PM, Yang Shi wrote:
> > 
> > 
> > 
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index 940343beb3d4..baae6d458996 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_ENABLE_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.
> > > +
> > >   endmenu # "ARMv8.4 architectural features"
> > >     menu "ARMv8.5 architectural features"
> > > diff --git a/arch/arm64/include/asm/cpucaps.h
> > > b/arch/arm64/include/asm/cpucaps.h
> > > index 0b5ca6e0eb09..2d6db33d4e45 100644
> > > --- a/arch/arm64/include/asm/cpucaps.h
> > > +++ b/arch/arm64/include/asm/cpucaps.h
> > > @@ -23,6 +23,8 @@ cpucap_is_possible(const unsigned int cap)
> > >           return IS_ENABLED(CONFIG_ARM64_PAN);
> > >       case ARM64_HAS_EPAN:
> > >           return IS_ENABLED(CONFIG_ARM64_EPAN);
> > > +    case ARM64_HAS_BBML2_NOABORT:
> > > +        return IS_ENABLED(CONFIG_ARM64_BBML2_NOABORT);
> > >       case ARM64_SVE:
> > >           return IS_ENABLED(CONFIG_ARM64_SVE);
> > >       case ARM64_SME:
> > > diff --git a/arch/arm64/include/asm/cpufeature.h
> > > b/arch/arm64/include/asm/cpufeature.h
> > > index e0e4478f5fb5..108ef3fbbc00 100644
> > > --- a/arch/arm64/include/asm/cpufeature.h
> > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > @@ -866,6 +866,11 @@ static __always_inline bool
> > > system_supports_mpam_hcr(void)
> > >       return alternative_has_cap_unlikely(ARM64_MPAM_HCR);
> > >   }
> > >   +static inline bool system_supports_bbml2_noabort(void)
> > > +{
> > > +    return alternative_has_cap_unlikely(ARM64_HAS_BBML2_NOABORT);
> > > +}
> > 
> > Hi Miko,
> > 
> > I added AmpereOne mdir on top of this patch. I can see BBML2 feature is
> > detected via dmesg. But system_supports_bbml2_noabort() returns false.
> > The warning in the below debug patch is triggered:
> > 
> > diff --git a/arch/arm64/kernel/cpufeature.c
> > b/arch/arm64/kernel/cpufeature.c
> > index faa9094d97dd..a70829ae2bd0 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -3814,6 +3814,9 @@ void __init setup_system_features(void)
> >  {
> >         setup_system_capabilities();
> > 
> > +       if (!system_supports_bbml2_noabort())
> > +               WARN_ON_ONCE(1);
> > +
> >         kpti_install_ng_mappings();
> > 
> >         sve_setup();
> > 
> > I thought it may be too early. But it seems other system features work
> > well, for example, MPAM. I didn't figure out why. It is weird.
> 
> I just figured out the problem It is because the wrong kconfig name is used
> in cpucaps.h. The code is:
> 
> +    case ARM64_HAS_BBML2_NOABORT:
> +        return IS_ENABLED(CONFIG_ARM64_BBML2_NOABORT);
> 
> But the kconfig name actually is:
> 
> +config ARM64_ENABLE_BBML2_NOABORT
> 
> IMHO, the "ENABLE" in kconfig name sounds unnecessary.
> 
> Thanks,
> Yang
> 
> 

Hi Yang,

Thank you for the review, and apologies for the slight delay.

Thanks again for the spot, I agree that `ENABLE` is probably redundant
(and clearly, caused an issue here). Will remove this. Please let me
know if there are any other issues with rebasing your patches on top of
mine.

-- 
Kind regards,
Mikołaj Lenczewski