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

Baolu Lu
2026-03-16T06:25:58Z

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