Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces

[Baolu Lu]

On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > > Assuming we leave room for multi-device groups this logic should just
> > > be
> > >
> > > 	group = iommu_group_get(dev);
> > > 	if (!group)
> > > 		return -ENODEV;
> > >
> > > 	mutex_lock(&group->mutex);
> > > 	domain = xa_load(&group->pasid_array, mm->pasid);
> > > 	if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
> > > 		domain = iommu_sva_alloc_domain(dev, mm);
> > >
> > > ?
> > Agreed. As a helper in iommu core, how about making it more generic like
> > below?
> IDK, is there more users of this? AFAIK SVA is the only place that
> will be auto-sharing?

The generic thing is that components, like SVA, want to fetch the
attached domain from the iommu core.

> > +       mutex_lock(&group->mutex);
> > +       domain = xa_load(&group->pasid_array, pasid);
> > +       if (domain && domain->type != type)
> > +               domain = NULL;
> > +       mutex_unlock(&group->mutex);
> > +       iommu_group_put(group);
> > +
> > +       return domain;
> This is bad locking, group->pasid_array values cannot be taken outside
> the lock.

It's not iommu core, but SVA (or other feature components) that manage
the life cycle of a domain. The iommu core only provides a place to
store the domain pointer. The feature components are free to fetch their
domain pointers from iommu core as long as they are sure that the domain
is alive during use.

> > > And stick the refcount in the sva_domain
> > >
> > > Also, given the current arrangement it might make sense to have a
> > > struct iommu_domain_sva given that no driver is wrappering this in
> > > something else.
> > Fair enough. How about below wrapper?
> >
> > +struct iommu_sva_domain {
> > +       /*
> > +        * Common iommu domain header,*must*  be put at the top
> > +        * of the structure.
> > +        */
> > +       struct iommu_domain domain;
> > +       struct mm_struct *mm;
> > +       struct iommu_sva bond;
> > +}
> >
> > The refcount is wrapped in bond.
> I'm still not sure that bond is necessary

"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().

> But yes, something like that

Best regards,
baolu