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

[PATCH rc v1 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.

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

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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 49 +++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 49 ++++++++++++++-----
 3 files changed, 85 insertions(+), 15 deletions(-)


base-commit: 0d081b16946ef449fcb35b6edc1ef6f9fea6f0a0
-- 
2.43.0

[PATCH rc v1 1/4] iommu/arm-smmu-v3: Add ignored 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_ignored() to allow STE update algorithm to ignore
those fields, avoiding the STE update breakages.

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

Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.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  | 17 ++++++++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 24 ++++++++++++-------
 3 files changed, 32 insertions(+), 11 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..d5f0e5407b9f 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_ignored)(__le64 *ignored_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_ignored(__le64 *ignored_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..9287904c93a2 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
@@ -37,6 +37,7 @@ enum arm_smmu_test_master_feat {
 
 static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
 						const __le64 *used_bits,
+						const __le64 *ignored,
 						const __le64 *target,
 						unsigned int length)
 {
@@ -44,7 +45,7 @@ static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
 	unsigned int i;
 
 	for (i = 0; i < length; i++) {
-		if ((entry[i] & used_bits[i]) != target[i])
+		if ((entry[i] & used_bits[i]) != (target[i] & ~ignored[i]))
 			differs = true;
 	}
 	return differs;
@@ -56,12 +57,18 @@ 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 *ignored_bits;
 
 	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);
 
+	ignored_bits = kunit_kzalloc(test_writer->test,
+				     sizeof(*ignored_bits) * NUM_ENTRY_QWORDS,
+				     GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test_writer->test, ignored_bits);
+
 	pr_debug("STE value is now set to: ");
 	print_hex_dump_debug("    ", DUMP_PREFIX_NONE, 16, 8,
 			     test_writer->entry,
@@ -79,14 +86,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_ignored)
+			writer->ops->get_ignored(ignored_bits);
 		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) &&
+				ignored_bits, test_writer->init_entry,
+				NUM_ENTRY_QWORDS) &&
 				arm_smmu_entry_differs_in_used_bits(
 					test_writer->entry, entry_used_bits,
-					test_writer->target_entry,
+					ignored_bits, test_writer->target_entry,
 					NUM_ENTRY_QWORDS));
 	}
 }
@@ -106,6 +116,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_ignored = arm_smmu_get_ste_ignored,
 };
 
 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..95a4cfc5882d 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_ignored(__le64 *ignored_bits)
+{
+}
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_ignored);
+
 /*
  * 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,11 +1100,14 @@ 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 ignored[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_ignored)
+		writer->ops->get_ignored(ignored);
 
 	for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
 		/*
@@ -1106,16 +1115,17 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
 		 * allowed to set a bit to 1 if the used function doesn't say it
 		 * is used.
 		 */
-		WARN_ON_ONCE(target[i] & ~target_used[i]);
+		WARN_ON_ONCE(target[i] & ~target_used[i] & ~ignored[i]);
 
 		/* Bits can change because they are not currently being used */
-		unused_update[i] = (entry[i] & cur_used[i]) |
+		unused_update[i] = (entry[i] & (cur_used[i] | ignored[i])) |
 				   (target[i] & ~cur_used[i]);
 		/*
 		 * Each bit indicates that a used bit in a qword needs to be
 		 * changed after unused_update is applied.
 		 */
-		if ((unused_update[i] & target_used[i]) != target[i])
+		if ((unused_update[i] & target_used[i]) !=
+		    (target[i] & ~ignored[i]))
 			used_qword_diff |= 1 << i;
 	}
 	return used_qword_diff;
@@ -1207,12 +1217,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 ignored 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 +1550,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_ignored = arm_smmu_get_ste_ignored,
 };
 
 static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
-- 
2.43.0

[PATCH rc v1 2/4] iommu/arm-smmu-v3: Ignore STE MEV 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 ignore MEV 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>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

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 95a4cfc5882d..2df657c87abd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1052,7 +1052,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
 				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
 				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
-				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
+				    STRTAB_STE_1_EATS);
 		used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
 
 		/*
@@ -1068,7 +1068,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 	if (cfg & BIT(1)) {
 		used_bits[1] |=
 			cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
-				    STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
+				    STRTAB_STE_1_SHCFG);
 		used_bits[2] |=
 			cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
 				    STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
@@ -1085,6 +1085,16 @@ EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
 VISIBLE_IF_KUNIT
 void arm_smmu_get_ste_ignored(__le64 *ignored_bits)
 {
+	/*
+	 * MEV does not meaningfully impact the operation of the HW, it only
+	 * changes how many fault events are generated, thus we can ignore 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.
+	 */
+	ignored_bits[1] |= cpu_to_le64(STRTAB_STE_1_MEV);
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_ignored);
 
-- 
2.43.0

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

[Nicolin Chen]

From: Jason Gunthorpe <jgg@nvidia.com>

If a VM want to toggle EATS off 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.

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>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

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 2df657c87abd..6a982051c3c2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1051,8 +1051,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 		used_bits[1] |=
 			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
 				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
-				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
-				    STRTAB_STE_1_EATS);
+				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW);
 		used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
 
 		/*
@@ -1067,8 +1066,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 	/* S2 translates */
 	if (cfg & BIT(1)) {
 		used_bits[1] |=
-			cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
-				    STRTAB_STE_1_SHCFG);
+			cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_SHCFG);
 		used_bits[2] |=
 			cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
 				    STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
@@ -1095,6 +1093,15 @@ void arm_smmu_get_ste_ignored(__le64 *ignored_bits)
 	 *  fault records even when MEV == 0.
 	 */
 	ignored_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.
+	 */
+	ignored_bits[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_ignored);
 
-- 
2.43.0

[PATCH rc v1 4/4] iommu/arm-smmu-v3-test: Add nested s1bypass 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.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 32 +++++++++++++++++++
 1 file changed, 32 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 9287904c93a2..56bdcf5a517e 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
@@ -553,6 +553,36 @@ 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_s2_ste(&s1_ste, ARM_SMMU_MASTER_TEST_ATS);
+	arm_smmu_test_make_cdtable_ste(&s1_ste, STRTAB_STE_1_S1DSS_SSID0,
+				       fake_cdtab_dma_addr,
+				       ARM_SMMU_MASTER_TEST_ATS);
+	arm_smmu_test_make_s2_ste(&s2_ste, 0);
+	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_s2_ste(&s1_ste, ARM_SMMU_MASTER_TEST_ATS);
+	arm_smmu_test_make_cdtable_ste(&s1_ste, STRTAB_STE_1_S1DSS_SSID0,
+				       fake_cdtab_dma_addr,
+				       ARM_SMMU_MASTER_TEST_ATS);
+	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(3));
+}
+
 static void arm_smmu_v3_write_cd_test_sva_clear(struct kunit *test)
 {
 	struct arm_smmu_cd cd = {};
@@ -599,6 +629,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 v1 4/4] iommu/arm-smmu-v3-test: Add nested s1bypass coverage

[Shuai Xue]

在 2025/12/6 08:52, 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.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 32 +++++++++++++++++++
>   1 file changed, 32 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 9287904c93a2..56bdcf5a517e 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
> @@ -553,6 +553,36 @@ 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_s2_ste(&s1_ste, ARM_SMMU_MASTER_TEST_ATS);
> +	arm_smmu_test_make_cdtable_ste(&s1_ste, STRTAB_STE_1_S1DSS_SSID0,

arm_smmu_test_make_s2_ste() makes a s2 ste and it will be overwrited by
arm_smmu_test_make_cdtable_ste(). Finnaly, we got a s1 STE, not a nested
s1dssbypass ste.

I think we need a function like arm_smmu_make_nested_cd_table_ste()
here.

Besides, from the function name, I think you mean
STRTAB_STE_1_S1DSS_BYPASS?

+	arm_smmu_test_make_s2_ste(&s2_ste, 0);
+	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s1_ste, &s2_ste,
+						       NUM_EXPECTED_SYNCS(3));

With get_ignored(), a nested s1dssbypass STE to a nested s1bypass STE
will be hitless, a.k.a, NUM_EXPECTED_SYNCS(1).

Thanks.
Shuai

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

[Shuai Xue]

在 2025/12/6 08:52, 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_ignored() to allow STE update algorithm to ignore
> those fields, avoiding the STE update breakages.
> 
> Note that this change is required by both MEV and EATS fields, which were
> introduced in different kernel versions. So add this get_ignored() first.
> The MEV and EATS will be added in arm_smmu_get_ste_ignored() separately.
> 
> Fixes: 1e8be08d1c91 ("iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.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  | 17 ++++++++++---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 24 ++++++++++++-------
>   3 files changed, 32 insertions(+), 11 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..d5f0e5407b9f 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_ignored)(__le64 *ignored_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_ignored(__le64 *ignored_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..9287904c93a2 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
> @@ -37,6 +37,7 @@ enum arm_smmu_test_master_feat {
>   
>   static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
>   						const __le64 *used_bits,
> +						const __le64 *ignored,
>   						const __le64 *target,
>   						unsigned int length)
>   {
> @@ -44,7 +45,7 @@ static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
>   	unsigned int i;
>   
>   	for (i = 0; i < length; i++) {
> -		if ((entry[i] & used_bits[i]) != target[i])
> +		if ((entry[i] & used_bits[i]) != (target[i] & ~ignored[i]))
>   			differs = true;
>   	}
>   	return differs;
> @@ -56,12 +57,18 @@ 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 *ignored_bits;
>   
>   	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);
>   
> +	ignored_bits = kunit_kzalloc(test_writer->test,
> +				     sizeof(*ignored_bits) * NUM_ENTRY_QWORDS,
> +				     GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test_writer->test, ignored_bits);
> +
>   	pr_debug("STE value is now set to: ");
>   	print_hex_dump_debug("    ", DUMP_PREFIX_NONE, 16, 8,
>   			     test_writer->entry,
> @@ -79,14 +86,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_ignored)
> +			writer->ops->get_ignored(ignored_bits);
>   		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) &&
> +				ignored_bits, test_writer->init_entry,
> +				NUM_ENTRY_QWORDS) &&
>   				arm_smmu_entry_differs_in_used_bits(
>   					test_writer->entry, entry_used_bits,
> -					test_writer->target_entry,
> +					ignored_bits, test_writer->target_entry,
>   					NUM_ENTRY_QWORDS));
>   	}
>   }
> @@ -106,6 +116,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_ignored = arm_smmu_get_ste_ignored,
>   };
>   
>   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..95a4cfc5882d 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_ignored(__le64 *ignored_bits)
> +{
> +}
> +EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_ignored);
> +
>   /*
>    * 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,11 +1100,14 @@ 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 ignored[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_ignored)
> +		writer->ops->get_ignored(ignored);
>   
>   	for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
>   		/*
> @@ -1106,16 +1115,17 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
>   		 * allowed to set a bit to 1 if the used function doesn't say it
>   		 * is used.
>   		 */

Hi, Jason,

Instead of modifying the calculation logic, would it be cleaner to keep
the original arm_smmu_get_ste_used() semantics intact and simply exclude
ignored bits from cur_used after obtaining them?

For example, simply exclude the ignored bits from cur_used after
obtaining it:

         cur_used[i] &= ~ignored[i];

> -		WARN_ON_ONCE(target[i] & ~target_used[i]);
> +		WARN_ON_ONCE(target[i] & ~target_used[i] & ~ignored[i]);

Keep arm_smmu_get_ste_used() unchanged so target_used still accurately
reflects the bits actually used by hardware, then we do not need above
changes.


>   
>   		/* Bits can change because they are not currently being used */
> -		unused_update[i] = (entry[i] & cur_used[i]) |
> +		unused_update[i] = (entry[i] & (cur_used[i] | ignored[i])) |
>   				   (target[i] & ~cur_used[i]);
>   		/*
>   		 * Each bit indicates that a used bit in a qword needs to be
>   		 * changed after unused_update is applied.
>   		 */
> -		if ((unused_update[i] & target_used[i]) != target[i])
> +		if ((unused_update[i] & target_used[i]) !=
> +		    (target[i] & ~ignored[i]))
>   			used_qword_diff |= 1 << i;

And since `ignored` bits are cleared from cur_used, applying
unused_update to target will result in target itself. So we don't need
the above changes either.

The key insight is that we only need to exclude ignored bits from
cur_used (which determines what can be updated non-disruptively), while
keeping target_used intact for both the safety check and the final
comparison.

This way, we achieve the same goal with minimal changes to the existing
logic.

Thanks.
Shuai

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

[Jason Gunthorpe]

On Fri, Dec 05, 2025 at 04:52:00PM -0800, Nicolin Chen wrote:
> @@ -1106,16 +1115,17 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
>  		 * allowed to set a bit to 1 if the used function doesn't say it
>  		 * is used.
>  		 */
> -		WARN_ON_ONCE(target[i] & ~target_used[i]);
> +		WARN_ON_ONCE(target[i] & ~target_used[i] & ~ignored[i]);
>  
>  		/* Bits can change because they are not currently being used */
> -		unused_update[i] = (entry[i] & cur_used[i]) |
> +		unused_update[i] = (entry[i] & (cur_used[i] | ignored[i])) |
>  				   (target[i] & ~cur_used[i]);

This can't be right? We don't want to ever copy an ignored bit from
entry, the ignored bits should always come from target. The line
should be left alone.

Jason

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

[Jason Gunthorpe]

On Sat, Dec 06, 2025 at 10:19:08PM +0800, Shuai Xue wrote:
> Instead of modifying the calculation logic, would it be cleaner to keep
> the original arm_smmu_get_ste_used() semantics intact and simply exclude
> ignored bits from cur_used after obtaining them?

I'm OK with this idea

Jason

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

[Jason Gunthorpe]

On Sat, Dec 06, 2025 at 08:34:09PM +0800, Shuai Xue wrote:
> 
> +	arm_smmu_test_make_s2_ste(&s2_ste, 0);
> +	arm_smmu_v3_test_ste_expect_hitless_transition(test, &s1_ste, &s2_ste,
> +						       NUM_EXPECTED_SYNCS(3));
> 
> With get_ignored(), a nested s1dssbypass STE to a nested s1bypass STE
> will be hitless, a.k.a, NUM_EXPECTED_SYNCS(1).

hitless is tested by the ste_expect_hitless in the function name. The
expected SYNCS have to do with how many updates are required to fix
the STE, 3 is still fine for a hitless update. One to set the unused
bits, one to set the critical qword, one to clear unused bits.

Jason

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

[Nicolin Chen]

On Sat, Dec 06, 2025 at 03:34:08PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 05, 2025 at 04:52:00PM -0800, Nicolin Chen wrote:
> > @@ -1106,16 +1115,17 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
> >  		 * allowed to set a bit to 1 if the used function doesn't say it
> >  		 * is used.
> >  		 */
> > -		WARN_ON_ONCE(target[i] & ~target_used[i]);
> > +		WARN_ON_ONCE(target[i] & ~target_used[i] & ~ignored[i]);
> >  
> >  		/* Bits can change because they are not currently being used */
> > -		unused_update[i] = (entry[i] & cur_used[i]) |
> > +		unused_update[i] = (entry[i] & (cur_used[i] | ignored[i])) |
> >  				   (target[i] & ~cur_used[i]);
> 
> This can't be right? We don't want to ever copy an ignored bit from
> entry, the ignored bits should always come from target. The line
> should be left alone.

Hmm, without this change, the following coverages will be broken:
  arm_smmu_v3_write_ste_test_s1dssbypass_to_stebypass
  arm_smmu_v3_write_ste_test_stebypass_to_s1dssbypass

Both were expect num_syncs=2, but it would be 3 if we don't include
the ignored bits to unused_update. Or should we update the num_syncs
instead?

Nicolin

Re: [PATCH rc v1 3/4] iommu/arm-smmu-v3: Ignore STE EATS when computing the update sequence

[Jason Gunthorpe]

On Fri, Dec 05, 2025 at 04:52:02PM -0800, Nicolin Chen wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> If a VM want to toggle EATS off 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.

"EATS off at the same time as changing CFG"

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

Jason

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

[Nicolin Chen]

On Sat, Dec 06, 2025 at 08:34:09PM +0800, Shuai Xue wrote:
> 
> 
> 在 2025/12/6 08:52, 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.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 32 +++++++++++++++++++
> >   1 file changed, 32 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 9287904c93a2..56bdcf5a517e 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
> > @@ -553,6 +553,36 @@ 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_s2_ste(&s1_ste, ARM_SMMU_MASTER_TEST_ATS);
> > +	arm_smmu_test_make_cdtable_ste(&s1_ste, STRTAB_STE_1_S1DSS_SSID0,
> 
> arm_smmu_test_make_s2_ste() makes a s2 ste and it will be overwrited by
> arm_smmu_test_make_cdtable_ste(). Finnaly, we got a s1 STE, not a nested
> s1dssbypass ste.

You are right. There is memset.

> I think we need a function like arm_smmu_make_nested_cd_table_ste()
> here.
> 
> Besides, from the function name, I think you mean
> STRTAB_STE_1_S1DSS_BYPASS?

Yes, I will fix these.

Thanks
Nicolin

Re: [PATCH rc v1 3/4] iommu/arm-smmu-v3: Ignore STE EATS when computing the update sequence

[Nicolin Chen]

On Sat, Dec 06, 2025 at 03:46:37PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 05, 2025 at 04:52:02PM -0800, Nicolin Chen wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > If a VM want to toggle EATS off 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.
> 
> "EATS off at the same time as changing CFG"
> 
> In bare metal EATS is ignored by CFG=ABORT/BYPASS which is why this
> doesn't cause a problem until we have nested where CFG is always a
> variation of S2 trans that does use EATS.

Ack. Will add squash these two.

Nicolin

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

[Jason Gunthorpe]

On Sat, Dec 06, 2025 at 11:45:40AM -0800, Nicolin Chen wrote:
> On Sat, Dec 06, 2025 at 03:34:08PM -0400, Jason Gunthorpe wrote:
> > On Fri, Dec 05, 2025 at 04:52:00PM -0800, Nicolin Chen wrote:
> > > @@ -1106,16 +1115,17 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
> > >  		 * allowed to set a bit to 1 if the used function doesn't say it
> > >  		 * is used.
> > >  		 */
> > > -		WARN_ON_ONCE(target[i] & ~target_used[i]);
> > > +		WARN_ON_ONCE(target[i] & ~target_used[i] & ~ignored[i]);
> > >  
> > >  		/* Bits can change because they are not currently being used */
> > > -		unused_update[i] = (entry[i] & cur_used[i]) |
> > > +		unused_update[i] = (entry[i] & (cur_used[i] | ignored[i])) |
> > >  				   (target[i] & ~cur_used[i]);
> > 
> > This can't be right? We don't want to ever copy an ignored bit from
> > entry, the ignored bits should always come from target. The line
> > should be left alone.
> 
> Hmm, without this change, the following coverages will be broken:
>   arm_smmu_v3_write_ste_test_s1dssbypass_to_stebypass
>   arm_smmu_v3_write_ste_test_stebypass_to_s1dssbypass
> 
> Both were expect num_syncs=2, but it would be 3 if we don't include
> the ignored bits to unused_update. Or should we update the num_syncs
> instead?

Hmm!

I think that supports more that we should do what Shuai suggested and
keep used as-is.

Then ignored should be adjusted by the used: Only if both used are 1
should the bit become ignored. Otherwise we can rely on which ever
used is 0 to generate the hitless update.

That will preserve the tests and minimize the cases where we rely on
the ignored exception - though the reasoning for ignore is sound and
the two tests could be updated just fine to num_syncs=2 as well.

Like this?

--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1094,13 +1094,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 ignored[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_ignored)
+               writer->ops->get_ignored(ignored);
 
        for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
+               /*
+                * Ignored is only used for bits that are used by both entries,
+                * otherwise it is sequenced according to the unused entry.
+                */
+               ignored[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 +1118,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] &= ~ignored[i];
                unused_update[i] = (entry[i] & cur_used[i]) |
                                   (target[i] & ~cur_used[i]);
                /*

Jason

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

[Nicolin Chen]

On Sat, Dec 06, 2025 at 03:57:52PM -0400, Jason Gunthorpe wrote:
> I think that supports more that we should do what Shuai suggested and
> keep used as-is.

Yes, that will be probably cleaner.

> Then ignored should be adjusted by the used: Only if both used are 1
> should the bit become ignored. Otherwise we can rely on which ever
> used is 0 to generate the hitless update.

Hmm, not sure why it has to be both used.

The unused_update is computed using cur_used, and the equation for
used_qword_diff is computed using target_used, either of which can
be affected by ignored bits, right?

E.g.
if cur_used[] includes ING bit, target_used doesn't:
    // target must unset IGN bit, last equation isn't affected
    if cur sets IGN bit
        cur_used should exclude IGN bit
    if cur unsets IGN bit
        not affected
if cur_used[] doesn't include ignores, target_used does:
    // cur must unset IGN bit, cur_used isn't affected
    if target sets IGN bit:
        last equation must exclude IGN bit on both sides
    if target unsets IGN bit:
        not affected

> @@ -1109,6 +1118,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] &= ~ignored[i];
>                 unused_update[i] = (entry[i] & cur_used[i]) |
>                                    (target[i] & ~cur_used[i]);

If one of ignored bits is set in entry[i] but unset in target[i],
the unused_update will first mask it away, resulting in an extra
unnecessary update (though it's still hitless).

So, I think this might be better:

-		cur_used[i] &= ~ignored[i];
+		cur_unused[i] = ~cur_used[i] | ignored[i];
 		unused_update[i] = (entry[i] & cur_used[i]) |
-				   (target[i] & ~cur_used[i]);
+				   (target[i] & cur_unused[i]);

Because cur_used includes ignored, the unused_update will retain
the ignored bits from entry. On the other hand, having cur_unused
will also retain the ignored bits from target.

One more change that we need is at the last equation:
-		if ((unused_update[i] & target_used[i]) != target[i])
+		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
+		    (target[i] & ~ignored[i]))

Either side might have the ignored bits, so we have to suppress
ignored on both sides, which is required in the similar routine
in arm_smmu_entry_differs_in_used_bits() of the kunit code.

With these additional changes, nesting sanity and kunit test are
both passing. I will do a few more tests to make sure things are
okay, before wrapping up the v2. Please let me know if all these
make sense to you.

Thanks
Nicolin

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

[Jason Gunthorpe]

On Sat, Dec 06, 2025 at 08:37:30PM -0800, Nicolin Chen wrote:
> > Then ignored should be adjusted by the used: Only if both used are 1
> > should the bit become ignored. Otherwise we can rely on which ever
> > used is 0 to generate the hitless update.
> 
> Hmm, not sure why it has to be both used.

Thats the only case that causes an issue, if only one is used then
there is no need to perform a breaking update.

If cur_used is 0 then the bit will be set in the first update, if
target_used is 0 then the bit will be set during the last update.

> >                 /* Bits can change because they are not currently being used */
> > +               cur_used[i] &= ~ignored[i];
> >                 unused_update[i] = (entry[i] & cur_used[i]) |
> >                                    (target[i] & ~cur_used[i]);
> 
> If one of ignored bits is set in entry[i] but unset in target[i],
> the unused_update will first mask it away, resulting in an extra
> unnecessary update (though it's still hitless).

Yes, this is how it has always worked. The point is to leave the
existing the same not try to optimize it using ignored.

> One more change that we need is at the last equation:
> -		if ((unused_update[i] & target_used[i]) != target[i])
> +		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
> +		    (target[i] & ~ignored[i]))
> 
> Either side might have the ignored bits, so we have to suppress
> ignored on both sides, which is required in the similar routine
> in arm_smmu_entry_differs_in_used_bits() of the kunit code.

The only way ignored is set is if both sides have it set and then we
update the bit in the firsy cycle meaning unused_update must have the
final value. There is no need to mask target since it will match. Not
changing this line is a big part of what makes this appealing because
it keeps the logic straightforward, in case ignored is used we shift
the update always to the first cycle then everything else is the same.

Jason

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

[Nicolin Chen]

On Sun, Dec 07, 2025 at 12:09:10PM -0400, Jason Gunthorpe wrote:
> On Sat, Dec 06, 2025 at 08:37:30PM -0800, Nicolin Chen wrote:
> > > Then ignored should be adjusted by the used: Only if both used are 1
> > > should the bit become ignored. Otherwise we can rely on which ever
> > > used is 0 to generate the hitless update.
> > 
> > Hmm, not sure why it has to be both used.
> 
> Thats the only case that causes an issue, if only one is used then
> there is no need to perform a breaking update.
> 
> If cur_used is 0 then the bit will be set in the first update, if
> target_used is 0 then the bit will be set during the last update.

Ah, I see.

> > >                 /* Bits can change because they are not currently being used */
> > > +               cur_used[i] &= ~ignored[i];
> > >                 unused_update[i] = (entry[i] & cur_used[i]) |
> > >                                    (target[i] & ~cur_used[i]);
> > 
> > If one of ignored bits is set in entry[i] but unset in target[i],
> > the unused_update will first mask it away, resulting in an extra
> > unnecessary update (though it's still hitless).
> 
> Yes, this is how it has always worked. The point is to leave the
> existing the same not try to optimize it using ignored.

OK. Let's leave it and ask the test case to expect 3 v.s. 2.

> > One more change that we need is at the last equation:
> > -		if ((unused_update[i] & target_used[i]) != target[i])
> > +		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
> > +		    (target[i] & ~ignored[i]))
> > 
> > Either side might have the ignored bits, so we have to suppress
> > ignored on both sides, which is required in the similar routine
> > in arm_smmu_entry_differs_in_used_bits() of the kunit code.
> 
> The only way ignored is set is if both sides have it set and then we
> update the bit in the firsy cycle meaning unused_update must have the
> final value. There is no need to mask target since it will match. Not
> changing this line is a big part of what makes this appealing because
> it keeps the logic straightforward, in case ignored is used we shift
> the update always to the first cycle then everything else is the same.

The reason that I changed this is because the kunit tests failed
in arm_smmu_entry_differs_in_used_bits() when running the nested
cases:

STE initial value: 
    f800f0f0f0f0f0ef 00001000180800d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE used bits: 
    f80fffffffffffff 00003000fa0800ff
    065fffff0000ffff 000ffffffffffff0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE target value: 
    000000000000000d 0000100000000000
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE used bits: 
    000000000000000f 0000300032080000
    065fffff0000ffff 000ffffffffffff0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE value is now set to: 
    f800f0f0f0f0f0ef 00001000080000d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    # arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass: EXPECTATION FAILED at drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c:92
                   Expected arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->init_entry, ignored, 8) && arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->target_entry, ignored, 8) to be false, but is true
STE value is now set to: 
    000000000000000d 00001000080000d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE value is now set to: 
    000000000000000d 0000100000000000
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    not ok 21 arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass

STE initial value: 
    000000000000000d 0000100000000000
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE used bits: 
    000000000000000f 0000300032080000
    065fffff0000ffff 000ffffffffffff0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE target value: 
    f800f0f0f0f0f0ef 00001000180800d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE used bits: 
    f80fffffffffffff 00003000fa0800ff
    065fffff0000ffff 000ffffffffffff0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
STE value is now set to: 
    000000000000000d 00001000180800d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    # arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass: EXPECTATION FAILED at drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c:92
                   Expected arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->init_entry, ignored, 8) && arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->target_entry, ignored, 8) to be false, but is true
STE value is now set to: 
    f800f0f0f0f0f0ef 00001000180800d5
    0449b6c400000000 000dbeefdeadbee0
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    not ok 22 arm_smmu_v3_write_ste_test_nested_s1bypass_to_s1dssbypass

Any thought?

Thanks
Nicolin

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

[Nicolin Chen]

On Sun, Dec 07, 2025 at 11:35:06AM -0800, Nicolin Chen wrote:
> On Sun, Dec 07, 2025 at 12:09:10PM -0400, Jason Gunthorpe wrote:
> > On Sat, Dec 06, 2025 at 08:37:30PM -0800, Nicolin Chen wrote:
> > > One more change that we need is at the last equation:
> > > -		if ((unused_update[i] & target_used[i]) != target[i])
> > > +		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
> > > +		    (target[i] & ~ignored[i]))
> > > 
> > > Either side might have the ignored bits, so we have to suppress
> > > ignored on both sides, which is required in the similar routine
> > > in arm_smmu_entry_differs_in_used_bits() of the kunit code.
> > 
> > The only way ignored is set is if both sides have it set and then we
> > update the bit in the firsy cycle meaning unused_update must have the
> > final value. There is no need to mask target since it will match. Not
> > changing this line is a big part of what makes this appealing because
> > it keeps the logic straightforward, in case ignored is used we shift
> > the update always to the first cycle then everything else is the same.
> 
> The reason that I changed this is because the kunit tests failed
> in arm_smmu_entry_differs_in_used_bits() when running the nested
> cases:
...
>     # arm_smmu_v3_write_ste_test_nested_s1dssbypass_to_s1bypass: EXPECTATION FAILED at drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c:92
>                    Expected arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->init_entry, ignored, 8) && arm_smmu_entry_differs_in_used_bits( test_writer->entry, entry_used_bits, test_writer->target_entry, ignored, 8) to be false, but is true

I think I figured it out. The driver one has included ignored bits
from target in unused_update[]. But the kunit one is comparing the
raw value.

So, we don't need to change the driver one as you remarked, but we
do need to mask the target[] in the kunit one:

@@ -46,7 +46,9 @@ static bool arm_smmu_entry_differs_in_used_bits(const __le64 *entry,
        unsigned int i;

        for (i = 0; i < length; i++) {
-               if ((entry[i] & used_bits[i]) != target[i])
+               __le64 used = used_bits[i] & ~ignored[i];
+
+               if ((entry[i] & used) != (target[i] & used))
                        differs = true;
        }
        return differs;

I will wrap up v2 and send it today.

Thanks
Nicolin