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

Nicolin Chen
2023-01-30T10:22:41Z

On Sun, Jan 29, 2023 at 02:38:55AM -0800, Nicolin Chen wrote:

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

Wondering if we can let iommu_attach/detach_device handle this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0d7c2177ad6..b38f71e92e2a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -57,6 +57,7 @@ struct iommu_group {
struct iommu_domain *domain;
struct list_head entry;
unsigned int owner_cnt;
+ unsigned int attached_cnt;
void *owner;
};

@@ -64,6 +65,7 @@ struct group_device {
struct list_head list;
struct device *dev;
char *name;
+ bool attached;
};

struct iommu_group_attribute {
@@ -2035,6 +2037,7 @@ static int __iommu_attach_device(struct iommu_domain *domain,
*/
int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
{
+ struct group_device *grp_dev;
struct iommu_group *group;
int ret;

@@ -2042,16 +2045,22 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
if (!group)
return -ENODEV;

- /*
- * Lock the group to make sure the device-count doesn't
- * change while we are attaching
- */
mutex_lock(&group->mutex);
- ret = -EINVAL;
- if (iommu_group_device_count(group) != 1)
+
+ list_for_each_entry(grp_dev, &group->devices, list)
+ if (grp_dev->dev == dev)
+ break;
+ if (grp_dev->attached)
goto out_unlock;

- ret = __iommu_attach_group(domain, group);
+ /* Attach the group when attaching the first device in the group */
+ if (group->attached_cnt == 0) {
+ ret = __iommu_attach_group(domain, group);
+ if (ret)
+ goto out_unlock;
+ }
+ grp_dev->attached = true;
+ group->attached_cnt++;

out_unlock:
mutex_unlock(&group->mutex);
@@ -2071,6 +2080,7 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)

void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
{
+ struct group_device *grp_dev;
struct iommu_group *group;

group = iommu_group_get(dev);
@@ -2078,10 +2088,20 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
return;

mutex_lock(&group->mutex);
- if (WARN_ON(domain != group->domain) ||
- WARN_ON(iommu_group_device_count(group) != 1))
+ if (WARN_ON(domain != group->domain))
goto out_unlock;
- __iommu_group_set_core_domain(group);
+
+ list_for_each_entry(grp_dev, &group->devices, list)
+ if (grp_dev->dev == dev)
+ break;
+ if (!grp_dev->attached)
+ goto out_unlock;
+
+ grp_dev->attached = false;
+ group->attached_cnt--;
+ /* Detach the group when detaching the last device in the group */
+ if (group->attached_cnt == 0)
+ __iommu_group_set_core_domain(group);

out_unlock:
mutex_unlock(&group->mutex);