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

[Nicolin Chen]

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);