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

Nicolin Chen
2023-01-29T10:39:09Z

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@nvidia.com>
> > 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