Re: [PATCH v2 3/3] iommufd/device: Change iommufd_hw_pagetable_has_group to device centric

[Nicolin Chen]

On Sun, Jan 29, 2023 at 09:37:00AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Sent: Sunday, January 29, 2023 5:18 AM
> >
> > -static bool iommufd_hw_pagetable_has_group(struct
> > iommufd_hw_pagetable *hwpt,
> > -                                        struct iommu_group *group)
> > +static bool iommufd_hw_pagetable_has_device(struct
> > iommufd_hw_pagetable *hwpt,
> > +                                         struct device *dev)
> >  {
> > -     struct iommufd_device *cur_dev;
> > -
> > -     list_for_each_entry(cur_dev, &hwpt->devices, devices_item)
> > -             if (cur_dev->group == group)
> > -                     return true;
> > -     return false;
> > +     /*
> > +      * iommu_get_domain_for_dev() returns an iommu_group->domain
> > ptr, if it
> > +      * is the same domain as the hwpt->domain, it means that this hwpt
> > has
> > +      * the iommu_group/device.
> > +      */
> > +     return hwpt->domain == iommu_get_domain_for_dev(dev);
> >  }
> 
> Here we could have three scenarios:
> 
> 1) the device is attached to blocked domain;
> 2) the device is attached to hwpt->domain;
> 3) the device is attached to another hwpt->domain;
> 
> if this function returns false then iommufd_device_do_attach() will attach
> the device to the specified hwpt. But then it's wrong for 3).
> 
> Has 3) been denied in earlier path? If yes at least a WARN_ON for
> case 3) makes sense here.

The case #3 means the device is already attached to some other
domain? Then vfio_iommufd_physical_attach_ioas returns -EBUSY
at the sanity of vdev->iommufd_attached. And the case #3 feels
like a domain replacement use case to me. So probably not that
necessary to add a wARN_ON?

> > @@ -385,10 +372,8 @@ void iommufd_device_detach(struct
> > iommufd_device *idev)
> >       struct iommufd_hw_pagetable *hwpt = idev->hwpt;
> >
> >       mutex_lock(&hwpt->ioas->mutex);
> > -     mutex_lock(&hwpt->devices_lock);
> >       refcount_dec(hwpt->devices_users);
> > -     list_del(&idev->devices_item);
> > -     if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
> > +     if (iommufd_hw_pagetable_has_device(hwpt, idev->dev)) {
> >               if (refcount_read(hwpt->devices_users) == 1) {
> >                       iopt_table_remove_domain(&hwpt->ioas->iopt,
> >                                                hwpt->domain);
> > @@ -397,7 +382,6 @@ void iommufd_device_detach(struct iommufd_device
> > *idev)
> >               iommu_detach_group(hwpt->domain, idev->group);
> >       }
> 
> emmm how do we track last device detach in a group? Here the first
> device detach already leads to group detach...

Oh no. That's a bug. Thanks for catching it.

We need an additional refcount somewhere to track the number of
attached devices in the iommu_group.

Nicolin