[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y+tpkpNYil3duTIP@Asurada-Nvidia>
Date: Tue, 14 Feb 2023 02:59:30 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "jgg@...dia.com" <jgg@...dia.com>,
"joro@...tes.org" <joro@...tes.org>,
"will@...nel.org" <will@...nel.org>,
"robin.murphy@....com" <robin.murphy@....com>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"shuah@...nel.org" <shuah@...nel.org>,
"Liu, Yi L" <yi.l.liu@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH v2 08/10] iommufd/device: Use iommu_group_replace_domain()
On Fri, Feb 10, 2023 at 02:11:23AM +0000, Tian, Kevin wrote:
> My confusion is that we have different flows between detach/attach
> and replace.
>
> today with separate detach+attach we have following flow:
>
> Remove device from current hwpt;
> if (last_device in hwpt) {
> Remove hwpt domain from current iopt;
> if (last_device in group)
> detach group from hwpt domain;
> }
>
> if (first device in group) {
> attach group to new hwpt domain;
> if (first_device in hwpt)
> Add hwpt domain to new iopt;
> Add device to new hwpt;
>
> but replace flow is different on the detach part:
>
> if (first device in group) {
> replace group's domain from current hwpt to new hwpt;
> if (first_device in hwpt)
> Add hwpt domain to new iopt;
> }
>
> Remove device from old hwpt;
> if (last_device in old hwpt)
> Remove hwpt domain from old iopt;
>
> Add device to new hwpt;
>
> I'm yet to figure out whether we have sufficient lock protection to
> prevent other paths from using old iopt/hwpt to find the device
> which is already attached to a different domain.
With Jason's new series, the detach() routine is lighter now.
I wonder if it'd be safer now to do the detach() call after
iommu_group_replace_domain()?
Thanks
Nic
@@ -196,10 +198,41 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
return false;
}
+/**
+ * __iommufd_device_detach - Detach a device from idev->hwpt
+ * @idev: device to detach
+ * @detach_group: flag to call iommu_detach_group
+ *
+ * This is a cleanup helper shared by the replace and detach routines. Comparing
+ * to a detach routine, a replace call does not need the iommu_detach_group().
+ */
+static void __iommufd_device_detach(struct iommufd_device *idev,
+ bool detach_group)
+{
+ struct iommufd_hw_pagetable *hwpt = idev->hwpt;
+
+ mutex_lock(&hwpt->devices_lock);
+ list_del(&idev->devices_item);
+ if (detach_group && !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);
+
+ if (hwpt->auto_domain)
+ iommufd_object_destroy_user(idev->ictx, &hwpt->obj);
+ else
+ refcount_dec(&hwpt->obj.users);
+
+ idev->hwpt = NULL;
+
+ refcount_dec(&idev->obj.users);
+}
+
/* On success this consumes a hwpt reference from the caller */
static int iommufd_device_do_attach(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt)
{
+ struct iommufd_hw_pagetable *cur_hwpt = idev->hwpt;
phys_addr_t sw_msi_start = PHYS_ADDR_MAX;
int rc;
@@ -237,7 +270,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
* the group once for the first device that is in the group.
*/
if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
- rc = iommu_attach_group(hwpt->domain, idev->group);
+ rc = iommu_group_replace_domain(idev->group, hwpt->domain);
if (rc)
goto out_iova;
@@ -249,6 +282,10 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
}
}
+ /* Detach from the cur_hwpt without iommu_detach_group() */
+ if (cur_hwpt)
+ __iommufd_device_detach(idev, false);
+
idev->hwpt = hwpt;
/* The HWPT reference from the caller is moved to this list */
list_add(&idev->devices_item, &hwpt->devices);
Powered by blists - more mailing lists