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

Nicolin Chen
2026-03-13T05:40:08Z

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