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

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:

quoted

On Fri, Dec 05, 2025 at 04:52:00PM -0800, Nicolin Chen wrote:

quoted

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