Re: [PATCH rfcv1 4/8] iommu/arm-smmu-v3: Introduce a per-domain arm_smmu_invs array

Jason Gunthorpe
2025-08-26T19:50:08Z

On Wed, Aug 13, 2025 at 06:25:35PM -0700, Nicolin Chen wrote:
> +/**
> + * arm_smmu_invs_add() - Combine @old_invs with @add_invs to a new array
> + * @old_invs: the old invalidation array
> + * @add_invs: an array of invlidations to add
> + *
> + * Return: a newly allocated and sorted invalidation array on success, or an
> + * ERR_PTR.
> + *
> + * This function must be locked and serialized with arm_smmu_invs_del/dec(),
> + * but do not lockdep on any lock for KUNIT test.
> + *
> + * Caller is resposible for freeing the @old_invs and the returned one.
> + *
> + * Entries marked as trash can be resued if @add_invs wants to add them back.
> + * Otherwise, they will be completely removed in the returned array.
> + */
> +VISIBLE_IF_KUNIT
> +struct arm_smmu_invs *arm_smmu_invs_add(struct arm_smmu_invs *old_invs,
> + struct arm_smmu_invs *add_invs)
> +{
> + size_t need = old_invs->num_invs + add_invs->num_invs;
> + struct arm_smmu_invs *new_invs;
> + size_t deletes = 0, i, j;
> + u64 existed = 0;
> +
> + /* Max of add_invs->num_invs is 64 */
> + if (WARN_ON(add_invs->num_invs > sizeof(existed) * 8))
> + return ERR_PTR(-EINVAL);

Since this is driven off of num_streams using a fixed bitmap doesn't
seem great since I suppose the dt isn't limited to 64.

Given how this is working now I think you can just add a new member to
the struct:

struct arm_smmu_inv {
/* invalidation items */
struct arm_smmu_device *smmu;
u8 type;
u8 size_opcode;
u8 nsize_opcode;
/* Temporary bits for add/del functions */
u8 reuse:1;
u8 todel:1;

And use reuse as the temporary instead of the bitmap.

> + for (i = 0; i != old_invs->num_invs; i++) {
> + struct arm_smmu_inv *cur = &old_invs->inv[i];

missing newline

> + /* Count the trash entries to deletes */
> + if (cur->todel) {
> + WARN_ON_ONCE(refcount_read(&cur->users));
> + deletes++;
> + }

Just do continue here.

todel should only be used as a temporary. Use refcount_read() ==
0. Then you don't need a WARN either.

> + for (j = 0; j != add_invs->num_invs; j++) {
> + if (!same_op(cur, &add_invs->inv[j]))
> + continue;
> + /* Found duplicated entries in add_invs */
> + if (WARN_ON_ONCE(existed & BIT_ULL(j)))
> + continue;

inv[j].reuse

> + /* Revert the todel marker for reuse */
> + if (cur->todel) {
> + cur->todel = false;
> + deletes--;

This wil blow up the refcount_inc() below because users is 0..
There is no point in trying to optimize like this just discard the
old entry and add a new one.

> + new_invs = arm_smmu_invs_alloc(need);
> + if (IS_ERR(new_invs)) {
> + /* Don't forget to revert all the todel markers */
> + for (i = 0; i != old_invs->num_invs; i++) {
> + if (refcount_read(&old_invs->inv[i].users) == 0)
> + old_invs->inv[i].todel = true;
> + }

Drop as well

> + return new_invs;
> + }
> +
> + /* Copy the entire array less all the todel entries */
> + for (i = 0; i != old_invs->num_invs; i++) {
> + if (old_invs->inv[i].todel)
> + continue;

refcount_read

> + new_invs->inv[new_invs->num_invs++] = old_invs->inv[i];
> + }
> +
> + for (j = 0; j != add_invs->num_invs; j++) {
> + if (existed & BIT_ULL(j)) {

inv[j]->reuse

> + unsigned int idx = add_invs->inv[j].id;

Similar remarks for del, use users to set todel, don't expect it to be
valid coming into the function.

Jason