[PATCH rc v5 0/4] iommu/arm-smmu-v3: Fix hitless STE update in nesting cases

[PATCH rc v5 0/4] iommu/arm-smmu-v3: Fix hitless STE update in nesting cases

[Nicolin Chen]

Occasional C_BAD_STE errors were observed in nesting setups where a device
attached to a nested bypass/identity domain enables PASID.

This occurred when the physical STE was updated from S2-only mode to S1+S2
nesting mode, but the update failed to use the hitless routine that it was
supposed to use. Instead, it cleared STE.V bit to load the CD table, while
the default substream was still actively performing DMA.

It was later found that the diff algorithm in arm_smmu_entry_qword_diff()
enforced an additional critical word due to misaligned MEV and EATS fields
between S2-only and S1+S2 modes.

Both fields are either well-managed or non-critical, so move them to the
"ignored" list to relax the qword diff algorithm.

Additionally, add KUnit test coverage for these nesting STE cases.

This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_ste_fixes/

A host kernel must apply this to fix the bug.

Changelog
v5:
 * Pass in feat to arm_smmu_test_make_s2_ste()
v4:
 https://lore.kernel.org/all/cover.1765945258.git.nicolinc@nvidia.com/
 * s/ignored/update_safe
 * Change entry_set to void
v3:
 https://lore.kernel.org/all/cover.1765334526.git.nicolinc@nvidia.com/
 * Add Reviewed-by from Shuai
 * Add an inline comments in nested test cases
 * Reuse arm_smmu_test_make_cdtable_ste() for nested test cases
v2:
 https://lore.kernel.org/all/cover.1765140287.git.nicolinc@nvidia.com/
 * Fix kunit tests
 * Update commit message and inline comments
 * Keep MEV/EATS in used list by masking them away using ignored_bits
v1:
 https://lore.kernel.org/all/cover.1764982046.git.nicolinc@nvidia.com/

Jason Gunthorpe (3):
  iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence
  iommu/arm-smmu-v3: Mark STE MEV safe when computing the update
    sequence
  iommu/arm-smmu-v3: Mark STE EATS safe when computing the update
    sequence

Nicolin Chen (1):
  iommu/arm-smmu-v3-test: Add nested s1bypass/s1dssbypass coverage

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 65 ++++++++++++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 46 +++++++++++--
 3 files changed, 103 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

C_BAD_STE was observed when updating nested STE from an S1-bypass mode to
an S1DSS-bypass mode. As both modes enabled S2, the used bit is slightly
different than the normal S1-bypass and S1DSS-bypass modes. As a result,
fields like MEV and EATS in S2's used list marked the word1 as a critical
word that requested a STE.V=0. This breaks a hitless update.

However, both MEV and EATS aren't critical in terms of STE update. One
controls the merge of the events and the other controls the ATS that is
managed by the driver at the same time via pci_enable_ats().

Add an arm_smmu_get_ste_update_safe() to allow STE update algorithm to
relax those fields, avoiding the STE update breakages.

After this change, entry_set has no caller checking its return value, so
change it to void.

Note that this change is required by both MEV and EATS fields, which were
introduced in different kernel versions. So add get_update_safe() first.
MEV and EATS will be added to arm_smmu_get_ste_update_safe() separately.

Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
 3 files changed, 37 insertions(+), 10 deletions(-)

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 ae23aacc3840..a6c976fa9df2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -900,6 +900,7 @@ struct arm_smmu_entry_writer {
 
 struct arm_smmu_entry_writer_ops {
 	void (*get_used)(const __le64 *entry, __le64 *used);
+	void (*get_update_safe)(__le64 *safe_bits);
 	void (*sync)(struct arm_smmu_entry_writer *writer);
 };
 
@@ -911,6 +912,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 
 #if IS_ENABLED(CONFIG_KUNIT)
 void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits);
+void arm_smmu_get_ste_update_safe(__le64 *safe_bits);
 void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *cur,
 			  const __le64 *target);
 void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
index d2671bfd3798..5db14718fdd6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
@@ -38,13 +38,16 @@ enum arm_smmu_test_master_feat {
 static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
 						const __le64 *used_bits,
 						const __le64 *target,
+						const __le64 *safe,
 						unsigned int length)
 {
 	bool differs = false;
 	unsigned int i;
 
 	for (i = 0; i < length; i++) {
-		if ((entry[i] & used_bits[i]) != target[i])
+		__le64 used = used_bits[i] & ~safe[i];
+
+		if ((entry[i] & used) != (target[i] & used))
 			differs = true;
 	}
 	return differs;
@@ -56,12 +59,17 @@ arm_smmu_test_writer_record_syncs(struct arm_smmu_entry_writer *writer)
 	struct arm_smmu_test_writer *test_writer =
 		container_of(writer, struct arm_smmu_test_writer, writer);
 	__le64 *entry_used_bits;
+	__le64 *safe;
 
 	entry_used_bits = kunit_kzalloc(
 		test_writer->test, sizeof(*entry_used_bits) * NUM_ENTRY_QWORDS,
 		GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test_writer->test, entry_used_bits);
 
+	safe = kunit_kzalloc(test_writer->test,
+			     sizeof(*safe) * NUM_ENTRY_QWORDS, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test_writer->test, safe);
+
 	pr_debug("STE value is now set to: ");
 	print_hex_dump_debug("    ", DUMP_PREFIX_NONE, 16, 8,
 			     test_writer->entry,
@@ -79,14 +87,17 @@ arm_smmu_test_writer_record_syncs(struct arm_smmu_entry_writer *writer)
 		 * configuration.
 		 */
 		writer->ops->get_used(test_writer->entry, entry_used_bits);
+		if (writer->ops->get_update_safe)
+			writer->ops->get_update_safe(safe);
 		KUNIT_EXPECT_FALSE(
 			test_writer->test,
 			arm_smmu_entry_differs_in_used_bits(
 				test_writer->entry, entry_used_bits,
-				test_writer->init_entry, NUM_ENTRY_QWORDS) &&
+				test_writer->init_entry, safe,
+				NUM_ENTRY_QWORDS) &&
 				arm_smmu_entry_differs_in_used_bits(
 					test_writer->entry, entry_used_bits,
-					test_writer->target_entry,
+					test_writer->target_entry, safe,
 					NUM_ENTRY_QWORDS));
 	}
 }
@@ -106,6 +117,7 @@ arm_smmu_v3_test_debug_print_used_bits(struct arm_smmu_entry_writer *writer,
 static const struct arm_smmu_entry_writer_ops test_ste_ops = {
 	.sync = arm_smmu_test_writer_record_syncs,
 	.get_used = arm_smmu_get_ste_used,
+	.get_update_safe = arm_smmu_get_ste_update_safe,
 };
 
 static const struct arm_smmu_entry_writer_ops test_cd_ops = {
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 d16d35c78c06..8dbf4ad5b51e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1082,6 +1082,12 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
 
+VISIBLE_IF_KUNIT
+void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
+{
+}
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);
+
 /*
  * Figure out if we can do a hitless update of entry to become target. Returns a
  * bit mask where 1 indicates that qword needs to be set disruptively.
@@ -1094,13 +1100,22 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
 {
 	__le64 target_used[NUM_ENTRY_QWORDS] = {};
 	__le64 cur_used[NUM_ENTRY_QWORDS] = {};
+	__le64 safe[NUM_ENTRY_QWORDS] = {};
 	u8 used_qword_diff = 0;
 	unsigned int i;
 
 	writer->ops->get_used(entry, cur_used);
 	writer->ops->get_used(target, target_used);
+	if (writer->ops->get_update_safe)
+		writer->ops->get_update_safe(safe);
 
 	for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
+		/*
+		 * Safe is only used for bits that are used by both entries,
+		 * otherwise it is sequenced according to the unused entry.
+		 */
+		safe[i] &= target_used[i] & cur_used[i];
+
 		/*
 		 * Check that masks are up to date, the make functions are not
 		 * allowed to set a bit to 1 if the used function doesn't say it
@@ -1109,6 +1124,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
 		WARN_ON_ONCE(target[i] & ~target_used[i]);
 
 		/* Bits can change because they are not currently being used */
+		cur_used[i] &= ~safe[i];
 		unused_update[i] = (entry[i] & cur_used[i]) |
 				   (target[i] & ~cur_used[i]);
 		/*
@@ -1121,7 +1137,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
 	return used_qword_diff;
 }
 
-static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
+static void entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
 		      const __le64 *target, unsigned int start,
 		      unsigned int len)
 {
@@ -1137,7 +1153,6 @@ static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
 
 	if (changed)
 		writer->ops->sync(writer);
-	return changed;
 }
 
 /*
@@ -1207,12 +1222,9 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 		entry_set(writer, entry, target, 0, 1);
 	} else {
 		/*
-		 * No inuse bit changed. Sanity check that all unused bits are 0
-		 * in the entry. The target was already sanity checked by
-		 * compute_qword_diff().
+		 * No inuse bit changed, though safe bits may have changed.
 		 */
-		WARN_ON_ONCE(
-			entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
+		entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS);
 	}
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_write_entry);
@@ -1543,6 +1555,7 @@ static void arm_smmu_ste_writer_sync_entry(struct arm_smmu_entry_writer *writer)
 static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
 	.sync = arm_smmu_ste_writer_sync_entry,
 	.get_used = arm_smmu_get_ste_used,
+	.get_update_safe = arm_smmu_get_ste_update_safe,
 };
 
 static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
-- 
2.43.0

[PATCH rc v5 2/4] iommu/arm-smmu-v3: Mark STE MEV safe when computing the update sequence

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

Nested CD tables set the MEV bit to try to reduce multi-fault spamming on
the hypervisor. Since MEV is in STE word 1 this causes a breaking update
sequence that is not required and impacts real workloads.

For the purposes of STE updates the value of MEV doesn't matter, if it is
set/cleared early or late it just results in a change to the fault reports
that must be supported by the kernel anyhow. The spec says:

 Note: Software must expect, and be able to deal with, coalesced fault
 records even when MEV == 0.

So mark STE MEV safe when computing the update sequence, to avoid creating
a breaking update.

Fixes: da0c56520e88 ("iommu/arm-smmu-v3: Set MEV bit in nested STE for DoS mitigations")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

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 8dbf4ad5b51e..12a9669bcc83 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1085,6 +1085,16 @@ EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
 VISIBLE_IF_KUNIT
 void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
 {
+	/*
+	 * MEV does not meaningfully impact the operation of the HW, it only
+	 * changes how many fault events are generated, thus we can relax it
+	 * when computing the ordering. The spec notes the device can act like
+	 * MEV=1 anyhow:
+	 *
+	 *  Note: Software must expect, and be able to deal with, coalesced
+	 *  fault records even when MEV == 0.
+	 */
+	safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);
 
-- 
2.43.0

[PATCH rc v5 3/4] iommu/arm-smmu-v3: Mark STE EATS safe when computing the update sequence

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

If a VM wants to toggle EATS off at the same time as changing the CFG, the
hypervisor will see EATS change to 0 and insert a V=0 breaking update into
the STE even though the VM did not ask for that.

In bare metal, EATS is ignored by CFG=ABORT/BYPASS, which is why this does
not cause a problem until we have nested where CFG is always a variation of
S2 trans that does use EATS.

Relax the rules for EATS sequencing, we don't need it to be exact because
the enclosing code will always disable ATS at the PCI device if we are
changing EATS. This ensures there are no ATS transactions that can race
with an EATS change so we don't need to carefully sequence these bits.

Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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 12a9669bcc83..a3b29ad20a82 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1095,6 +1095,15 @@ void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
 	 *  fault records even when MEV == 0.
 	 */
 	safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
+
+	/*
+	 * EATS is used to reject and control the ATS behavior of the device. If
+	 * we are changing it away from 0 then we already trust the device to
+	 * use ATS properly and we have sequenced the device's ATS enable in PCI
+	 * config space to prevent it from issuing ATS while we are changing
+	 * EATS.
+	 */
+	safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);
 
-- 
2.43.0

[PATCH rc v5 4/4] iommu/arm-smmu-v3-test: Add nested s1bypass/s1dssbypass coverage

[Nicolin Chen]

STE in a nested case requires both S1 and S2 fields. And this makes the use
case different from the existing one.

Add coverage for previously failed cases shifting between S2-only and S1+S2
STEs.

Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
index 5db14718fdd6..4a072c2c367c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
@@ -33,8 +33,12 @@ static struct mm_struct sva_mm = {
 enum arm_smmu_test_master_feat {
 	ARM_SMMU_MASTER_TEST_ATS = BIT(0),
 	ARM_SMMU_MASTER_TEST_STALL = BIT(1),
+	ARM_SMMU_MASTER_TEST_NESTED = BIT(2),
 };
 
+static void arm_smmu_test_make_s2_ste(struct arm_smmu_ste *ste,
+				      enum arm_smmu_test_master_feat feat);
+
 static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
 						const __le64 *used_bits,
 						const __le64 *target,
@@ -197,6 +201,18 @@ static void arm_smmu_test_make_cdtable_ste(struct arm_smmu_ste *ste,
 	};
 
 	arm_smmu_make_cdtable_ste(ste, &master, ats_enabled, s1dss);
+	if (feat & ARM_SMMU_MASTER_TEST_NESTED) {
+		struct arm_smmu_ste s2ste;
+		int i;
+
+		arm_smmu_test_make_s2_ste(&s2ste,
+					  feat & ~ARM_SMMU_MASTER_TEST_NESTED);
+		ste->data[0] |= cpu_to_le64(
+			FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_NESTED));
+		ste->data[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
+		for (i = 2; i < NUM_ENTRY_QWORDS; i++)
+			ste->data[i] = s2ste.data[i];
+	}
 }
 
 static void arm_smmu_v3_write_ste_test_bypass_to_abort(struct kunit *test)
@@ -554,6 +570,35 @@ static void arm_smmu_v3_write_ste_test_s2_to_s1_stall(struct kunit *test)
 						       NUM_EXPECTED_SYNCS(3));
 }
 
+static void
+arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass(struct kunit *test)
+{
+	struct arm_smmu_ste s1_ste;
+	struct arm_smmu_ste s2_ste;
+
+	arm_smmu_test_make_cdtable_ste(
+		&s1_ste, STRTAB_STE_1_S1DSS_BYPASS, fake_cdtab_dma_addr,
+		ARM_SMMU_MASTER_TEST_ATS | ARM_SMMU_MASTER_TEST_NESTED);
+	arm_smmu_test_make_s2_ste(&s2_ste, 0);
+	/* Expect an additional sync to unset ignored bits: EATS and MEV */
+	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s1_ste, &s2_ste,
+						       NUM_EXPECTED_SYNCS(3));
+}
+
+static void
+arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass(struct kunit *test)
+{
+	struct arm_smmu_ste s1_ste;
+	struct arm_smmu_ste s2_ste;
+
+	arm_smmu_test_make_cdtable_ste(
+		&s1_ste, STRTAB_STE_1_S1DSS_BYPASS, fake_cdtab_dma_addr,
+		ARM_SMMU_MASTER_TEST_ATS | ARM_SMMU_MASTER_TEST_NESTED);
+	arm_smmu_test_make_s2_ste(&s2_ste, 0);
+	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s2_ste, &s1_ste,
+						       NUM_EXPECTED_SYNCS(2));
+}
+
 static void arm_smmu_v3_write_cd_test_sva_clear(struct kunit *test)
 {
 	struct arm_smmu_cd cd = {};
@@ -600,6 +645,8 @@ static struct kunit_case arm_smmu_v3_test_cases[] = {
 	KUNIT_CASE(arm_smmu_v3_write_cd_test_s1_change_asid),
 	KUNIT_CASE(arm_smmu_v3_write_ste_test_s1_to_s2_stall),
 	KUNIT_CASE(arm_smmu_v3_write_ste_test_s2_to_s1_stall),
+	KUNIT_CASE(arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass),
+	KUNIT_CASE(arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass),
 	KUNIT_CASE(arm_smmu_v3_write_cd_test_sva_clear),
 	KUNIT_CASE(arm_smmu_v3_write_cd_test_sva_release),
 	{},
-- 
2.43.0

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Mostafa Saleh]

On Thu, Dec 18, 2025 at 01:41:56PM -0800, Nicolin Chen wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> C_BAD_STE was observed when updating nested STE from an S1-bypass mode to
> an S1DSS-bypass mode. As both modes enabled S2, the used bit is slightly
> different than the normal S1-bypass and S1DSS-bypass modes. As a result,
> fields like MEV and EATS in S2's used list marked the word1 as a critical
> word that requested a STE.V=0. This breaks a hitless update.
> 
> However, both MEV and EATS aren't critical in terms of STE update. One
> controls the merge of the events and the other controls the ATS that is
> managed by the driver at the same time via pci_enable_ats().
> 
> Add an arm_smmu_get_ste_update_safe() to allow STE update algorithm to
> relax those fields, avoiding the STE update breakages.
> 
> After this change, entry_set has no caller checking its return value, so
> change it to void.
> 
> Note that this change is required by both MEV and EATS fields, which were
> introduced in different kernel versions. So add get_update_safe() first.
> MEV and EATS will be added to arm_smmu_get_ste_update_safe() separately.
> 
> Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Mostafa Saleh <smostafa@google.com>
Thanks,
Mostafa

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> 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 ae23aacc3840..a6c976fa9df2 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -900,6 +900,7 @@ struct arm_smmu_entry_writer {
>  
>  struct arm_smmu_entry_writer_ops {
>  	void (*get_used)(const __le64 *entry, __le64 *used);
> +	void (*get_update_safe)(__le64 *safe_bits);
>  	void (*sync)(struct arm_smmu_entry_writer *writer);
>  };
>  
> @@ -911,6 +912,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>  
>  #if IS_ENABLED(CONFIG_KUNIT)
>  void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits);
> +void arm_smmu_get_ste_update_safe(__le64 *safe_bits);
>  void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *cur,
>  			  const __le64 *target);
>  void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits);
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> index d2671bfd3798..5db14718fdd6 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> @@ -38,13 +38,16 @@ enum arm_smmu_test_master_feat {
>  static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
>  						const __le64 *used_bits,
>  						const __le64 *target,
> +						const __le64 *safe,
>  						unsigned int length)
>  {
>  	bool differs = false;
>  	unsigned int i;
>  
>  	for (i = 0; i < length; i++) {
> -		if ((entry[i] & used_bits[i]) != target[i])
> +		__le64 used = used_bits[i] & ~safe[i];
> +
> +		if ((entry[i] & used) != (target[i] & used))
>  			differs = true;
>  	}
>  	return differs;
> @@ -56,12 +59,17 @@ arm_smmu_test_writer_record_syncs(struct arm_smmu_entry_writer *writer)
>  	struct arm_smmu_test_writer *test_writer =
>  		container_of(writer, struct arm_smmu_test_writer, writer);
>  	__le64 *entry_used_bits;
> +	__le64 *safe;
>  
>  	entry_used_bits = kunit_kzalloc(
>  		test_writer->test, sizeof(*entry_used_bits) * NUM_ENTRY_QWORDS,
>  		GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_NULL(test_writer->test, entry_used_bits);
>  
> +	safe = kunit_kzalloc(test_writer->test,
> +			     sizeof(*safe) * NUM_ENTRY_QWORDS, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test_writer->test, safe);
> +
>  	pr_debug("STE value is now set to: ");
>  	print_hex_dump_debug("    ", DUMP_PREFIX_NONE, 16, 8,
>  			     test_writer->entry,
> @@ -79,14 +87,17 @@ arm_smmu_test_writer_record_syncs(struct arm_smmu_entry_writer *writer)
>  		 * configuration.
>  		 */
>  		writer->ops->get_used(test_writer->entry, entry_used_bits);
> +		if (writer->ops->get_update_safe)
> +			writer->ops->get_update_safe(safe);
>  		KUNIT_EXPECT_FALSE(
>  			test_writer->test,
>  			arm_smmu_entry_differs_in_used_bits(
>  				test_writer->entry, entry_used_bits,
> -				test_writer->init_entry, NUM_ENTRY_QWORDS) &&
> +				test_writer->init_entry, safe,
> +				NUM_ENTRY_QWORDS) &&
>  				arm_smmu_entry_differs_in_used_bits(
>  					test_writer->entry, entry_used_bits,
> -					test_writer->target_entry,
> +					test_writer->target_entry, safe,
>  					NUM_ENTRY_QWORDS));
>  	}
>  }
> @@ -106,6 +117,7 @@ arm_smmu_v3_test_debug_print_used_bits(struct arm_smmu_entry_writer *writer,
>  static const struct arm_smmu_entry_writer_ops test_ste_ops = {
>  	.sync = arm_smmu_test_writer_record_syncs,
>  	.get_used = arm_smmu_get_ste_used,
> +	.get_update_safe = arm_smmu_get_ste_update_safe,
>  };
>  
>  static const struct arm_smmu_entry_writer_ops test_cd_ops = {
> 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 d16d35c78c06..8dbf4ad5b51e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1082,6 +1082,12 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>  }
>  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
>  
> +VISIBLE_IF_KUNIT
> +void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
> +{
> +}
> +EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);
> +
>  /*
>   * Figure out if we can do a hitless update of entry to become target. Returns a
>   * bit mask where 1 indicates that qword needs to be set disruptively.
> @@ -1094,13 +1100,22 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
>  {
>  	__le64 target_used[NUM_ENTRY_QWORDS] = {};
>  	__le64 cur_used[NUM_ENTRY_QWORDS] = {};
> +	__le64 safe[NUM_ENTRY_QWORDS] = {};
>  	u8 used_qword_diff = 0;
>  	unsigned int i;
>  
>  	writer->ops->get_used(entry, cur_used);
>  	writer->ops->get_used(target, target_used);
> +	if (writer->ops->get_update_safe)
> +		writer->ops->get_update_safe(safe);
>  
>  	for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
> +		/*
> +		 * Safe is only used for bits that are used by both entries,
> +		 * otherwise it is sequenced according to the unused entry.
> +		 */
> +		safe[i] &= target_used[i] & cur_used[i];
> +
>  		/*
>  		 * Check that masks are up to date, the make functions are not
>  		 * allowed to set a bit to 1 if the used function doesn't say it
> @@ -1109,6 +1124,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
>  		WARN_ON_ONCE(target[i] & ~target_used[i]);
>  
>  		/* Bits can change because they are not currently being used */
> +		cur_used[i] &= ~safe[i];
>  		unused_update[i] = (entry[i] & cur_used[i]) |
>  				   (target[i] & ~cur_used[i]);
>  		/*
> @@ -1121,7 +1137,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
>  	return used_qword_diff;
>  }
>  
> -static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
> +static void entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
>  		      const __le64 *target, unsigned int start,
>  		      unsigned int len)
>  {
> @@ -1137,7 +1153,6 @@ static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
>  
>  	if (changed)
>  		writer->ops->sync(writer);
> -	return changed;
>  }
>  
>  /*
> @@ -1207,12 +1222,9 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
>  		entry_set(writer, entry, target, 0, 1);
>  	} else {
>  		/*
> -		 * No inuse bit changed. Sanity check that all unused bits are 0
> -		 * in the entry. The target was already sanity checked by
> -		 * compute_qword_diff().
> +		 * No inuse bit changed, though safe bits may have changed.
>  		 */
> -		WARN_ON_ONCE(
> -			entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
> +		entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS);
>  	}
>  }
>  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_write_entry);
> @@ -1543,6 +1555,7 @@ static void arm_smmu_ste_writer_sync_entry(struct arm_smmu_entry_writer *writer)
>  static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
>  	.sync = arm_smmu_ste_writer_sync_entry,
>  	.get_used = arm_smmu_get_ste_used,
> +	.get_update_safe = arm_smmu_get_ste_update_safe,
>  };
>  
>  static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
> -- 
> 2.43.0
> 

Re: [PATCH rc v5 2/4] iommu/arm-smmu-v3: Mark STE MEV safe when computing the update sequence

[Mostafa Saleh]

On Thu, Dec 18, 2025 at 01:41:57PM -0800, Nicolin Chen wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Nested CD tables set the MEV bit to try to reduce multi-fault spamming on
> the hypervisor. Since MEV is in STE word 1 this causes a breaking update
> sequence that is not required and impacts real workloads.
> 
> For the purposes of STE updates the value of MEV doesn't matter, if it is
> set/cleared early or late it just results in a change to the fault reports
> that must be supported by the kernel anyhow. The spec says:
> 
>  Note: Software must expect, and be able to deal with, coalesced fault
>  records even when MEV == 0.
> 
> So mark STE MEV safe when computing the update sequence, to avoid creating
> a breaking update.
> 
> Fixes: da0c56520e88 ("iommu/arm-smmu-v3: Set MEV bit in nested STE for DoS mitigations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> 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 8dbf4ad5b51e..12a9669bcc83 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1085,6 +1085,16 @@ EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
>  VISIBLE_IF_KUNIT
>  void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
>  {
> +	/*
> +	 * MEV does not meaningfully impact the operation of the HW, it only
> +	 * changes how many fault events are generated, thus we can relax it
> +	 * when computing the ordering. The spec notes the device can act like
> +	 * MEV=1 anyhow:
> +	 *
> +	 *  Note: Software must expect, and be able to deal with, coalesced
> +	 *  fault records even when MEV == 0.
> +	 */
> +	safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
>  }
>  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);
>  
> -- 
> 2.43.0
> 

Re: [PATCH rc v5 4/4] iommu/arm-smmu-v3-test: Add nested s1bypass/s1dssbypass coverage

[Mostafa Saleh]

On Thu, Dec 18, 2025 at 01:41:59PM -0800, Nicolin Chen wrote:
> STE in a nested case requires both S1 and S2 fields. And this makes the use
> case different from the existing one.
> 
> Add coverage for previously failed cases shifting between S2-only and S1+S2
> STEs.
> 
> Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa

> ---
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> index 5db14718fdd6..4a072c2c367c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
> @@ -33,8 +33,12 @@ static struct mm_struct sva_mm = {
>  enum arm_smmu_test_master_feat {
>  	ARM_SMMU_MASTER_TEST_ATS = BIT(0),
>  	ARM_SMMU_MASTER_TEST_STALL = BIT(1),
> +	ARM_SMMU_MASTER_TEST_NESTED = BIT(2),
>  };
>  
> +static void arm_smmu_test_make_s2_ste(struct arm_smmu_ste *ste,
> +				      enum arm_smmu_test_master_feat feat);
> +
>  static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
>  						const __le64 *used_bits,
>  						const __le64 *target,
> @@ -197,6 +201,18 @@ static void arm_smmu_test_make_cdtable_ste(struct arm_smmu_ste *ste,
>  	};
>  
>  	arm_smmu_make_cdtable_ste(ste, &master, ats_enabled, s1dss);
> +	if (feat & ARM_SMMU_MASTER_TEST_NESTED) {
> +		struct arm_smmu_ste s2ste;
> +		int i;
> +
> +		arm_smmu_test_make_s2_ste(&s2ste,
> +					  feat & ~ARM_SMMU_MASTER_TEST_NESTED);
> +		ste->data[0] |= cpu_to_le64(
> +			FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_NESTED));
> +		ste->data[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
> +		for (i = 2; i < NUM_ENTRY_QWORDS; i++)
> +			ste->data[i] = s2ste.data[i];
> +	}
>  }
>  
>  static void arm_smmu_v3_write_ste_test_bypass_to_abort(struct kunit *test)
> @@ -554,6 +570,35 @@ static void arm_smmu_v3_write_ste_test_s2_to_s1_stall(struct kunit *test)
>  						       NUM_EXPECTED_SYNCS(3));
>  }
>  
> +static void
> +arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass(struct kunit *test)
> +{
> +	struct arm_smmu_ste s1_ste;
> +	struct arm_smmu_ste s2_ste;
> +
> +	arm_smmu_test_make_cdtable_ste(
> +		&s1_ste, STRTAB_STE_1_S1DSS_BYPASS, fake_cdtab_dma_addr,
> +		ARM_SMMU_MASTER_TEST_ATS | ARM_SMMU_MASTER_TEST_NESTED);
> +	arm_smmu_test_make_s2_ste(&s2_ste, 0);
> +	/* Expect an additional sync to unset ignored bits: EATS and MEV */
> +	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s1_ste, &s2_ste,
> +						       NUM_EXPECTED_SYNCS(3));
> +}
> +
> +static void
> +arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass(struct kunit *test)
> +{
> +	struct arm_smmu_ste s1_ste;
> +	struct arm_smmu_ste s2_ste;
> +
> +	arm_smmu_test_make_cdtable_ste(
> +		&s1_ste, STRTAB_STE_1_S1DSS_BYPASS, fake_cdtab_dma_addr,
> +		ARM_SMMU_MASTER_TEST_ATS | ARM_SMMU_MASTER_TEST_NESTED);
> +	arm_smmu_test_make_s2_ste(&s2_ste, 0);
> +	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s2_ste, &s1_ste,
> +						       NUM_EXPECTED_SYNCS(2));
> +}
> +
>  static void arm_smmu_v3_write_cd_test_sva_clear(struct kunit *test)
>  {
>  	struct arm_smmu_cd cd = {};
> @@ -600,6 +645,8 @@ static struct kunit_case arm_smmu_v3_test_cases[] = {
>  	KUNIT_CASE(arm_smmu_v3_write_cd_test_s1_change_asid),
>  	KUNIT_CASE(arm_smmu_v3_write_ste_test_s1_to_s2_stall),
>  	KUNIT_CASE(arm_smmu_v3_write_ste_test_s2_to_s1_stall),
> +	KUNIT_CASE(arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass),
> +	KUNIT_CASE(arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass),
>  	KUNIT_CASE(arm_smmu_v3_write_cd_test_sva_clear),
>  	KUNIT_CASE(arm_smmu_v3_write_cd_test_sva_release),
>  	{},
> -- 
> 2.43.0
> 

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Will Deacon]

On Thu, Dec 18, 2025 at 01:41:56PM -0800, Nicolin Chen wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> C_BAD_STE was observed when updating nested STE from an S1-bypass mode to
> an S1DSS-bypass mode. As both modes enabled S2, the used bit is slightly
> different than the normal S1-bypass and S1DSS-bypass modes. As a result,
> fields like MEV and EATS in S2's used list marked the word1 as a critical
> word that requested a STE.V=0. This breaks a hitless update.
> 
> However, both MEV and EATS aren't critical in terms of STE update. One
> controls the merge of the events and the other controls the ATS that is
> managed by the driver at the same time via pci_enable_ats().
> 
> Add an arm_smmu_get_ste_update_safe() to allow STE update algorithm to
> relax those fields, avoiding the STE update breakages.
> 
> After this change, entry_set has no caller checking its return value, so
> change it to void.
> 
> Note that this change is required by both MEV and EATS fields, which were
> introduced in different kernel versions. So add get_update_safe() first.
> MEV and EATS will be added to arm_smmu_get_ste_update_safe() separately.
> 
> Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
>  3 files changed, 37 insertions(+), 10 deletions(-)

Hmm. So this appears to ignore the safe bits entirely, whereas the
rationale for the change is that going from {MEV,EATS} disabled to
enabled is safe (which I agree with). So what prevents an erroneous
hitless STE update when going from {MEV,EATS} enabled to disabled after
this change?

Will

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Wed, Jan 07, 2026 at 09:20:06PM +0000, Will Deacon wrote:
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
> >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
> >  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> Hmm. So this appears to ignore the safe bits entirely, whereas the
> rationale for the change is that going from {MEV,EATS} disabled to
> enabled is safe (which I agree with). 

The argument was it doesn't matter for either direction be it disabled
to enabled or vice versa, see my reply to Mustfa in the v4 posting:

https://lore.kernel.org/all/20251218180129.GA254720@nvidia.com/

> So what prevents an erroneous hitless STE update when going from
> {MEV,EATS} enabled to disabled after this change?

Nothing, it isn't erroneous.

Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Will Deacon]

On Wed, Jan 07, 2026 at 08:36:46PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 07, 2026 at 09:20:06PM +0000, Will Deacon wrote:
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
> > >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
> > >  3 files changed, 37 insertions(+), 10 deletions(-)
> > 
> > Hmm. So this appears to ignore the safe bits entirely, whereas the
> > rationale for the change is that going from {MEV,EATS} disabled to
> > enabled is safe (which I agree with). 
> 
> The argument was it doesn't matter for either direction be it disabled
> to enabled or vice versa, see my reply to Mustfa in the v4 posting:
> 
> https://lore.kernel.org/all/20251218180129.GA254720@nvidia.com/

It would be good to include some of that rationale in the comment and
commit message for patch 3, as at the moment it only talks about the
change in one direction.

I'm also still not convinced that this is generally safe, even if it
works within what Linux currently does. For example, if somebody tries
to disable S2S and enable ATS at the same time, couldn't you transiently
get an illegal STE?

Will

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Mon, Jan 12, 2026 at 03:53:29PM +0000, Will Deacon wrote:
> On Wed, Jan 07, 2026 at 08:36:46PM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 07, 2026 at 09:20:06PM +0000, Will Deacon wrote:
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
> > > >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 18 ++++++++++---
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 27 ++++++++++++++-----
> > > >  3 files changed, 37 insertions(+), 10 deletions(-)
> > > 
> > > Hmm. So this appears to ignore the safe bits entirely, whereas the
> > > rationale for the change is that going from {MEV,EATS} disabled to
> > > enabled is safe (which I agree with). 
> > 
> > The argument was it doesn't matter for either direction be it disabled
> > to enabled or vice versa, see my reply to Mustfa in the v4 posting:
> > 
> > https://lore.kernel.org/all/20251218180129.GA254720@nvidia.com/
> 
> It would be good to include some of that rationale in the comment and
> commit message for patch 3, as at the moment it only talks about the
> change in one direction.

Sure, I can help Nicolin with that.

> I'm also still not convinced that this is generally safe, even if it
> works within what Linux currently does. For example, if somebody tries
> to disable S2S and enable ATS at the same time, couldn't you transiently
> get an illegal STE?

I would argue that the driver will never concurrently support S2S and
ATS together for the same device, it doesn't make sense as far as I
can understand.

You are correct that there is an illegal STE issue here in this case.

However, keep in mind, if there is concurrent DMA while the driver is
trying to do such a thing there must be a STE error, and we should try
to make it be a non-valid STE error.

Still, it seems easy enough to improve, do not add EATS to the safe
bits if either the current or new STE has S2S set. That will force a
V=0 and avoid the illegal STE. Nicolin?

Thanks,
Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Nicolin Chen]

On Mon, Jan 12, 2026 at 12:10:10PM -0400, Jason Gunthorpe wrote:
> Still, it seems easy enough to improve, do not add EATS to the safe
> bits if either the current or new STE has S2S set. That will force a
> V=0 and avoid the illegal STE. Nicolin?

Ack. I made the following changes:
-----------------------------------------------------------------
@@ -1083,7 +1083,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);

 VISIBLE_IF_KUNIT
-void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
+void arm_smmu_get_ste_update_safe(const __le64 *cur, const __le64 *target,
+                                 __le64 *safe_bits)
 {
        /*
         * MEV does not meaningfully impact the operation of the HW, it only
@@ -1097,13 +1098,22 @@ void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
        safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);

        /*
-        * EATS is used to reject and control the ATS behavior of the device. If
-        * we are changing it away from 0 then we already trust the device to
-        * use ATS properly and we have sequenced the device's ATS enable in PCI
-        * config space to prevent it from issuing ATS while we are changing
-        * EATS.
+        * When a STE comes to change EATS the sequencing code in the attach
+        * logic already will have the PCI cap for ATS disabled. Thus at this
+        * moment we can expect that the device will not generate ATS queries
+        * and so we don't care about the sequencing of EATS. The purpose of
+        * EATS is to protect the system from hostile untrusted devices that
+        * issue ATS when the PCI config space is disabled. However, if EATS
+        * is being changed then we already must be trusting the device since
+        * the EATS security block is being disabled.
+        *
+        *  Note: Since we moved the EATS update to the first phase, changing
+        *  S2S and EATS might transiently set S2S=1 and EATS=1, resulting in
+        *  a bad STE. See "5.2 Stream Table Entry". In such a case, we can't
+        *  do a hitless update.
         */
-       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
+       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
+               safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_update_safe);

-----------------------------------------------------------------

I'll send v6 after running some tests.

Thanks
Nicolin

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Will Deacon]

On Mon, Jan 12, 2026 at 10:58:22AM -0800, Nicolin Chen wrote:
> On Mon, Jan 12, 2026 at 12:10:10PM -0400, Jason Gunthorpe wrote:
> > Still, it seems easy enough to improve, do not add EATS to the safe
> > bits if either the current or new STE has S2S set. That will force a
> > V=0 and avoid the illegal STE. Nicolin?
> 
> Ack. I made the following changes:
> -----------------------------------------------------------------
> @@ -1083,7 +1083,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
> 
>  VISIBLE_IF_KUNIT
> -void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
> +void arm_smmu_get_ste_update_safe(const __le64 *cur, const __le64 *target,
> +                                 __le64 *safe_bits)
>  {
>         /*
>          * MEV does not meaningfully impact the operation of the HW, it only
> @@ -1097,13 +1098,22 @@ void arm_smmu_get_ste_update_safe(__le64 *safe_bits)
>         safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
> 
>         /*
> -        * EATS is used to reject and control the ATS behavior of the device. If
> -        * we are changing it away from 0 then we already trust the device to
> -        * use ATS properly and we have sequenced the device's ATS enable in PCI
> -        * config space to prevent it from issuing ATS while we are changing
> -        * EATS.
> +        * When a STE comes to change EATS the sequencing code in the attach
> +        * logic already will have the PCI cap for ATS disabled. Thus at this
> +        * moment we can expect that the device will not generate ATS queries
> +        * and so we don't care about the sequencing of EATS. The purpose of
> +        * EATS is to protect the system from hostile untrusted devices that
> +        * issue ATS when the PCI config space is disabled. However, if EATS
> +        * is being changed then we already must be trusting the device since
> +        * the EATS security block is being disabled.
> +        *
> +        *  Note: Since we moved the EATS update to the first phase, changing
> +        *  S2S and EATS might transiently set S2S=1 and EATS=1, resulting in
> +        *  a bad STE. See "5.2 Stream Table Entry". In such a case, we can't
> +        *  do a hitless update.
>          */
> -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> +               safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);

I suppose we shouldn't ever see the case that they both have S2S, but
that's fine.

The spec also suggests that there's an additional illegal STE case w/
split-stage ATS (EATS_S1CHK) if Config != S1+S2.

I do wonder whether having all the hitless machinery alongside this
"safe" stuff is really overkill and we wouldn't be better off just
checking the cases that we actually care about rather than checking
architecturally and then subtracting the cases we don't care about.

Will

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Tue, Jan 13, 2026 at 03:05:52PM +0000, Will Deacon wrote:
> I suppose we shouldn't ever see the case that they both have S2S, but
> that's fine.

If they both have S2S then it works correctly? Any S2S forces EATS to
follow the normal rules.

> The spec also suggests that there's an additional illegal STE case w/
> split-stage ATS (EATS_S1CHK) if Config != S1+S2.

The driver doesn't support that either..

It is fixed by checking if new EATS is valid under old config and old
EATS valid under new config.

Also to support S1CHK someday we cannot allow the hypervisor to leave
S1_S2 and go to S2, since the HW can't deal with that...

> I do wonder whether having all the hitless machinery alongside this
> "safe" stuff is really overkill and we wouldn't be better off just
> checking the cases that we actually care about rather than checking
> architecturally and then subtracting the cases we don't care about.

I'm not sure what you are thinking here. I'd argue that v4 was like
that because it was correct with in the limits of the current driver
capability.

Adding more architectural checks the driver cannot hit today is a nice
future proofing. I don't mind doing it and maybe it will save someone
alot of time down the road.

It isn't like there is some easy shortcut to sequence this someplace
else. Eg the S1CHK stuff above, is very complex in the general
case. We'd have many different versions of EATS with different configs
that can be applied in any sequence.

IMHO two spec derived conditionals is a pretty light cost to deal with
that.

This series originated from customer bugs getting spurious STE faults
because a hitless update in the VM was not hitless in the
hypervisor. This is not just a theoretical need.

I don't want to try to shortcut things to only support a few things we
"think" should be needed and find out later it still causes VM visible
misbehavior :(

Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Nicolin Chen]

On Tue, Jan 13, 2026 at 12:12:53PM -0400, Jason Gunthorpe wrote:
> On Tue, Jan 13, 2026 at 03:05:52PM +0000, Will Deacon wrote:
> > I suppose we shouldn't ever see the case that they both have S2S, but
> > that's fine.
> 
> If they both have S2S then it works correctly? Any S2S forces EATS to
> follow the normal rules.
> 
> > The spec also suggests that there's an additional illegal STE case w/
> > split-stage ATS (EATS_S1CHK) if Config != S1+S2.
> 
> The driver doesn't support that either..
> 
> It is fixed by checking if new EATS is valid under old config and old
> EATS valid under new config.
> 
> Also to support S1CHK someday we cannot allow the hypervisor to leave
> S1_S2 and go to S2, since the HW can't deal with that...

Perhaps the safe bits should only have EATS_TRANS, excluding S1CHK:

--------------------------------------------------------------------------
+        * When an STE changes EATS_TRANS, the sequencing code in the attach
+        * logic already will have the PCI cap for ATS disabled. Thus at this
+        * moment we can expect that the device will not generate ATS queries
+        * and so we don't care about the sequencing of EATS. The purpose of
+        * EATS_TRANS is to protect the system from hostile untrusted devices
+        * that issue ATS when the PCI config space is disabled. However, if
+        * EATS_TRANS is being changed, then we must have already trusted the
+        * device as the EATS_TRANS security block is being disabled.
+        *
+        *  Note: now the EATS_TRANS update is moved to the first entry_set().
+        *  Changing S2S and EATS might transiently result in S2S=1 and EATS=1
+        *  which is a bad STE (see "5.2 Stream Table Entry"). In such a case,
+        *  we can't do a hitless update. Also, STRTAB_STE_1_EATS_S1CHK should
+        *  not be added to the safe bits because it relies on CFG[2:0]=0b111,
+        *  to prevent another bad STE.
         */
-       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
+       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
+               safe_bits[1] |= cpu_to_le64(
+                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
--------------------------------------------------------------------------

@will, does this look good to you? I can send a v7 with this.

Thanks
Nicolin

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Tue, Jan 13, 2026 at 12:29:11PM -0800, Nicolin Chen wrote:
> On Tue, Jan 13, 2026 at 12:12:53PM -0400, Jason Gunthorpe wrote:
> > On Tue, Jan 13, 2026 at 03:05:52PM +0000, Will Deacon wrote:
> > > I suppose we shouldn't ever see the case that they both have S2S, but
> > > that's fine.
> > 
> > If they both have S2S then it works correctly? Any S2S forces EATS to
> > follow the normal rules.
> > 
> > > The spec also suggests that there's an additional illegal STE case w/
> > > split-stage ATS (EATS_S1CHK) if Config != S1+S2.
> > 
> > The driver doesn't support that either..
> > 
> > It is fixed by checking if new EATS is valid under old config and old
> > EATS valid under new config.
> > 
> > Also to support S1CHK someday we cannot allow the hypervisor to leave
> > S1_S2 and go to S2, since the HW can't deal with that...
> 
> Perhaps the safe bits should only have EATS_TRANS, excluding S1CHK:
> 
> --------------------------------------------------------------------------
> +        * When an STE changes EATS_TRANS, the sequencing code in the attach
> +        * logic already will have the PCI cap for ATS disabled. Thus at this
> +        * moment we can expect that the device will not generate ATS queries
> +        * and so we don't care about the sequencing of EATS. The purpose of
> +        * EATS_TRANS is to protect the system from hostile untrusted devices
> +        * that issue ATS when the PCI config space is disabled. However, if
> +        * EATS_TRANS is being changed, then we must have already trusted the
> +        * device as the EATS_TRANS security block is being disabled.
> +        *
> +        *  Note: now the EATS_TRANS update is moved to the first entry_set().
> +        *  Changing S2S and EATS might transiently result in S2S=1 and EATS=1
> +        *  which is a bad STE (see "5.2 Stream Table Entry"). In such a case,
> +        *  we can't do a hitless update. Also, STRTAB_STE_1_EATS_S1CHK should
> +        *  not be added to the safe bits because it relies on CFG[2:0]=0b111,
> +        *  to prevent another bad STE.
>          */
> -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> +               safe_bits[1] |= cpu_to_le64(
> +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> --------------------------------------------------------------------------
> 
> @will, does this look good to you? I can send a v7 with this.

That is an easy way to address Will's observation, makes sense to me.

Thanks,
Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Tue, Jan 13, 2026 at 04:51:12PM -0400, Jason Gunthorpe wrote:
> > -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> > +               safe_bits[1] |= cpu_to_le64(
> > +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> > --------------------------------------------------------------------------
> > 
> > @will, does this look good to you? I can send a v7 with this.
> 
> That is an easy way to address Will's observation, makes sense to me.

Ah, but it looks like it can generate an errant view of a EATS that is
neither old or new. Ie value 3, reserved.

I think you should just check if old or new has EATS bit 1 set:

if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)) &&
    !((cur[1] | target[1]) & cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS, 2))))

Which the current driver never does..

Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Nicolin Chen]

On Thu, Jan 15, 2026 at 09:11:51AM -0400, Jason Gunthorpe wrote:
> On Tue, Jan 13, 2026 at 04:51:12PM -0400, Jason Gunthorpe wrote:
> > > -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > > +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> > > +               safe_bits[1] |= cpu_to_le64(
> > > +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> > > --------------------------------------------------------------------------
> > > 
> > > @will, does this look good to you? I can send a v7 with this.
> > 
> > That is an easy way to address Will's observation, makes sense to me.
> 
> Ah, but it looks like it can generate an errant view of a EATS that is
> neither old or new. Ie value 3, reserved.
> 
> I think you should just check if old or new has EATS bit 1 set:
> 
> if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)) &&
>     !((cur[1] | target[1]) & cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS, 2))))
> 
> Which the current driver never does..

The EATS field is completely controlled by the driver. So, we are
safe for now, right?

Should we add this when the driver has the actual support for the
split stage thing?

Thanks
Nicolin

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Jason Gunthorpe]

On Thu, Jan 15, 2026 at 08:25:05AM -0800, Nicolin Chen wrote:
> On Thu, Jan 15, 2026 at 09:11:51AM -0400, Jason Gunthorpe wrote:
> > On Tue, Jan 13, 2026 at 04:51:12PM -0400, Jason Gunthorpe wrote:
> > > > -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > > > +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> > > > +               safe_bits[1] |= cpu_to_le64(
> > > > +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> > > > --------------------------------------------------------------------------
> > > > 
> > > > @will, does this look good to you? I can send a v7 with this.
> > > 
> > > That is an easy way to address Will's observation, makes sense to me.
> > 
> > Ah, but it looks like it can generate an errant view of a EATS that is
> > neither old or new. Ie value 3, reserved.
> > 
> > I think you should just check if old or new has EATS bit 1 set:
> > 
> > if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)) &&
> >     !((cur[1] | target[1]) & cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS, 2))))
> > 
> > Which the current driver never does..
> 
> The EATS field is completely controlled by the driver. So, we are
> safe for now, right?
> 
> Should we add this when the driver has the actual support for the
> split stage thing?

If we have figured it out now I would add it because it would be a big
leap to think the next person will remember about this detail..

But yes, this and the S2S thing don't effect the driver as it is now,
it is just doing work to help future people.

Jason

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Nicolin Chen]

On Thu, Jan 15, 2026 at 12:29:19PM -0400, Jason Gunthorpe wrote:
> On Thu, Jan 15, 2026 at 08:25:05AM -0800, Nicolin Chen wrote:
> > On Thu, Jan 15, 2026 at 09:11:51AM -0400, Jason Gunthorpe wrote:
> > > On Tue, Jan 13, 2026 at 04:51:12PM -0400, Jason Gunthorpe wrote:
> > > > > -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > > > > +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> > > > > +               safe_bits[1] |= cpu_to_le64(
> > > > > +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> > > > > --------------------------------------------------------------------------
> > > > > 
> > > > > @will, does this look good to you? I can send a v7 with this.
> > > > 
> > > > That is an easy way to address Will's observation, makes sense to me.
> > > 
> > > Ah, but it looks like it can generate an errant view of a EATS that is
> > > neither old or new. Ie value 3, reserved.
> > > 
> > > I think you should just check if old or new has EATS bit 1 set:
> > > 
> > > if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)) &&
> > >     !((cur[1] | target[1]) & cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS, 2))))
> > > 
> > > Which the current driver never does..
> > 
> > The EATS field is completely controlled by the driver. So, we are
> > safe for now, right?
> > 
> > Should we add this when the driver has the actual support for the
> > split stage thing?
> 
> If we have figured it out now I would add it because it would be a big
> leap to think the next person will remember about this detail..
> 
> But yes, this and the S2S thing don't effect the driver as it is now,
> it is just doing work to help future people.

OK. Let's add that.

I will send the v7 by the end of the day. Hopefully, Will is okay
with all of these..

Thanks
Nicolin

Re: [PATCH rc v5 1/4] iommu/arm-smmu-v3: Add update_safe bits to fix STE update sequence

[Will Deacon]

On Thu, Jan 15, 2026 at 08:34:43AM -0800, Nicolin Chen wrote:
> On Thu, Jan 15, 2026 at 12:29:19PM -0400, Jason Gunthorpe wrote:
> > On Thu, Jan 15, 2026 at 08:25:05AM -0800, Nicolin Chen wrote:
> > > On Thu, Jan 15, 2026 at 09:11:51AM -0400, Jason Gunthorpe wrote:
> > > > On Tue, Jan 13, 2026 at 04:51:12PM -0400, Jason Gunthorpe wrote:
> > > > > > -       safe_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > > > > > +       if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)))
> > > > > > +               safe_bits[1] |= cpu_to_le64(
> > > > > > +                       FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS));
> > > > > > --------------------------------------------------------------------------
> > > > > > 
> > > > > > @will, does this look good to you? I can send a v7 with this.
> > > > > 
> > > > > That is an easy way to address Will's observation, makes sense to me.
> > > > 
> > > > Ah, but it looks like it can generate an errant view of a EATS that is
> > > > neither old or new. Ie value 3, reserved.
> > > > 
> > > > I think you should just check if old or new has EATS bit 1 set:
> > > > 
> > > > if (!((cur[2] | target[2]) & cpu_to_le64(STRTAB_STE_2_S2S)) &&
> > > >     !((cur[1] | target[1]) & cpu_to_le64(FIELD_PREP(STRTAB_STE_1_EATS, 2))))
> > > > 
> > > > Which the current driver never does..
> > > 
> > > The EATS field is completely controlled by the driver. So, we are
> > > safe for now, right?
> > > 
> > > Should we add this when the driver has the actual support for the
> > > split stage thing?
> > 
> > If we have figured it out now I would add it because it would be a big
> > leap to think the next person will remember about this detail..
> > 
> > But yes, this and the S2S thing don't effect the driver as it is now,
> > it is just doing work to help future people.
> 
> OK. Let's add that.
> 
> I will send the v7 by the end of the day. Hopefully, Will is okay
> with all of these..

Sounds about right but I'll wait and see what you post.

Will