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

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Mostafa,

On Sun, Dec 14, 2025 at 10:32:35PM +0000, Mostafa Saleh wrote:
> On Tue, Dec 09, 2025 at 06:45:16PM -0800, Nicolin Chen wrote:
> > @@ -1207,12 +1223,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);
> 
> After this change, no other caller uses the entry_set() return value, so it
> can be changed to return void.

OK.

> >  	}
> >  }
> >  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_write_entry);
> > @@ -1543,6 +1556,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,
> >  };
> >  
> 
> I have some mixed feelings about this, having get_used(), then get_ignored()
> with the same bits set seems confusing to me, specially the get_ignored()
> loops back to update cur_used, which is set from get_used()
> 
> My initial though was just to remove this bit from get_used() + some changes
> to checks setting bits that are not used would be enough, and the semantics
> of get_used() can be something as:
> “Return bits used by the updated translation regime that MUST be observed
> atomically” and in that case we can ignore things as MEV as it doesn’t
> impact the translation.
> 
> However, this approach makes it a bit explicit which bits are ignored, if we
> keep this logic, I think changing the name of get_ignored() might help, to
> something as "get_allowed_break()" or "get_update_safe()"?

I think "ignored" itself is brief and understandable.. Instead,
perhaps we can add a kdocs to make it clearer:

/**
 * struct arm_smmu_entry_writer_ops - STE/CD entry writer operations
 * @get_used: Output to @used the bits used by the hardware corresponding to the
 *            configurations bits set in a given @entry
 * @get_ignored: Output to @ignored the bits that are listed in the "used" list
 *               but allowed to be ignored by arm_smmu_entry_qword_diff(). Each
 *               field (bits) must provide a reason to justify that the entries
 *               can be updated safely without breaking STE/CD configurations.
 * @sync: Operation to synchronize the updated STE/CD entries in the memory
 */
struct arm_smmu_entry_writer_ops {
	void (*get_used)(const __le64 *entry, __le64 *used);
	void (*get_ignored)(__le64 *ignored);
	void (*sync)(struct arm_smmu_entry_writer *writer);
};

?

Thanks
Nicolin