[PATCH v2 0/4] Initial BBML2 support for contpte_convert()

[PATCH v2 0/4] Initial BBML2 support for contpte_convert()

[Mikołaj Lenczewski]

Hi All,

This patch series adds adding initial support for eliding
break-before-make requirements on systems that support BBML2 and
additionally guarantee to never raise a conflict abort.

This support reorders and optionally elides a TLB invalidation in
contpte_convert(). The elision of said invalidation leads to a 12%
improvement when executing a microbenchmark designed to force the
pathological path where contpte_convert() gets called. This
represents an 80% reduction in the cost of calling contpte_convert().

However, even without the elision, the reodering represents a
performance improvement due to reducing thread contention, as there is
a smaller time window for racing threads to see an invalid pagetable
entry (especially if they already have a cached entry in their TLB
that they are working off of).

This series is based on v6.14-rc3 (0ad2507d5d93).

Patch 1 implements an allow-list of cpus that support BBML2, but with
the additional constraint of never causing TLB conflict aborts. We
settled on this constraint because we will use the feature for kernel
mappings in the future, for which we cannot handle conflict aborts
safely.

Yang Shi has a series at [1] that aims to use BBML2 to enable splitting
the linear map at runtime. This series partially overlaps with it to add
the cpu feature. We beleive this series is fully compatible with Yang's
requirements and could go first, given there is still a lot of discussion
around the best way to manage the mechanics of splitting/collapsing the
linear map.

[1]:
  https://lore.kernel.org/linux-arm-kernel/20250103011822.1257189-1-yang@os.amperecomputing.com/

Mikołaj Lenczewski (4):
  arm64: Add BBM Level 2 cpu feature
  arm64/mm: Delay tlbi in contpte_convert() under BBML2
  arm64/mm: Elide tlbi in contpte_convert() under BBML2
  iommu/arm: Add BBM Level 2 smmu feature

 arch/arm64/Kconfig                            | 11 +++
 arch/arm64/include/asm/cpucaps.h              |  2 +
 arch/arm64/include/asm/cpufeature.h           |  5 ++
 arch/arm64/kernel/cpufeature.c                | 68 +++++++++++++++++++
 arch/arm64/mm/contpte.c                       |  3 +-
 arch/arm64/tools/cpucaps                      |  1 +
 .../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 ++
 9 files changed, 99 insertions(+), 1 deletion(-)

-- 
2.45.3

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

[Mikołaj Lenczewski]

The Break-Before-Make cpu feature supports multiple levels (levels 0-2),
and this commit adds a dedicated BBML2 cpufeature to test against
support for.

This is a system feature as we might have a big.LITTLE architecture
where some cores support BBML2 and some don't, but we want all cores to
be available and BBM to default to level 0 (as opposed to having cores
without BBML2 not coming online).

To support BBML2 in as wide a range of contexts as we can, we want not
only the architectural guarantees that BBML2 makes, but additionally
want BBML2 to not create TLB conflict aborts. Not causing aborts avoids
us having to prove that no recursive faults can be induced in any path
that uses BBML2, allowing its use for arbitrary kernel mappings.
Support detection of such CPUs.

Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
---
 arch/arm64/Kconfig                  | 11 +++++
 arch/arm64/include/asm/cpucaps.h    |  2 +
 arch/arm64/include/asm/cpufeature.h |  5 +++
 arch/arm64/kernel/cpufeature.c      | 69 +++++++++++++++++++++++++++++
 arch/arm64/tools/cpucaps            |  1 +
 5 files changed, 88 insertions(+)

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);
+}
+
 int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt);
 bool try_emulate_mrs(struct pt_regs *regs, u32 isn);
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d561cf3b8ac7..63f6d356dc77 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2176,6 +2176,68 @@ 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_ENABLE_BBML2_NOABORT))
+		return false;
+
+	if (scope & SCOPE_SYSTEM) {
+		int cpu;
+
+		/* We are a boot CPU, and must verify that all enumerated boot
+		 * CPUs have MIDR values within our allowlist. Otherwise, we do
+		 * not allow the BBML2 feature to avoid potential faults when
+		 * the insufficient CPUs access memory regions using BBML2
+		 * semantics.
+		 */
+		for_each_online_cpu(cpu) {
+			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
+		 * insufficient CPU, we will not use BBML2 (this is safe). If
+		 * 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());
+	}
+
+	return false;
+}
+
 #ifdef CONFIG_ARM64_PAN
 static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
 {
@@ -2926,6 +2988,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.matches = has_cpuid_feature,
 		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, EVT, IMP)
 	},
+	{
+		.desc = "BBM Level 2 without conflict abort",
+		.capability = ARM64_HAS_BBML2_NOABORT,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_bbml2_noabort,
+		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, BBM, 2)
+	},
 	{
 		.desc = "52-bit Virtual Addressing for KVM (LPA2)",
 		.capability = ARM64_HAS_LPA2,
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 1e65f2fb45bd..b03a375e5507 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -14,6 +14,7 @@ HAS_ADDRESS_AUTH_ARCH_QARMA5
 HAS_ADDRESS_AUTH_IMP_DEF
 HAS_AMU_EXTN
 HAS_ARMv8_4_TTL
+HAS_BBML2_NOABORT
 HAS_CACHE_DIC
 HAS_CACHE_IDC
 HAS_CNP
-- 
2.45.3

[PATCH v2 2/4] arm64/mm: Delay tlbi in contpte_convert() under BBML2

[Mikołaj Lenczewski]

When converting a region via contpte_convert() to use mTHP, we have two
different goals. We have to mark each entry as contiguous, and we would
like to smear the dirty and young (access) bits across all entries in
the contiguous block. Currently, we do this by first accumulating the
dirty and young bits in the block, using an atomic
__ptep_get_and_clear() and the relevant pte_{dirty,young}() calls,
performing a tlbi, and finally smearing the correct bits across the
block using __set_ptes().

This approach works fine for BBM level 0, but with support for BBM level
2 we are allowed to reorder the tlbi to after setting the pagetable
entries. This reordering means that other threads will not see an
invalid pagetable entry, instead operating on stale data, until we have
performed our smearing and issued the invalidation. Avoiding this
invalid entry reduces faults in other threads, and thus improves
performance marginally (more so when there are more threads).

Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
---
 arch/arm64/mm/contpte.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index 55107d27d3f8..145530f706a9 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -68,9 +68,13 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
 			pte = pte_mkyoung(pte);
 	}
 
-	__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
+	if (!system_supports_bbml2_noabort())
+		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
 
 	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
+
+	if (system_supports_bbml2_noabort())
+		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
 }
 
 void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
-- 
2.45.3

[PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[Mikołaj Lenczewski]

If we support bbml2 without conflict aborts, we can avoid the final
flush and have hardware manage the tlb entries for us. Avoiding flushes
is a win.

Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
---
 arch/arm64/mm/contpte.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index 145530f706a9..77ed03b30b72 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
 		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
 
 	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
-
-	if (system_supports_bbml2_noabort())
-		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
 }
 
 void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
-- 
2.45.3

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

[Mikołaj Lenczewski]

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

[Jason Gunthorpe]

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 1/4] arm64: Add BBM Level 2 cpu feature

[Yang Shi]

Hi Miko,

Thanks for getting this work. It is perfect timing. I'm going to have my 
series build on top of this patch.

Yang



On 2/28/25 10:24 AM, Mikołaj Lenczewski wrote:
> The Break-Before-Make cpu feature supports multiple levels (levels 0-2),
> and this commit adds a dedicated BBML2 cpufeature to test against
> support for.
>
> This is a system feature as we might have a big.LITTLE architecture
> where some cores support BBML2 and some don't, but we want all cores to
> be available and BBM to default to level 0 (as opposed to having cores
> without BBML2 not coming online).
>
> To support BBML2 in as wide a range of contexts as we can, we want not
> only the architectural guarantees that BBML2 makes, but additionally
> want BBML2 to not create TLB conflict aborts. Not causing aborts avoids
> us having to prove that no recursive faults can be induced in any path
> that uses BBML2, allowing its use for arbitrary kernel mappings.
> Support detection of such CPUs.
>
> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> ---
>   arch/arm64/Kconfig                  | 11 +++++
>   arch/arm64/include/asm/cpucaps.h    |  2 +
>   arch/arm64/include/asm/cpufeature.h |  5 +++
>   arch/arm64/kernel/cpufeature.c      | 69 +++++++++++++++++++++++++++++
>   arch/arm64/tools/cpucaps            |  1 +
>   5 files changed, 88 insertions(+)
>
> 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);
> +}
> +
>   int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt);
>   bool try_emulate_mrs(struct pt_regs *regs, u32 isn);
>   
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d561cf3b8ac7..63f6d356dc77 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2176,6 +2176,68 @@ 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_ENABLE_BBML2_NOABORT))
> +		return false;
> +
> +	if (scope & SCOPE_SYSTEM) {
> +		int cpu;
> +
> +		/* We are a boot CPU, and must verify that all enumerated boot
> +		 * CPUs have MIDR values within our allowlist. Otherwise, we do
> +		 * not allow the BBML2 feature to avoid potential faults when
> +		 * the insufficient CPUs access memory regions using BBML2
> +		 * semantics.
> +		 */
> +		for_each_online_cpu(cpu) {
> +			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
> +		 * insufficient CPU, we will not use BBML2 (this is safe). If
> +		 * 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());
> +	}
> +
> +	return false;
> +}
> +
>   #ifdef CONFIG_ARM64_PAN
>   static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
>   {
> @@ -2926,6 +2988,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>   		.matches = has_cpuid_feature,
>   		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, EVT, IMP)
>   	},
> +	{
> +		.desc = "BBM Level 2 without conflict abort",
> +		.capability = ARM64_HAS_BBML2_NOABORT,
> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
> +		.matches = has_bbml2_noabort,
> +		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, BBM, 2)
> +	},
>   	{
>   		.desc = "52-bit Virtual Addressing for KVM (LPA2)",
>   		.capability = ARM64_HAS_LPA2,
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index 1e65f2fb45bd..b03a375e5507 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -14,6 +14,7 @@ HAS_ADDRESS_AUTH_ARCH_QARMA5
>   HAS_ADDRESS_AUTH_IMP_DEF
>   HAS_AMU_EXTN
>   HAS_ARMv8_4_TTL
> +HAS_BBML2_NOABORT
>   HAS_CACHE_DIC
>   HAS_CACHE_IDC
>   HAS_CNP

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

[Yang Shi]

On 2/28/25 10:24 AM, Mikołaj Lenczewski wrote:
> The Break-Before-Make cpu feature supports multiple levels (levels 0-2),
> and this commit adds a dedicated BBML2 cpufeature to test against
> support for.
>
> This is a system feature as we might have a big.LITTLE architecture
> where some cores support BBML2 and some don't, but we want all cores to
> be available and BBM to default to level 0 (as opposed to having cores
> without BBML2 not coming online).
>
> To support BBML2 in as wide a range of contexts as we can, we want not
> only the architectural guarantees that BBML2 makes, but additionally
> want BBML2 to not create TLB conflict aborts. Not causing aborts avoids
> us having to prove that no recursive faults can be induced in any path
> that uses BBML2, allowing its use for arbitrary kernel mappings.
> Support detection of such CPUs.
>
> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> ---
>   arch/arm64/Kconfig                  | 11 +++++
>   arch/arm64/include/asm/cpucaps.h    |  2 +
>   arch/arm64/include/asm/cpufeature.h |  5 +++
>   arch/arm64/kernel/cpufeature.c      | 69 +++++++++++++++++++++++++++++
>   arch/arm64/tools/cpucaps            |  1 +
>   5 files changed, 88 insertions(+)
>
> 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.

Thanks,
Yang


> +
>   int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt);
>   bool try_emulate_mrs(struct pt_regs *regs, u32 isn);
>   
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d561cf3b8ac7..63f6d356dc77 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2176,6 +2176,68 @@ 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_ENABLE_BBML2_NOABORT))
> +		return false;
> +
> +	if (scope & SCOPE_SYSTEM) {
> +		int cpu;
> +
> +		/* We are a boot CPU, and must verify that all enumerated boot
> +		 * CPUs have MIDR values within our allowlist. Otherwise, we do
> +		 * not allow the BBML2 feature to avoid potential faults when
> +		 * the insufficient CPUs access memory regions using BBML2
> +		 * semantics.
> +		 */
> +		for_each_online_cpu(cpu) {
> +			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
> +		 * insufficient CPU, we will not use BBML2 (this is safe). If
> +		 * 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());
> +	}
> +
> +	return false;
> +}
> +
>   #ifdef CONFIG_ARM64_PAN
>   static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
>   {
> @@ -2926,6 +2988,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>   		.matches = has_cpuid_feature,
>   		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, EVT, IMP)
>   	},
> +	{
> +		.desc = "BBM Level 2 without conflict abort",
> +		.capability = ARM64_HAS_BBML2_NOABORT,
> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
> +		.matches = has_bbml2_noabort,
> +		ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, BBM, 2)
> +	},
>   	{
>   		.desc = "52-bit Virtual Addressing for KVM (LPA2)",
>   		.capability = ARM64_HAS_LPA2,
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index 1e65f2fb45bd..b03a375e5507 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -14,6 +14,7 @@ HAS_ADDRESS_AUTH_ARCH_QARMA5
>   HAS_ADDRESS_AUTH_IMP_DEF
>   HAS_AMU_EXTN
>   HAS_ARMv8_4_TTL
> +HAS_BBML2_NOABORT
>   HAS_CACHE_DIC
>   HAS_CACHE_IDC
>   HAS_CNP

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

[Yang Shi]

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 1/4] arm64: Add BBM Level 2 cpu feature

[Yang Shi]

On 2/28/25 5:29 PM, Yang Shi wrote:
>
>
>
> On 2/28/25 10:24 AM, Mikołaj Lenczewski wrote:
>> The Break-Before-Make cpu feature supports multiple levels (levels 0-2),
>> and this commit adds a dedicated BBML2 cpufeature to test against
>> support for.
>>
>> This is a system feature as we might have a big.LITTLE architecture
>> where some cores support BBML2 and some don't, but we want all cores to
>> be available and BBM to default to level 0 (as opposed to having cores
>> without BBML2 not coming online).
>>
>> To support BBML2 in as wide a range of contexts as we can, we want not
>> only the architectural guarantees that BBML2 makes, but additionally
>> want BBML2 to not create TLB conflict aborts. Not causing aborts avoids
>> us having to prove that no recursive faults can be induced in any path
>> that uses BBML2, allowing its use for arbitrary kernel mappings.
>> Support detection of such CPUs.
>>
>> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
>> ---
>>   arch/arm64/Kconfig                  | 11 +++++
>>   arch/arm64/include/asm/cpucaps.h    |  2 +
>>   arch/arm64/include/asm/cpufeature.h |  5 +++
>>   arch/arm64/kernel/cpufeature.c      | 69 +++++++++++++++++++++++++++++
>>   arch/arm64/tools/cpucaps            |  1 +
>>   5 files changed, 88 insertions(+)
>>
>> 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


>
> Thanks,
> Yang
>
>
>> +
>>   int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt);
>>   bool try_emulate_mrs(struct pt_regs *regs, u32 isn);
>>   diff --git a/arch/arm64/kernel/cpufeature.c 
>> b/arch/arm64/kernel/cpufeature.c
>> index d561cf3b8ac7..63f6d356dc77 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -2176,6 +2176,68 @@ 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_ENABLE_BBML2_NOABORT))
>> +        return false;
>> +
>> +    if (scope & SCOPE_SYSTEM) {
>> +        int cpu;
>> +
>> +        /* We are a boot CPU, and must verify that all enumerated boot
>> +         * CPUs have MIDR values within our allowlist. Otherwise, we do
>> +         * not allow the BBML2 feature to avoid potential faults when
>> +         * the insufficient CPUs access memory regions using BBML2
>> +         * semantics.
>> +         */
>> +        for_each_online_cpu(cpu) {
>> +            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
>> +         * insufficient CPU, we will not use BBML2 (this is safe). If
>> +         * 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());
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   #ifdef CONFIG_ARM64_PAN
>>   static void cpu_enable_pan(const struct arm64_cpu_capabilities 
>> *__unused)
>>   {
>> @@ -2926,6 +2988,13 @@ static const struct arm64_cpu_capabilities 
>> arm64_features[] = {
>>           .matches = has_cpuid_feature,
>>           ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, EVT, IMP)
>>       },
>> +    {
>> +        .desc = "BBM Level 2 without conflict abort",
>> +        .capability = ARM64_HAS_BBML2_NOABORT,
>> +        .type = ARM64_CPUCAP_SYSTEM_FEATURE,
>> +        .matches = has_bbml2_noabort,
>> +        ARM64_CPUID_FIELDS(ID_AA64MMFR2_EL1, BBM, 2)
>> +    },
>>       {
>>           .desc = "52-bit Virtual Addressing for KVM (LPA2)",
>>           .capability = ARM64_HAS_LPA2,
>> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
>> index 1e65f2fb45bd..b03a375e5507 100644
>> --- a/arch/arm64/tools/cpucaps
>> +++ b/arch/arm64/tools/cpucaps
>> @@ -14,6 +14,7 @@ HAS_ADDRESS_AUTH_ARCH_QARMA5
>>   HAS_ADDRESS_AUTH_IMP_DEF
>>   HAS_AMU_EXTN
>>   HAS_ARMv8_4_TTL
>> +HAS_BBML2_NOABORT
>>   HAS_CACHE_DIC
>>   HAS_CACHE_IDC
>>   HAS_CNP
>

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

[Shameerali Kolothum Thodi]

> -----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 0/4] Initial BBML2 support for contpte_convert()

[David Hildenbrand]

On 28.02.25 19:24, Mikołaj Lenczewski wrote:
> Hi All,
> 
> This patch series adds adding initial support for eliding
> break-before-make requirements on systems that support BBML2 and
> additionally guarantee to never raise a conflict abort.
> 
> This support reorders and optionally elides a TLB invalidation in
> contpte_convert(). The elision of said invalidation leads to a 12%
> improvement when executing a microbenchmark designed to force the
> pathological path where contpte_convert() gets called. This
> represents an 80% reduction in the cost of calling contpte_convert().

Just a general comment: Nice! :)

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[David Hildenbrand]

On 28.02.25 19:24, Mikołaj Lenczewski wrote:
> If we support bbml2 without conflict aborts, we can avoid the final
> flush and have hardware manage the tlb entries for us. Avoiding flushes
> is a win.
> 
> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   arch/arm64/mm/contpte.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index 145530f706a9..77ed03b30b72 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
>   		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>   
>   	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
> -
> -	if (system_supports_bbml2_noabort())
> -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>   }
>   
>   void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,

What's the point of not squashing this into #2? :)

If this split was requested during earlier review, at least seeing patch 
#2 on its own confused me.

-- 
Cheers,

David / dhildenb

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

[Mikołaj Lenczewski]

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

Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[Mikołaj Lenczewski]

Hi David,

Thanks for taking the time to review.

On Mon, Mar 03, 2025 at 10:17:12AM +0100, David Hildenbrand wrote:
> On 28.02.25 19:24, Mikołaj Lenczewski wrote:
> > If we support bbml2 without conflict aborts, we can avoid the final
> > flush and have hardware manage the tlb entries for us. Avoiding flushes
> > is a win.
> > 
> > Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> > ---
> >   arch/arm64/mm/contpte.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> > index 145530f706a9..77ed03b30b72 100644
> > --- a/arch/arm64/mm/contpte.c
> > +++ b/arch/arm64/mm/contpte.c
> > @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
> >   		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
> >   	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
> > -
> > -	if (system_supports_bbml2_noabort())
> > -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
> >   }
> >   void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
> 
> What's the point of not squashing this into #2? :)
> 
> If this split was requested during earlier review, at least seeing patch #2
> on its own confused me.

This split is a holdover from an earlier patchset, where it was still
unknown whether the removal of the second flush was permitted with
BBML2. Partly this was due to us being worried about conflict aborts
after the removal, and partly this was because the "delay" is a separate
optimisation that we could apply even if it turned out the final patch
was not architecturally sound.

Now that we do not handle conflict aborts (preferring only systems that
handle BBML2 without ever raising aborts), the first issue is not a
problem. The reasoning behind the second patch is also a little bit
outdated, but I can see the logical split between a tlbi reorder, and
the removal of the tlbi. If this is truly redundant though, I would be
happy to squash the two into a single patch.

-- 
Kind regards,
Mikołaj Lenczewski

Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[David Hildenbrand]

On 03.03.25 10:49, Mikołaj Lenczewski wrote:
> Hi David,
> 
> Thanks for taking the time to review.
> 
> On Mon, Mar 03, 2025 at 10:17:12AM +0100, David Hildenbrand wrote:
>> On 28.02.25 19:24, Mikołaj Lenczewski wrote:
>>> If we support bbml2 without conflict aborts, we can avoid the final
>>> flush and have hardware manage the tlb entries for us. Avoiding flushes
>>> is a win.
>>>
>>> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
>>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>    arch/arm64/mm/contpte.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>> index 145530f706a9..77ed03b30b72 100644
>>> --- a/arch/arm64/mm/contpte.c
>>> +++ b/arch/arm64/mm/contpte.c
>>> @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
>>>    		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>    	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
>>> -
>>> -	if (system_supports_bbml2_noabort())
>>> -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>    }
>>>    void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
>>
>> What's the point of not squashing this into #2? :)
>>
>> If this split was requested during earlier review, at least seeing patch #2
>> on its own confused me.
> 
> This split is a holdover from an earlier patchset, where it was still
> unknown whether the removal of the second flush was permitted with
> BBML2. Partly this was due to us being worried about conflict aborts
> after the removal, and partly this was because the "delay" is a separate
> optimisation that we could apply even if it turned out the final patch
> was not architecturally sound.
> 
> Now that we do not handle conflict aborts (preferring only systems that
> handle BBML2 without ever raising aborts), the first issue is not a
> problem. The reasoning behind the second patch is also a little bit
> outdated, but I can see the logical split between a tlbi reorder, and
> the removal of the tlbi. If this is truly redundant though, I would be
> happy to squash the two into a single patch.

Thanks for the information.

Does patch #2 (reordering the tlbi) have any benefit on its own? I read 
"other threads will not see an invalid pagetable entry", but I am not 
sure that is correct. A concurrent HW page table walker would still find 
the invalid PTE? It's just a matter of TLB state.

If there is no benefit in having patch #2 independently, I'd just squash 
them. Reordering to then remove is more complicated than just removing 
it IMHO.

-- 
Cheers,

David / dhildenb

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

[Ryan Roberts]

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

[Mikołaj Lenczewski]

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

[Mikołaj Lenczewski]

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 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[Mikołaj Lenczewski]

On Mon, Mar 03, 2025 at 10:57:21AM +0100, David Hildenbrand wrote:
> On 03.03.25 10:49, Mikołaj Lenczewski wrote:
> > Hi David,
> > 
> > Thanks for taking the time to review.
> > 
> > On Mon, Mar 03, 2025 at 10:17:12AM +0100, David Hildenbrand wrote:
> > > On 28.02.25 19:24, Mikołaj Lenczewski wrote:
> > > > If we support bbml2 without conflict aborts, we can avoid the final
> > > > flush and have hardware manage the tlb entries for us. Avoiding flushes
> > > > is a win.
> > > > 
> > > > Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> > > > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> > > > ---
> > > >    arch/arm64/mm/contpte.c | 3 ---
> > > >    1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> > > > index 145530f706a9..77ed03b30b72 100644
> > > > --- a/arch/arm64/mm/contpte.c
> > > > +++ b/arch/arm64/mm/contpte.c
> > > > @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
> > > >    		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
> > > >    	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
> > > > -
> > > > -	if (system_supports_bbml2_noabort())
> > > > -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
> > > >    }
> > > >    void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
> > > 
> > > What's the point of not squashing this into #2? :)
> > > 
> > > If this split was requested during earlier review, at least seeing patch #2
> > > on its own confused me.
> > 
> > This split is a holdover from an earlier patchset, where it was still
> > unknown whether the removal of the second flush was permitted with
> > BBML2. Partly this was due to us being worried about conflict aborts
> > after the removal, and partly this was because the "delay" is a separate
> > optimisation that we could apply even if it turned out the final patch
> > was not architecturally sound.
> > 
> > Now that we do not handle conflict aborts (preferring only systems that
> > handle BBML2 without ever raising aborts), the first issue is not a
> > problem. The reasoning behind the second patch is also a little bit
> > outdated, but I can see the logical split between a tlbi reorder, and
> > the removal of the tlbi. If this is truly redundant though, I would be
> > happy to squash the two into a single patch.
> 
> Thanks for the information.
> 
> Does patch #2 (reordering the tlbi) have any benefit on its own? I read
> "other threads will not see an invalid pagetable entry", but I am not sure
> that is correct. A concurrent HW page table walker would still find the
> invalid PTE? It's just a matter of TLB state.

I think I understand what you mean. I agree that it is possible for a
concurrent walk to see an invalid TLBI state, if it is on the same TLB
that the repaint is happening on. For other TLBs, the flush has not yet
propagated our invalidated PTEs (from `__ptep_get_and_clear()`) though?
That invalidation will only be seen by other TLBs after the
`__flush_tlb_range()`, so we should save a few faults because only
"local" threads will ever see the invalid entry, as opposed to all
threads that try to read our modified range? Or it is the case that I
have misunderstood something basic here, or that I have misinterpreted
what you have written?

> If there is no benefit in having patch #2 independently, I'd just squash
> them. Reordering to then remove is more complicated than just removing it
> IMHO.
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

-- 
Kind regards,
Mikołaj Lenczewski

Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[David Hildenbrand]

On 03.03.25 11:55, Mikołaj Lenczewski wrote:
> On Mon, Mar 03, 2025 at 10:57:21AM +0100, David Hildenbrand wrote:
>> On 03.03.25 10:49, Mikołaj Lenczewski wrote:
>>> Hi David,
>>>
>>> Thanks for taking the time to review.
>>>
>>> On Mon, Mar 03, 2025 at 10:17:12AM +0100, David Hildenbrand wrote:
>>>> On 28.02.25 19:24, Mikołaj Lenczewski wrote:
>>>>> If we support bbml2 without conflict aborts, we can avoid the final
>>>>> flush and have hardware manage the tlb entries for us. Avoiding flushes
>>>>> is a win.
>>>>>
>>>>> Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
>>>>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>> ---
>>>>>     arch/arm64/mm/contpte.c | 3 ---
>>>>>     1 file changed, 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>>>> index 145530f706a9..77ed03b30b72 100644
>>>>> --- a/arch/arm64/mm/contpte.c
>>>>> +++ b/arch/arm64/mm/contpte.c
>>>>> @@ -72,9 +72,6 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr,
>>>>>     		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>>>     	__set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES);
>>>>> -
>>>>> -	if (system_supports_bbml2_noabort())
>>>>> -		__flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3);
>>>>>     }
>>>>>     void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
>>>>
>>>> What's the point of not squashing this into #2? :)
>>>>
>>>> If this split was requested during earlier review, at least seeing patch #2
>>>> on its own confused me.
>>>
>>> This split is a holdover from an earlier patchset, where it was still
>>> unknown whether the removal of the second flush was permitted with
>>> BBML2. Partly this was due to us being worried about conflict aborts
>>> after the removal, and partly this was because the "delay" is a separate
>>> optimisation that we could apply even if it turned out the final patch
>>> was not architecturally sound.
>>>
>>> Now that we do not handle conflict aborts (preferring only systems that
>>> handle BBML2 without ever raising aborts), the first issue is not a
>>> problem. The reasoning behind the second patch is also a little bit
>>> outdated, but I can see the logical split between a tlbi reorder, and
>>> the removal of the tlbi. If this is truly redundant though, I would be
>>> happy to squash the two into a single patch.
>>
>> Thanks for the information.
>>
>> Does patch #2 (reordering the tlbi) have any benefit on its own? I read
>> "other threads will not see an invalid pagetable entry", but I am not sure
>> that is correct. A concurrent HW page table walker would still find the
>> invalid PTE? It's just a matter of TLB state.
> 
> I think I understand what you mean. I agree that it is possible for a
> concurrent walk to see an invalid TLBI state, if it is on the same TLB
> that the repaint is happening on. For other TLBs, the flush has not yet
> propagated our invalidated PTEs (from `__ptep_get_and_clear()`) though?

What I am saying is: if there is no TLB entry yet, HW will walk the page 
table to find no present PTE and trigger a fault.

> That invalidation will only be seen by other TLBs after the
> `__flush_tlb_range()`, so we should save a few faults because only
> "local" threads will ever see the invalid entry, as opposed to all
> threads that try to read our modified range?

So what you say is, that deferring the flush means that if there is 
already a TLB entry, flushing deferred reduces the likelihood that a 
page table walk is triggered that could find no present PTE: 
consequently, reducing the likelihood that a page fault is triggered.

(I use the word likelihood, because I assume other action could result 
in a TLB entry getting flushed in the meantime, such as TLB entry reuse)

Correct?

Or it is the case that I
> have misunderstood something basic here, or that I have misinterpreted
> what you have written?

No, that makes it clearer, thanks.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] arm64/mm: Elide tlbi in contpte_convert() under BBML2

[Mikołaj Lenczewski]

> > I think I understand what you mean. I agree that it is possible for a
> > concurrent walk to see an invalid TLBI state, if it is on the same TLB
> > that the repaint is happening on. For other TLBs, the flush has not yet
> > propagated our invalidated PTEs (from `__ptep_get_and_clear()`) though?
> 
> What I am saying is: if there is no TLB entry yet, HW will walk the page
> table to find no present PTE and trigger a fault.

Yes, that is 100% correct. I believe that this is unavoidable.

> > That invalidation will only be seen by other TLBs after the
> > `__flush_tlb_range()`, so we should save a few faults because only
> > "local" threads will ever see the invalid entry, as opposed to all
> > threads that try to read our modified range?
> 
> So what you say is, that deferring the flush means that if there is already
> a TLB entry, flushing deferred reduces the likelihood that a page table walk
> is triggered that could find no present PTE: consequently, reducing the
> likelihood that a page fault is triggered.
> 
> (I use the word likelihood, because I assume other action could result in a
> TLB entry getting flushed in the meantime, such as TLB entry reuse)
> 
> Correct?

Yes, and your language here is clearer than the original commit message
(and cover letter). Will amend it to be closer to your wording.

-- 
Kind regards,
Mikołaj Lenczewski

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

[Jason Gunthorpe]

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

[Mikołaj Lenczewski]

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 1/4] arm64: Add BBM Level 2 cpu feature

[Yang Shi]

On 3/3/25 1:40 AM, Mikołaj Lenczewski wrote:
> 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.

Thank you. I didn't spot any other problem on our machines. With this 
addressed, you can have Tested-by: Yang Shi <yang@amperecomputing.com>, 
although I can't test on asymmetric system.

Yang


>

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

[Yang Shi]

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

[Jason Gunthorpe]

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

[Ryan Roberts]

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

[Jason Gunthorpe]

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

[Suzuki K Poulose]

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

[Ryan Roberts]

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

[Suzuki K Poulose]

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

[Ryan Roberts]

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

[Robin Murphy]

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.