On Thu, Feb 09, 2023 at 04:49:55PM -0400, Jason Gunthorpe wrote:
On Thu, Feb 09, 2023 at 12:28:45PM -0800, Nicolin Chen wrote:
On Thu, Feb 09, 2023 at 03:13:08AM +0000, Tian, Kevin wrote:
--- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) iommufd_ref_to_users(obj); }
/** Set ioas to NULL to block any further iommufd_access_pin_pages().* iommufd_access_unpin_pages() can continue using access-ioas_unpin.
*/access->ioas = NULL;if (cur_ioas) {if (new_ioas) {mutex_unlock(&access->ioas_lock);access->ops->unmap(access->data, 0, ULONG_MAX);mutex_lock(&access->ioas_lock);}why does above only apply to a valid new_ioas? this is the cleanup on cur_ioas then required even when new_ioas=NULL.
Though it'd make sense to put it in the common path, our current detach routine doesn't call this unmap. If we do so, it'd become something new to the normal detach routine. Or does this mean the detach routine has been missing an unmap call so far?
By the time vfio_iommufd_emulated_unbind() is called the driver's close_device() has already returned
At this point the driver should have removed all active pins.
We should not call back into the driver with unmap after its close_device() returns.
I see. Just found that vfio_device_last_close().
However, this function is not on the close_device path so it should always flush all existing mappings before attempting to change the ioas to anything.
OK. That means I also need to fix my change here:
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 8a9834fc129a..ba3fd35b7540 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -465,7 +465,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) struct iommufd_access *access = container_of(obj, struct iommufd_access, obj);
- iommufd_access_set_ioas(access, 0); + if (access->ioas) { + iopt_remove_access(&access->ioas->iopt, access); + refcount_dec(&access->ioas->obj.users); + } iommufd_ctx_put(access->ictx); mutex_destroy(&access->ioas_lock); }
Otherwise, the close_device path would invoke this function via the unbind() call.
Thanks Nic