iommu/arm-smmu-v3: Fix hitless STE update in nesting cases

From: Jason Gunthorpe <[email protected]>

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 [email protected]

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
  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(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0

index ae23aacc3840..d5f0e5407b9f 100644
@@ -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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +14 -3

index d2671bfd3798..9287904c93a2 100644
@@ -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 = {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9 -0

index d16d35c78c06..95a4cfc5882d 100644
@@ -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++) {
  		/*

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?

Quoted 7 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1

@@ -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]);
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, · line 1122

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1

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

Quoted 8 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

(quoted diff) +1 -1

@@ -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]);
 

Quoted 8 lines

Review static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, · line 1122

Quoted 4 lines

(quoted diff) +1 -1

 		/* 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?

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.

Quoted 3 lines

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.

Quoted 4 lines

                /* 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).

One more change that we need is at the last equation:

(quoted diff) +2 -1

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

Quoted 4 lines · Nicolin Chen

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 5 lines

One more change that we need is at the last equation:

(quoted diff) +2 -1

-		if ((unused_update[i] & target_used[i]) != target[i])
+		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
+		    (target[i] & ~ignored[i]))

Quoted 3 lines

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.

Quoted 6 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

One more change that we need is at the last equation:

(quoted diff) +2 -1

-		if ((unused_update[i] & target_used[i]) != target[i])
+		if ((unused_update[i] & target_used[i] & ~ignored[i]) !=
+		    (target[i] & ~ignored[i]))

Quoted 10 lines

Quoted 3 lines

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

From: Jason Gunthorpe <[email protected]>

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.

Quoted 5 lines · Nicolin Chen

From: Jason Gunthorpe <[email protected]>

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.

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 <[email protected]>

---
  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 32 +++++++++++++++++++
  1 file changed, 32 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +0 -0

index 9287904c93a2..56bdcf5a517e 100644
@@ -553,6 +553,36 @@ static void arm_smmu_v3_write_ste_test_s2_to_s1_stall(struct kunit *test)
  						       NUM_EXPECTED_SYNCS(3));

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +3 -0

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

Quoted 22 lines · 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 <[email protected]>

---
  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  | 32 +++++++++++++++++++
  1 file changed, 32 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +4 -0

index 9287904c93a2..56bdcf5a517e 100644
@@ -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;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c static void arm_smmu_v3_write_ste_test_s2_to_s1_stall(struct kunit *test) · line 563

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c +4 -0

+	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?