| Message ID | 931be169ff4a1f4d4f1ed060d722c2dc17ce6667.1675802050.git.nicolinc@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add IO page table replacement support | expand |
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, February 8, 2023 5:18 AM > > Support an access->ioas replacement in iommufd_access_set_ioas(), which > sets the access->ioas to NULL provisionally so that any further incoming > iommufd_access_pin_pages() callback can be blocked. > > Then, call access->ops->unmap() to clean up the entire iopt. To allow an > iommufd_access_unpin_pages() callback to happen via this unmap() call, > add an ioas_unpin pointer so the unpin routine won't be affected by the > "access->ioas = NULL" trick above. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> move patch10 before this one. > --- > drivers/iommu/iommufd/device.c | 16 ++++++++++++++-- > drivers/iommu/iommufd/iommufd_private.h | 1 + > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/iommufd/device.c > b/drivers/iommu/iommufd/device.c > index f4bd6f532a90..10ce47484ffa 100644 > --- 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. Instead here the check should be "if (access->ops->unmap)". with patch10 the cleanup is required only for driver which is allowed to pin pages.
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? Thanks Nic
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. 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. Jason
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
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index f4bd6f532a90..10ce47484ffa 100644 --- 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); + } iopt_remove_access(&cur_ioas->iopt, access); refcount_dec(&cur_ioas->obj.users); } + access->ioas_unpin = new_ioas; access->ioas = new_ioas; mutex_unlock(&access->ioas_lock); @@ -587,11 +599,11 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, return; mutex_lock(&access->ioas_lock); - if (!access->ioas) { + if (!access->ioas_unpin) { mutex_unlock(&access->ioas_lock); return; } - iopt = &access->ioas->iopt; + iopt = &access->ioas_unpin->iopt; down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 2f4bb106bac6..593138bb37b8 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -261,6 +261,7 @@ struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; struct iommufd_ioas *ioas; + struct iommufd_ioas *ioas_unpin; struct mutex ioas_lock; const struct iommufd_access_ops *ops; void *data;
Support an access->ioas replacement in iommufd_access_set_ioas(), which sets the access->ioas to NULL provisionally so that any further incoming iommufd_access_pin_pages() callback can be blocked. Then, call access->ops->unmap() to clean up the entire iopt. To allow an iommufd_access_unpin_pages() callback to happen via this unmap() call, add an ioas_unpin pointer so the unpin routine won't be affected by the "access->ioas = NULL" trick above. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/device.c | 16 ++++++++++++++-- drivers/iommu/iommufd/iommufd_private.h | 1 + 2 files changed, 15 insertions(+), 2 deletions(-)