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.0On 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
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
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
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
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
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
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
在 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
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
© 2016 - 2025 Red Hat, Inc.