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