Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature

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

On Mon, Mar 03, 2025 at 10:17:28AM +0000, Ryan Roberts wrote:
> On 01/03/2025 01:32, Yang Shi wrote:
> > 
> > 
> > 
> > On 2/28/25 10:24 AM, Mikołaj Lenczewski wrote:
> >> For supporting BBM Level 2 for userspace mappings, we want to ensure
> >> that the smmu also supports its own version of BBM Level 2. Luckily, the
> >> smmu spec (IHI 0070G 3.21.1.3) is stricter than the aarch64 spec (DDI
> >> 0487K.a D8.16.2), so already guarantees that no aborts are raised when
> >> BBM level 2 is claimed.
> >>
> >> Add the feature and testing for it under arm_smmu_sva_supported().
> >>
> >> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> >> ---
> >>   arch/arm64/kernel/cpufeature.c                  | 7 +++----
> >>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++
> >>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 3 +++
> >>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 4 ++++
> >>   4 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >> index 63f6d356dc77..1022c63f81b2 100644
> >> --- a/arch/arm64/kernel/cpufeature.c
> >> +++ b/arch/arm64/kernel/cpufeature.c
> >> @@ -2223,8 +2223,6 @@ static bool has_bbml2_noabort(const struct
> >> arm64_cpu_capabilities *caps, int sco
> >>               if (!cpu_has_bbml2_noabort(__cpu_read_midr(cpu)))
> >>                   return false;
> >>           }
> >> -
> >> -        return true;
> >>       } else if (scope & SCOPE_LOCAL_CPU) {
> >>           /* We are a hot-plugged CPU, so only need to check our MIDR.
> >>            * If we have the correct MIDR, but the kernel booted on an
> >> @@ -2232,10 +2230,11 @@ static bool has_bbml2_noabort(const struct
> >> arm64_cpu_capabilities *caps, int sco
> >>            * we have an incorrect MIDR, but the kernel booted on a
> >>            * sufficient CPU, we will not bring up this CPU.
> >>            */
> >> -        return cpu_has_bbml2_noabort(read_cpuid_id());
> >> +        if (!cpu_has_bbml2_noabort(read_cpuid_id()))
> >> +            return false;
> >>       }
> >>   -    return false;
> >> +    return has_cpuid_feature(caps, scope);
> > 
> > Do we really need this? IIRC, it means the MIDR has to be in the allow list
> > *AND* MMFR2 register has to be set too. AmpereOne doesn't have MMFR2 register set.
> 
> Miko, I think this should have been squashed into patch #1? It doesn't belong in
> this patch.

Yes, 100%. Missed this, will put into patch #1.