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

From: Nicolin Chen
Date: Wed Jul 31 2024 - 17:22:13 EST

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@xxxxxxxxxx>
> > > 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 *