1. Patchew
  2. linux
  3. [PATCH v2 0/4] Initial BBML2 support for contpte_convert()
  4. View patch

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

Mikołaj Lenczewski posted 4 patches 11 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Mikołaj Lenczewski 11 months, 2 weeks ago 
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);
 }
 
 #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)
-- 
2.45.3
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Yang Shi 11 months, 2 weeks ago 


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.

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)
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Ryan Roberts 11 months, 1 week ago 
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.

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
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)
>
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Suzuki K Poulose 11 months ago 
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@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.
> 
> 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.

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)
>>
>
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Ryan Roberts 11 months ago 
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@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.
>>
>> 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?

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
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)
>>>
>>
>
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Suzuki K Poulose 11 months ago 
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@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.
>>>
>>> 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)
>>>>
>>>
>>
>
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Ryan Roberts 11 months ago 
On 11/03/2025 12:16, Suzuki K Poulose wrote:
> 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@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.
>>>>
>>>> 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).

Ahh I misunderstood then.

> 
> 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.

This sounds like a good solution to me. I guess we would wire this up to the SW
features "register".

Thanks,
Ryan

> 
> 
> 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)
>>>>>
>>>>
>>>
>>
>
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Yang Shi 11 months, 1 week ago 

On 3/3/25 2:17 AM, 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.
>
> 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
> given AmpereOne doesn't advertise BBML2 but does support it, I'd be happy to
> remove this check.

Thank you.

Yang

>
> 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)
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Mikołaj Lenczewski 11 months, 1 week ago 
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.
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Jason Gunthorpe 11 months, 2 weeks ago 
On Fri, Feb 28, 2025 at 06:24:04PM +0000, 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(-)

This patch looks good, for what it does. However for bisection safety
it should be earlier, before the patches that change the page table
algorithms to be unsafe for the SMMU.

However, I've heard people talking about shipping chips that have CPUs
with BBML2 but SMMUs without.

On such a system it seems like your series would break previously
working SVA support because this patch will end up disabling it?

Though I see your MIDR_REV list is limited, so perhaps that worry
doesn't effect any real chips made with those families? I am trying to
check some NVIDIA products against this list..

Jason
RE: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Shameerali Kolothum Thodi 11 months, 1 week ago 

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, February 28, 2025 7:32 PM
> To: Mikołaj Lenczewski <miko.lenczewski@arm.com>; Shameerali Kolothum
> Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: ryan.roberts@arm.com; suzuki.poulose@arm.com;
> yang@os.amperecomputing.com; catalin.marinas@arm.com;
> will@kernel.org; joro@8bytes.org; jean-philippe@linaro.org;
> mark.rutland@arm.com; joey.gouly@arm.com; oliver.upton@linux.dev;
> james.morse@arm.com; broonie@kernel.org; maz@kernel.org;
> david@redhat.com; akpm@linux-foundation.org; nicolinc@nvidia.com;
> mshavit@google.com; jsnitsel@redhat.com; smostafa@google.com; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> iommu@lists.linux.dev
> Subject: Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
> 
> On Fri, Feb 28, 2025 at 06:24:04PM +0000, 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(-)
> 
> This patch looks good, for what it does. However for bisection safety
> it should be earlier, before the patches that change the page table
> algorithms to be unsafe for the SMMU.
> 
> However, I've heard people talking about shipping chips that have CPUs
> with BBML2 but SMMUs without.
> 
> On such a system it seems like your series would break previously
> working SVA support because this patch will end up disabling it?
> 
> Though I see your MIDR_REV list is limited, so perhaps that worry
> doesn't effect any real chips made with those families? I am trying to
> check some NVIDIA products against this list..

We do have implementations that support CPUs with BBLM2 with TLB
conflict aborts and SMMUv3 with BBML2.  So don't think those platforms
be affected by this.  Will check with our hardware folks if there is
anything that will be affected by this.

Also we  have plans to try to use SMMUv3 BBML2 during VM live migration
to split block pages to 4K. I guess, in that case we can enable SMMU BBML2
independent of CPU side. 

Thanks,
Shameer
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Mikołaj Lenczewski 11 months, 1 week ago 
Hi Both,

Thanks for review and for checking other implementations for this
discrepancy.

On Mon, Mar 03, 2025 at 08:49:02AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Friday, February 28, 2025 7:32 PM
> > To: Mikołaj Lenczewski <miko.lenczewski@arm.com>; Shameerali Kolothum
> > Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: ryan.roberts@arm.com; suzuki.poulose@arm.com;
> > yang@os.amperecomputing.com; catalin.marinas@arm.com;
> > will@kernel.org; joro@8bytes.org; jean-philippe@linaro.org;
> > mark.rutland@arm.com; joey.gouly@arm.com; oliver.upton@linux.dev;
> > james.morse@arm.com; broonie@kernel.org; maz@kernel.org;
> > david@redhat.com; akpm@linux-foundation.org; nicolinc@nvidia.com;
> > mshavit@google.com; jsnitsel@redhat.com; smostafa@google.com; linux-
> > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > iommu@lists.linux.dev
> > Subject: Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
> > 
> > On Fri, Feb 28, 2025 at 06:24:04PM +0000, 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(-)
> > 
> > This patch looks good, for what it does. However for bisection safety
> > it should be earlier, before the patches that change the page table
> > algorithms to be unsafe for the SMMU.

Right, I should have noticed this earlier. Will reorder the patch.

> > However, I've heard people talking about shipping chips that have CPUs
> > with BBML2 but SMMUs without.
> > 
> > On such a system it seems like your series would break previously
> > working SVA support because this patch will end up disabling it?

Perhaps my understanding is flawed here, but I was under the impression
that with SVA both the core and smmu MUST support BBML2 to use it safely
for core translations? Otherwise the smmu might experience page faults
when it touches pages from the core that use BBML2, if it does not
support BBML2 itself? Again, I could very well be wrong, will double
check with the reference manuals.

> > Though I see your MIDR_REV list is limited, so perhaps that worry
> > doesn't effect any real chips made with those families? I am trying to
> > check some NVIDIA products against this list..

Hopefully, as you say, the MIDR list restricts the breakage to a limited
(ideally, zero-size) set of implementations which advertise BBML2
without conflict aborts, but which do not support BBML2 on the smmu.

However, if my understanding of the BBML2 feature and how it interacts
with SVA is flawed, this will obviously be something for me to fix.

> We do have implementations that support CPUs with BBLM2 with TLB
> conflict aborts and SMMUv3 with BBML2.  So don't think those platforms
> be affected by this.  Will check with our hardware folks if there is
> anything that will be affected by this.
> 
> Also we  have plans to try to use SMMUv3 BBML2 during VM live migration
> to split block pages to 4K. I guess, in that case we can enable SMMU BBML2
> independent of CPU side.
> 
> Thanks,
> Shameer

On independently enabling BBML2 on the smmu but not the CPU, this should
be possible. My check was intended to catch the case where the CPU is on
the MIDR allowlist, but the smmu does not support the feature. Whilst
this might still be bad logic, if it is correct, then as long as the CPU
is not in the allowlist sva should not be affected. And bbml2 itself on
the smmu should be safe regardless, as it is stricted than the
corresponding cpu-side feature.

-- 
Kind regards,
Mikołaj Lenczewski
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Jason Gunthorpe 11 months, 1 week ago 
On Mon, Mar 03, 2025 at 10:31:02AM +0000, Mikołaj Lenczewski wrote:

> > > On such a system it seems like your series would break previously
> > > working SVA support because this patch will end up disabling it?
> 
> Perhaps my understanding is flawed here, but I was under the impression
> that with SVA both the core and smmu MUST support BBML2 to use it safely
> for core translations? 

Yes

But today's kernel does not use BBML2 in the CPU or the SMMU so it is
compatible with everything.

So it is an upgrade issue, going from today's kernel without any BBML2
support to tomorrow's kernel that does then you loose SVA on
previously working HW.

> Hopefully, as you say, the MIDR list restricts the breakage to a limited
> (ideally, zero-size) set of implementations which advertise BBML2
> without conflict aborts, but which do not support BBML2 on the smmu.
> 
> However, if my understanding of the BBML2 feature and how it interacts
> with SVA is flawed, this will obviously be something for me to fix.

Lets hope, I was not able to discover any NVIDIA platforms that have
an issue with this series as is.

But every addition to the MIDR list will require some consideration :\

> On independently enabling BBML2 on the smmu but not the CPU, this should
> be possible.

What about the reverse? Could we disable BBML2 on the CPU side on a
per-mm basis? Ie when an old SMMU attaches with disable the
incompatible feature? Not something for this series, but if we get
into trouble down the road

Jason
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Mikołaj Lenczewski 11 months, 1 week ago 
On Mon, Mar 03, 2025 at 12:52:55PM -0400, Jason Gunthorpe wrote:
> On Mon, Mar 03, 2025 at 10:31:02AM +0000, Mikołaj Lenczewski wrote:
> 
> > > > On such a system it seems like your series would break previously
> > > > working SVA support because this patch will end up disabling it?
> > 
> > Perhaps my understanding is flawed here, but I was under the impression
> > that with SVA both the core and smmu MUST support BBML2 to use it safely
> > for core translations? 
> 
> Yes
> 
> But today's kernel does not use BBML2 in the CPU or the SMMU so it is
> compatible with everything.
> 
> So it is an upgrade issue, going from today's kernel without any BBML2
> support to tomorrow's kernel that does then you loose SVA on
> previously working HW.

I see what you mean.

If you have hardware that supports cpu BBML2 but doesn't support smmu
BBML2, either by virtue of having a pre-SMMUv3.2 smmu, or by only
implementing BBML1, the simplest option I see is then to disable BBML2
globally in the kernel. A bad klude, but would at least leave current
SVA working.

> > Hopefully, as you say, the MIDR list restricts the breakage to a limited
> > (ideally, zero-size) set of implementations which advertise BBML2
> > without conflict aborts, but which do not support BBML2 on the smmu.
> > 
> > However, if my understanding of the BBML2 feature and how it interacts
> > with SVA is flawed, this will obviously be something for me to fix.
> 
> Lets hope, I was not able to discover any NVIDIA platforms that have
> an issue with this series as is.
> 
> But every addition to the MIDR list will require some consideration :\

Yes, I agree. I believe that for larger cores, this is a guarantee that
implementors will be able to make when adding themselves to the list.
They know what smmu their core uses, and that tends to be
performance-focused (hence more likely to use an smmu that support
BBML2).

For smaller cores, i.e. mobile cores, this becomes a larger issue as you
may have a higher likelyhood of pairing such a core with an
older/insufficient smmu version. So, careful consideration is required.

> > On independently enabling BBML2 on the smmu but not the CPU, this should
> > be possible.
> 
> What about the reverse? Could we disable BBML2 on the CPU side on a
> per-mm basis? Ie when an old SMMU attaches with disable the
> incompatible feature? Not something for this series, but if we get
> into trouble down the road

I agree in principle with having, as you point out, some mechanism
to disable BBML2 when an smmu without BBML2 is used. However, I think
that the complexity of such a mechanism depends on how the BBML2 feature
is used by future patches.

For example, if we use BBML2 for kernel mappings, does that require us
to repaint all kernel mappings when disabling BBML2 on smmu attach? I
am not sure, but definitely something to be worked out.

-- 
Kind regards,
Mikołaj Lenczewski
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Jason Gunthorpe 11 months, 1 week ago 
On Mon, Mar 03, 2025 at 07:03:42PM +0000, Mikołaj Lenczewski wrote:

> For example, if we use BBML2 for kernel mappings, does that require us
> to repaint all kernel mappings when disabling BBML2 on smmu attach? I
> am not sure, but definitely something to be worked out.

No, it would be a per-mm_struct basis only if we did something like
that

When the SMMU driver puts a SVA on top of the mm_struct it would
disable BBML2 usage only for that mm_struct and it's contained VMAs.

Kernel mappings are never made into a SVA so are not effected.
Userspace only.

Jason
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Ryan Roberts 11 months, 1 week ago 
On 04/03/2025 14:26, Jason Gunthorpe wrote:
> On Mon, Mar 03, 2025 at 07:03:42PM +0000, Mikołaj Lenczewski wrote:
> 
>> For example, if we use BBML2 for kernel mappings, does that require us
>> to repaint all kernel mappings when disabling BBML2 on smmu attach? I
>> am not sure, but definitely something to be worked out.
> 
> No, it would be a per-mm_struct basis only if we did something like
> that
> 
> When the SMMU driver puts a SVA on top of the mm_struct it would
> disable BBML2 usage only for that mm_struct and it's contained VMAs.

I guess we would need to figure out some synchonization mechanism if disabling
BBML2 dynaically per-mm. If there was already a BBML2 operation in flight would
want to wait for it to end. But that's a problem to solve if/when it's shown to
be needed, I think.

> 
> Kernel mappings are never made into a SVA so are not effected.
> Userspace only.
> 
> Jason
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Jason Gunthorpe 11 months, 1 week ago 
On Tue, Mar 04, 2025 at 04:02:20PM +0000, Ryan Roberts wrote:
> On 04/03/2025 14:26, Jason Gunthorpe wrote:
> > On Mon, Mar 03, 2025 at 07:03:42PM +0000, Mikołaj Lenczewski wrote:
> > 
> >> For example, if we use BBML2 for kernel mappings, does that require us
> >> to repaint all kernel mappings when disabling BBML2 on smmu attach? I
> >> am not sure, but definitely something to be worked out.
> > 
> > No, it would be a per-mm_struct basis only if we did something like
> > that
> > 
> > When the SMMU driver puts a SVA on top of the mm_struct it would
> > disable BBML2 usage only for that mm_struct and it's contained VMAs.
> 
> I guess we would need to figure out some synchonization mechanism if disabling
> BBML2 dynaically per-mm. If there was already a BBML2 operation in flight would
> want to wait for it to end. But that's a problem to solve if/when it's shown to
> be needed, I think.

I have a feeling we can piggyback on the mmu notifiers to achieve this
as all the changes to the PTEs should be bracketed by notifier
callbacks..

Let's hope it isn't needed.

Jasob
Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
Posted by Robin Murphy 11 months ago 
On 04/03/2025 4:19 pm, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 04:02:20PM +0000, Ryan Roberts wrote:
>> On 04/03/2025 14:26, Jason Gunthorpe wrote:
>>> On Mon, Mar 03, 2025 at 07:03:42PM +0000, Mikołaj Lenczewski wrote:
>>>
>>>> For example, if we use BBML2 for kernel mappings, does that require us
>>>> to repaint all kernel mappings when disabling BBML2 on smmu attach? I
>>>> am not sure, but definitely something to be worked out.
>>>
>>> No, it would be a per-mm_struct basis only if we did something like
>>> that
>>>
>>> When the SMMU driver puts a SVA on top of the mm_struct it would
>>> disable BBML2 usage only for that mm_struct and it's contained VMAs.
>>
>> I guess we would need to figure out some synchonization mechanism if disabling
>> BBML2 dynaically per-mm. If there was already a BBML2 operation in flight would
>> want to wait for it to end. But that's a problem to solve if/when it's shown to
>> be needed, I think.
> 
> I have a feeling we can piggyback on the mmu notifiers to achieve this
> as all the changes to the PTEs should be bracketed by notifier
> callbacks..
> 
> Let's hope it isn't needed.

Yup, as mentioned previously, this is largely theoretical and at worst 
only a risk of affecting 3rd-party SMMU implementations. Arm's 
implementations from MMU-700 onwards do support BBML2; MMU-600 *might* 
actually be OK as well, but it predates the definition of the feature, 
and there are more practical reasons not to integrate a decade-old SMMU 
design with brand new CPUs anyway.

Thanks,
Robin.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.