Message-ID: <0ac0f1f5-e4a0-46ae-8ea0-2eba7e21a7e1@arm.com>
Date: Tue, 11 Mar 2025 12:16:14 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Ryan Roberts <ryan.roberts@....com>,
 Yang Shi <yang@...amperecomputing.com>,
 Mikołaj Lenczewski <miko.lenczewski@....com>,
 catalin.marinas@....com, will@...nel.org, joro@...tes.org,
 jean-philippe@...aro.org, mark.rutland@....com, joey.gouly@....com,
 oliver.upton@...ux.dev, james.morse@....com, broonie@...nel.org,
 maz@...nel.org, david@...hat.com, akpm@...ux-foundation.org, jgg@...pe.ca,
 nicolinc@...dia.com, mshavit@...gle.com, jsnitsel@...hat.com,
 smostafa@...gle.com, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Subject: Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature

On 11/03/2025 10:58, Ryan Roberts wrote:
> On 11/03/2025 10:17, Suzuki K Poulose wrote:
>> On 03/03/2025 10:17, 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@....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.
>>>
>>> Yang, we discussed this internally and decided that we thought it was best to
>>> still require BBML2 being advertised in the feature register. That way if trying
>>> to use KVM to emulate a CPU that is in the allow list but doesn't really support
>>> BBML2, we won't try to use it.
>>>
>>> But we still end up with the same problem if running on a physical CPU that
>>> supports BBML2 with conflict aborts, but emulating a CPU in the allow list. So
>>
>> I don't understand the problem here ? In the worst case, if we want to disable
>> the BBML2 feature on a given CPU, we could provide an id-
>> override to reset the value of BBML2. Or provide a kernel parameter to
>> disable this in case we want to absolutely disable the feature on a
>> "distro" kernel.
> 
> Hi Suzuki,
> 
> Sorry perhaps I'm confusing everyone; As I recall, we had a conversation before
> Miko posted this series where you were suggesting we should check BOTH that all
> the CPUs' MIDRs are in the allow list AND that BBML2 is advertised in MMFR2 in
> order to decide to enable the CPU feature. My understanding was that without the
> MMFR2 check, you were concerned that in a virtualization scenario, a CPU's MIDR
> could be overridden to emulate a CPU that is in the allow list, but in reality
> the CPU does not support BBML2. We would then enable BBML2 and BadThings (TM)
> will happen. So additionally checking the MMFR2 would solve this.
> 
> But Yang is saying that he plans to add the AmpereOne to the allow list because
> it does support BBML2+NOCONFLICT semantics and we want to benefit from that. But
> AmpereOne does not advertise BBML2 in it's MMFR2. So with the current approach,
> adding AmpereOne to the allow list is not sufficient to enable the feature.
> 
> But back to your original justification for checking the MMFR2; I don't think
> that really solves the problem in general, because we don't just require BBML2,
> we require BBML2+NOCONFLICT. And we can only determine that from the MIDR. So
> why bother checking MMFR2?

My concerns are not around enabling a CPU, but having a damage control 
with a "kernel" that a user has no control over (Read, standard 
distribution kernel).

1. If the combination of the above causes problem (in Virtualization)
2. If the combination of the above is detected to have problems in 
baremetal.

In (1), VMM could control the ID register and disable the feature.

For (2) we could provide an id-override on command line to disable it in
the worst case.

So, having the id register case is a good way to get the system running
with a given kernel (in either world). Without that, we don't have a 
tunable to control the behavior at runtime.

May be I am being over paranoid about this.

> 
> I guess we could provide an id-override on the kernel command line to *enable*
> BBML2 for AmpereOne, but that's not going to be suitable for mass deployment, I

Unfortunately, we can't override to provide the "feature" that is 
missing (at least today).

One option is, run with a whitelist, but provide a kernel parameter to 
disable bbml2 feature.

Something like, arm64.bbml2=0

So the decision is based on the parameter && MIDR list.


Cheers
Suzuki

> don't think?
> 
> Thanks,
> Ryan
> 
>>
>> Suzuki
>>
>>
>>> given AmpereOne doesn't advertise BBML2 but does support it, I'd be happy to
>>> remove this check.
>>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>>>
>>>> Thanks,
>>>> Yang
>>>>
>>>>>     }
>>>>>       #ifdef CONFIG_ARM64_PAN
>>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/
>>>>> arm/arm-smmu-v3/arm-smmu-v3-sva.c
>>>>> index 9ba596430e7c..6ba182572788 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>>>>> @@ -222,6 +222,9 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
>>>>>             feat_mask |= ARM_SMMU_FEAT_VAX;
>>>>>         }
>>>>>     +    if (system_supports_bbml2_noabort())
>>>>> +        feat_mask |= ARM_SMMU_FEAT_BBML2;
>>>>> +
>>>>>         if ((smmu->features & feat_mask) != feat_mask)
>>>>>             return false;
>>>>>     diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/
>>>>> arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> index 358072b4e293..dcee0bdec924 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> @@ -4406,6 +4406,9 @@ static int arm_smmu_device_hw_probe(struct
>>>>> arm_smmu_device *smmu)
>>>>>         if (FIELD_GET(IDR3_RIL, reg))
>>>>>             smmu->features |= ARM_SMMU_FEAT_RANGE_INV;
>>>>>     +    if (FIELD_GET(IDR3_BBML, reg) == IDR3_BBML2)
>>>>> +        smmu->features |= ARM_SMMU_FEAT_BBML2;
>>>>> +
>>>>>         /* IDR5 */
>>>>>         reg = readl_relaxed(smmu->base + ARM_SMMU_IDR5);
>>>>>     diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/
>>>>> arm/arm-smmu-v3/arm-smmu-v3.h
>>>>> index bd9d7c85576a..85eaf3ab88c2 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>>>> @@ -60,6 +60,9 @@ struct arm_smmu_device;
>>>>>     #define ARM_SMMU_IDR3            0xc
>>>>>     #define IDR3_FWB            (1 << 8)
>>>>>     #define IDR3_RIL            (1 << 10)
>>>>> +#define IDR3_BBML            GENMASK(12, 11)
>>>>> +#define IDR3_BBML1            (1 << 11)
>>>>> +#define IDR3_BBML2            (2 << 11)
>>>>>       #define ARM_SMMU_IDR5            0x14
>>>>>     #define IDR5_STALL_MAX            GENMASK(31, 16)
>>>>> @@ -754,6 +757,7 @@ struct arm_smmu_device {
>>>>>     #define ARM_SMMU_FEAT_HA        (1 << 21)
>>>>>     #define ARM_SMMU_FEAT_HD        (1 << 22)
>>>>>     #define ARM_SMMU_FEAT_S2FWB        (1 << 23)
>>>>> +#define ARM_SMMU_FEAT_BBML2        (1 << 24)
>>>>>         u32                features;
>>>>>       #define ARM_SMMU_OPT_SKIP_PREFETCH    (1 << 0)
>>>>
>>>
>>
> 

Powered by blists - more mailing lists