Re: [PATCH] iommufd: Enforce IOMMU_RESV_SW_MSI upon hwpt_paging allocation

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Jul 31, 2024 at 11:13:11AM -0700, Nicolin Chen wrote:
> On Wed, Jul 31, 2024 at 07:45:46AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Monday, July 29, 2024 7:51 AM
> > > @@ -364,7 +305,8 @@ int iommufd_hw_pagetable_attach(struct
> > > iommufd_hw_pagetable *hwpt,
> > >       }
> > >
> > >       if (hwpt_is_paging(hwpt)) {
> > > -             rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt),
> > > idev);
> > > +             rc = iopt_table_enforce_dev_resv_regions(
> > > +                             &to_hwpt_paging(hwpt)->ioas->iopt, idev-
> > > >dev);
> > 
> > Is it simpler to extend the original operation to the parent S2 when
> > it's hwpt_nested?
> 
> Likely. I recall that was what one of our WIP versions did.
> 
> > The name iommufd_hwpt_paging_attach() is a bit misleading. The
> > actual work there is all about reservations. It doesn't change any
> > tracking structure about attachment between device and hwpt.
> 
> How about iommufd_hwpt_enforce/remove_rr() taking hwpt v.s.
> hwpt_paging.
 
> > With that I think continuing this per-device reservation scheme is
> > easier than adding specific reservation for SW_MSI at hwpt creation
> > time and then further requiring check at attach time to verify
> > the attached device is allocated with the same address as the one
> > during allocation.
> 
> Jason, do you agree?

I came up with something plus a bit of naming alignment:
    iommufd_device_attach_reserved_iova()
    iommufd_group_remove_reserved_iova()
    iommufd_group_do_replace_reserved_iova()

If it looks good to both of you, I will send a formal patch.

Thanks
Nic

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 76b7297d22b0f..f03218dc4861e 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -338,8 +338,9 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
 	return 0;
 }
 
-static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
-				      struct iommufd_device *idev)
+static int
+iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
+				    struct iommufd_hwpt_paging *hwpt_paging)
 {
 	int rc;
 
@@ -374,11 +375,9 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
 		goto err_unlock;
 	}
 
-	if (hwpt_is_paging(hwpt)) {
-		rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
-		if (rc)
-			goto err_unlock;
-	}
+	rc = iommufd_device_attach_reserved_iova(idev, to_hwpt_paging(hwpt));
+	if (rc)
+		goto err_unlock;
 
 	/*
 	 * Only attach to the group once for the first device that is in the
@@ -398,9 +397,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
 	mutex_unlock(&idev->igroup->lock);
 	return 0;
 err_unresv:
-	if (hwpt_is_paging(hwpt))
-		iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt,
-					  idev->dev);
+	iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt, idev->dev);
 err_unlock:
 	mutex_unlock(&idev->igroup->lock);
 	return rc;
@@ -417,9 +414,7 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev)
 		iommufd_hwpt_detach_device(hwpt, idev);
 		idev->igroup->hwpt = NULL;
 	}
-	if (hwpt_is_paging(hwpt))
-		iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt,
-					  idev->dev);
+	iopt_remove_reserved_iova(&to_hwpt_paging(hwpt)->ioas->iopt, idev->dev);
 	mutex_unlock(&idev->igroup->lock);
 
 	/* Caller must destroy hwpt */
@@ -451,8 +446,8 @@ iommufd_group_remove_reserved_iova(struct iommufd_group *igroup,
 }
 
 static int
-iommufd_group_do_replace_paging(struct iommufd_group *igroup,
-				struct iommufd_hwpt_paging *hwpt_paging)
+iommufd_group_do_replace_reserved_iova(struct iommufd_group *igroup,
+				       struct iommufd_hwpt_paging *hwpt_paging)
 {
 	struct iommufd_hw_pagetable *old_hwpt = igroup->hwpt;
 	struct iommufd_device *cur;
@@ -460,8 +455,7 @@ iommufd_group_do_replace_paging(struct iommufd_group *igroup,
 
 	lockdep_assert_held(&igroup->lock);
 
-	if (!hwpt_is_paging(old_hwpt) ||
-	    hwpt_paging->ioas != to_hwpt_paging(old_hwpt)->ioas) {
+	if (hwpt_paging->ioas != to_hwpt_paging(old_hwpt)->ioas) {
 		list_for_each_entry(cur, &igroup->device_list, group_item) {
 			rc = iopt_table_enforce_dev_resv_regions(
 				&hwpt_paging->ioas->iopt, cur->dev, NULL);
@@ -502,20 +496,15 @@ iommufd_device_do_replace(struct iommufd_device *idev,
 	}
 
 	old_hwpt = igroup->hwpt;
-	if (hwpt_is_paging(hwpt)) {
-		rc = iommufd_group_do_replace_paging(igroup,
-						     to_hwpt_paging(hwpt));
-		if (rc)
-			goto err_unlock;
-	}
+	rc = iommufd_group_do_replace_reserved_iova(igroup, to_hwpt_paging(hwpt));
+	if (rc)
+		goto err_unlock;
 
 	rc = iommufd_hwpt_replace_device(idev, hwpt, old_hwpt);
 	if (rc)
 		goto err_unresv;
 
-	if (hwpt_is_paging(old_hwpt) &&
-	    (!hwpt_is_paging(hwpt) ||
-	     to_hwpt_paging(hwpt)->ioas != to_hwpt_paging(old_hwpt)->ioas))
+	if (to_hwpt_paging(hwpt)->ioas != to_hwpt_paging(old_hwpt)->ioas)
 		iommufd_group_remove_reserved_iova(igroup,
 						   to_hwpt_paging(old_hwpt));
 
@@ -535,9 +524,7 @@ iommufd_device_do_replace(struct iommufd_device *idev,
 	/* Caller must destroy old_hwpt */
 	return old_hwpt;
 err_unresv:
-	if (hwpt_is_paging(hwpt))
-		iommufd_group_remove_reserved_iova(igroup,
-						   to_hwpt_paging(hwpt));
+	iommufd_group_remove_reserved_iova(igroup, to_hwpt_paging(hwpt));
 err_unlock:
 	mutex_unlock(&idev->igroup->lock);
 	return ERR_PTR(rc);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 04109572a53ca..618524e1ce9c5 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -316,7 +316,9 @@ static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt)
 static inline struct iommufd_hwpt_paging *
 to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
 {
-	return container_of(hwpt, struct iommufd_hwpt_paging, common);
+	if (hwpt_is_paging(hwpt))
+		return container_of(hwpt, struct iommufd_hwpt_paging, common);
+	return container_of(hwpt, struct iommufd_hwpt_nested, common)->parent;
 }
 
 static inline struct iommufd_hwpt_paging *