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

[Baolu Lu]

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