Re: [PATCH 1/8] iommu: Lift and generalize the STE/CD update code from SMMUv3

[Baolu Lu <baolu.lu@linux.intel.com>]

On 3/13/26 13:39, Nicolin Chen wrote:
> Hi Baolu,

Hi Nicolin,

Thanks for the comments.

> 
> On Mon, Mar 09, 2026 at 02:06:41PM +0800, Lu Baolu wrote:
>> +struct entry_sync_writer_ops64;
>> +struct entry_sync_writer64 {
>> +	const struct entry_sync_writer_ops64 *ops;
>> +	size_t num_quantas;
>> +	size_t vbit_quanta;
>> +};
> 
> Though I could guess what the @num_quantas and @vbit_quanta likely
> mean, it'd be nicer to have some notes elaborating them.

Yes. I will make it like this,

struct entry_sync_writer64 {
         const struct entry_sync_writer_ops64 *ops;
         /* Total size of the entry in atomic units: */
         size_t num_quantas;
         /* The index of the quanta containing the Valid bit: */
         size_t vbit_quanta;
};

The same to entry_sync_writer128.

> 
>> +/*
>> + * Figure out if we can do a hitless update of entry to become target. Returns a
>> + * bit mask where 1 indicates that a quanta word needs to be set disruptively.
>> + * unused_update is an intermediate value of entry that has unused bits set to
>> + * their new values.
>> + */
>> +static u8 NS(entry_quanta_diff)(struct entry_sync_writer *writer,
>> +				const quanta_t *entry, const quanta_t *target,
>> +				quanta_t *unused_update, quanta_t *memory)
>> +{
>> +	quanta_t *target_used = memory + writer->num_quantas * 1;
>> +	quanta_t *cur_used = memory + writer->num_quantas * 2;
> 
> Should we have a kdoc somewhere mentioning that the two arrays are
> neighbors (IIUIC)?

The library uses a single block of scratchpad memory and offsets into 
it. A WARN_ON() is added in NS(entry_sync_write) to ensure this:

         if (WARN_ON(memory_len !=
                     ENTRY_SYNC_MEMORY_LEN(writer) * sizeof(*memory)))
                 return;

How about adding below comments around this WARN_ON()?

/*
  * The scratchpad memory is organized into three neighbors:
  * 1. [0, num_quantas): 'unused_update' - intermediate state with
  *    ignored bits updated.
  * 2. [num_quantas, 2*num_quantas): 'target_used' - bits active in
  *    the target state.
  * 3. [2*num_quantas, 3*num_quantas): 'cur_used' - bits active in
  *    the current state.
  */

>> +	u8 used_qword_diff = 0;
> 
> It seems to me that we want use "quanta" v.s. "qword"? 128 bits can
> be called "dqword" as well though.

Yes. "qword" is a bit too x86-centric. Since the library is designed
around the concept of an atomic "quanta" of update, I will unify the
terminology ("quanta" in general) and use used_quanta_diff

> 
>> +	unsigned int i;
>> +
>> +	writer->ops->get_used(entry, cur_used);
>> +	writer->ops->get_used(target, target_used);
> 
> SMMU has get_update_safe now. Can we take it together?

I will look into the SMMUv3 get_update_safe implementation. Or integrate
that specially when we transition the ARM SMMUv3 driver to use this
generic entry_sync library.

> 
>> +void NS(entry_sync_write)(struct entry_sync_writer *writer, quanta_t *entry,
>> +			  const quanta_t *target, quanta_t *memory,
>> +			  size_t memory_len)
>> +{
>> +	quanta_t *unused_update = memory + writer->num_quantas * 0;
>> +	u8 used_qword_diff;
>> +
>> +	if (WARN_ON(memory_len !=
>> +		    ENTRY_SYNC_MEMORY_LEN(writer) * sizeof(*memory)))
>> +		return;
>> +
>> +	used_qword_diff = NS(entry_quanta_diff)(writer, entry, target,
>> +						unused_update, memory);
>> +	if (hweight8(used_qword_diff) == 1) {
>> +		/*
>> +		 * Only one quanta needs its used bits to be changed. This is a
>> +		 * hitless update, update all bits the current entry is ignoring
>> +		 * to their new values, then update a single "critical quanta"
>> +		 * to change the entry and finally 0 out any bits that are now
>> +		 * unused in the target configuration.
>> +		 */
>> +		unsigned int critical_qword_index = ffs(used_qword_diff) - 1;
>> +
>> +		/*
>> +		 * Skip writing unused bits in the critical quanta since we'll
>> +		 * be writing it in the next step anyways. This can save a sync
>> +		 * when the only change is in that quanta.
>> +		 */
>> +		unused_update[critical_qword_index] =
>> +			entry[critical_qword_index];
>> +		NS(entry_set)(writer, entry, unused_update, 0,
>> +			      writer->num_quantas);
>> +		NS(entry_set)(writer, entry, target, critical_qword_index, 1);
>> +		NS(entry_set)(writer, entry, target, 0, writer->num_quantas);
>> +	} else if (used_qword_diff) {
>> +		/*
>> +		 * At least two quantas need their inuse bits to be changed.
>> +		 * This requires a breaking update, zero the V bit, write all
>> +		 * qwords but 0, then set qword 0
>> +		 */
> 
> Still, it'd be nicer to unify the wording between "quanta" and
> "qword".

Yes.

> 
> [..]
>> +EXPORT_SYMBOL(NS(entry_sync_write));
> 
> There is also a KUNIT test coverage in arm-smmu-v3 for all of these
> functions. Maybe we can make that generic as well?

Same here.

> 
>> +#define entry_sync_writer entry_sync_writer64
>> +#define quanta_t __le64
> [..]
>> +#define entry_sync_writer entry_sync_writer128
>> +#define quanta_t u128
> 
> u64 can be called 64 too, though we might not have use case for now.
> 
> But maybe we could just call them:
>      entry_sync_writer_le64
>      entry_sync_writer_u128
> ?
I'm fine with the new naming. It is more explicit. I will update the
names unless there are further objections.

Thanks,
baolu