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

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Baolu,

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.

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

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

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

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

[..]
> +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?

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

Nicolin