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

Nicolin Chen
2025-08-27T00:49:30Z

On Tue, Aug 26, 2025 at 04:50:03PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 13, 2025 at 06:25:35PM -0700, Nicolin Chen wrote:
> > +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.

In the other patch, you noted that it's likely very rare to have
an ATS-supported device with multiple SIDs. Also given that this
function is called per device. So, 64 should be enough?

With that being said...

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

... I do like this reuse flag. I will give it a try.

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

I did so until my last local pre-v1 version as I found it seems
cleaner to mark it using the todel. I'll try again and see how
it goes.

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

Oh right. refcount == 0 can't increase...

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

OK.

Thanks
Nicolin