diff mbox series

[v2,07/10] iommufd/device: Make hwpt_list list_add/del symmetric

Message ID 9d0d4f935c4972771f5aed4b4837d8ae35814e06.1675802050.git.nicolinc@nvidia.com (mailing list archive)
State New
Headers show
Series Add IO page table replacement support | expand

Commit Message

Nicolin Chen Feb. 7, 2023, 9:17 p.m. UTC 
Because list_del() is together with iopt_table_remove_domain(), it makes
sense to have list_add_tail() together with iopt_table_add_domain().

Also place the mutex outside the iommufd_device_do_attach() call, similar
to what's in the iommufd_device_auto_get_domain() function.

Co-developed-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Tian, Kevin Feb. 9, 2023, 3:23 a.m. UTC | #1 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, February 8, 2023 5:18 AM
> 
> Because list_del() is together with iopt_table_remove_domain(), it makes
> sense to have list_add_tail() together with iopt_table_add_domain().
> 
> Also place the mutex outside the iommufd_device_do_attach() call, similar
> to what's in the iommufd_device_auto_get_domain() function.
> 
> Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

shouldn't this be a separate bug fix and backported? double adding a
list item would certainly clobber the list...
Jason Gunthorpe Feb. 9, 2023, 1:24 p.m. UTC | #2 
On Thu, Feb 09, 2023 at 03:23:47AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Wednesday, February 8, 2023 5:18 AM
> > 
> > Because list_del() is together with iopt_table_remove_domain(), it makes
> > sense to have list_add_tail() together with iopt_table_add_domain().
> > 
> > Also place the mutex outside the iommufd_device_do_attach() call, similar
> > to what's in the iommufd_device_auto_get_domain() function.
> > 
> > Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> shouldn't this be a separate bug fix and backported? double adding a
> list item would certainly clobber the list...

AFAIK there is no bug, this is just reorganizing things

Jason
Tian, Kevin Feb. 10, 2023, 1:46 a.m. UTC | #3 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, February 9, 2023 9:24 PM
> 
> On Thu, Feb 09, 2023 at 03:23:47AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Wednesday, February 8, 2023 5:18 AM
> > >
> > > Because list_del() is together with iopt_table_remove_domain(), it makes
> > > sense to have list_add_tail() together with iopt_table_add_domain().
> > >
> > > Also place the mutex outside the iommufd_device_do_attach() call,
> similar
> > > to what's in the iommufd_device_auto_get_domain() function.
> > >
> > > Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> >
> > shouldn't this be a separate bug fix and backported? double adding a
> > list item would certainly clobber the list...
> 
> AFAIK there is no bug, this is just reorganizing things
> 

there is semantics change.

here is the current code:

	case IOMMUFD_OBJ_HW_PAGETABLE: {
		struct iommufd_hw_pagetable *hwpt =
			container_of(pt_obj, struct iommufd_hw_pagetable, obj);

		rc = iommufd_device_do_attach(idev, hwpt);
		if (rc)
			goto out_put_pt_obj;

		mutex_lock(&hwpt->ioas->mutex);
		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
		mutex_unlock(&hwpt->ioas->mutex);
		break;
	}

Above means every attach to hwpt will try to add the hwpt to the
list tail. Isn't it a bug?

with this patch the hwpt is added to the list only when it's attached
by the first device, i.e. when iopt_table_add_domain() is called.

	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
		rc = iommu_attach_group(hwpt->domain, idev->group);
		if (rc)
			goto out_iova;

		if (list_empty(&hwpt->devices)) {
			rc = iopt_table_add_domain(&hwpt->ioas->iopt,
						hwpt->domain);
			if (rc)
				goto out_detach;
			list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
		}
	}

so it's actually a bug fix.
Jason Gunthorpe Feb. 10, 2023, 9:17 p.m. UTC | #4 
On Fri, Feb 10, 2023 at 01:46:18AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, February 9, 2023 9:24 PM
> > 
> > On Thu, Feb 09, 2023 at 03:23:47AM +0000, Tian, Kevin wrote:
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Wednesday, February 8, 2023 5:18 AM
> > > >
> > > > Because list_del() is together with iopt_table_remove_domain(), it makes
> > > > sense to have list_add_tail() together with iopt_table_add_domain().
> > > >
> > > > Also place the mutex outside the iommufd_device_do_attach() call,
> > similar
> > > > to what's in the iommufd_device_auto_get_domain() function.
> > > >
> > > > Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > >
> > > shouldn't this be a separate bug fix and backported? double adding a
> > > list item would certainly clobber the list...
> > 
> > AFAIK there is no bug, this is just reorganizing things
> > 
> 
> there is semantics change.
> 
> here is the current code:
> 
> 	case IOMMUFD_OBJ_HW_PAGETABLE: {
> 		struct iommufd_hw_pagetable *hwpt =
> 			container_of(pt_obj, struct iommufd_hw_pagetable, obj);
> 
> 		rc = iommufd_device_do_attach(idev, hwpt);
> 		if (rc)
> 			goto out_put_pt_obj;
> 
> 		mutex_lock(&hwpt->ioas->mutex);
> 		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
> 		mutex_unlock(&hwpt->ioas->mutex);
> 		break;
> 	}
> 
> Above means every attach to hwpt will try to add the hwpt to the
> list tail. Isn't it a bug?

Yes, that looks like a bug..

But this patch isn't the right way to fix that.

The HWPT should be permanently linked to the IOAS as long as it
exists, and the linkage should happen when it is first created.

So attaching a HWPT to another device should never re-link it to the
ioas, thus delete these lines here.

However it looks like iommufd_device_detach() is technically wrong
too, it should only detach the IOAS and HWPT if it is going to destroy
the HWPT. We can't hit those kinds of bugs ATM because we cannot
create naked HWPTs that are not autodomains.

Maybe something like this.. I'll look closer next week

Jason

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index d81f93a321afcb..4e87a44533048a 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -279,7 +279,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	 */
 	mutex_lock(&ioas->mutex);
 	list_for_each_entry(hwpt, &ioas->hwpt_list, hwpt_item) {
-		if (!hwpt->auto_domain)
+		if (!hwpt->auto_domain || iommufd_object_alive(&hwpt->obj))
 			continue;
 
 		rc = iommufd_device_do_attach(idev, hwpt);
@@ -304,6 +304,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	rc = iommufd_device_do_attach(idev, hwpt);
 	if (rc)
 		goto out_abort;
+
 	list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
 
 	mutex_unlock(&ioas->mutex);
@@ -346,10 +347,6 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		rc = iommufd_device_do_attach(idev, hwpt);
 		if (rc)
 			goto out_put_pt_obj;
-
-		mutex_lock(&hwpt->ioas->mutex);
-		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
-		mutex_unlock(&hwpt->ioas->mutex);
 		break;
 	}
 	case IOMMUFD_OBJ_IOAS: {
@@ -390,14 +387,8 @@ void iommufd_device_detach(struct iommufd_device *idev)
 	mutex_lock(&hwpt->ioas->mutex);
 	mutex_lock(&hwpt->devices_lock);
 	list_del(&idev->devices_item);
-	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
-		if (list_empty(&hwpt->devices)) {
-			iopt_table_remove_domain(&hwpt->ioas->iopt,
-						 hwpt->domain);
-			list_del(&hwpt->hwpt_item);
-		}
+	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group))
 		iommu_detach_group(hwpt->domain, idev->group);
-	}
 	iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev);
 	mutex_unlock(&hwpt->devices_lock);
 	mutex_unlock(&hwpt->ioas->mutex);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 43d473989a0667..b11738bbdff7ec 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -13,6 +13,11 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)
 
 	WARN_ON(!list_empty(&hwpt->devices));
 
+	mutex_lock(&hwpt->ioas->mutex);
+	iopt_table_remove_domain(&hwpt->ioas->iopt, hwpt->domain);
+	list_del(&hwpt->hwpt_item);
+	mutex_unlock(&hwpt->ioas->mutex);
+
 	iommu_domain_free(hwpt->domain);
 	refcount_dec(&hwpt->ioas->obj.users);
 	mutex_destroy(&hwpt->devices_lock);
Tian, Kevin Feb. 13, 2023, 2:12 a.m. UTC | #5 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, February 11, 2023 5:17 AM
> 
> >
> > there is semantics change.
> >
> > here is the current code:
> >
> > 	case IOMMUFD_OBJ_HW_PAGETABLE: {
> > 		struct iommufd_hw_pagetable *hwpt =
> > 			container_of(pt_obj, struct iommufd_hw_pagetable,
> obj);
> >
> > 		rc = iommufd_device_do_attach(idev, hwpt);
> > 		if (rc)
> > 			goto out_put_pt_obj;
> >
> > 		mutex_lock(&hwpt->ioas->mutex);
> > 		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
> > 		mutex_unlock(&hwpt->ioas->mutex);
> > 		break;
> > 	}
> >
> > Above means every attach to hwpt will try to add the hwpt to the
> > list tail. Isn't it a bug?
> 
> Yes, that looks like a bug..
> 
> But this patch isn't the right way to fix that.
> 
> The HWPT should be permanently linked to the IOAS as long as it
> exists, and the linkage should happen when it is first created.
> 
> So attaching a HWPT to another device should never re-link it to the
> ioas, thus delete these lines here.
> 

yes, this makes more sense
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 10ce47484ffa..b8c3e3baccb5 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -200,6 +200,8 @@  static int iommufd_device_do_attach(struct iommufd_device *idev,
 	phys_addr_t sw_msi_start = PHYS_ADDR_MAX;
 	int rc;
 
+	lockdep_assert_held(&hwpt->ioas->mutex);
+
 	mutex_lock(&hwpt->devices_lock);
 
 	/*
@@ -243,6 +245,7 @@  static int iommufd_device_do_attach(struct iommufd_device *idev,
 						   hwpt->domain);
 			if (rc)
 				goto out_detach;
+			list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
 		}
 	}
 
@@ -304,7 +307,6 @@  static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	rc = iommufd_device_do_attach(idev, hwpt);
 	if (rc)
 		goto out_abort;
-	list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
 
 	mutex_unlock(&ioas->mutex);
 	iommufd_object_finalize(idev->ictx, &hwpt->obj);
@@ -343,13 +345,11 @@  int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		struct iommufd_hw_pagetable *hwpt =
 			container_of(pt_obj, struct iommufd_hw_pagetable, obj);
 
+		mutex_lock(&hwpt->ioas->mutex);
 		rc = iommufd_device_do_attach(idev, hwpt);
+		mutex_unlock(&hwpt->ioas->mutex);
 		if (rc)
 			goto out_put_pt_obj;
-
-		mutex_lock(&hwpt->ioas->mutex);
-		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
-		mutex_unlock(&hwpt->ioas->mutex);
 		break;
 	}
 	case IOMMUFD_OBJ_IOAS: {