| Message ID | bfe5aed6d354ef547979f0b256c8a3f9bd5b223b.1675320212.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add IO page table replacement support | expand |
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Thursday, February 2, 2023 3:05 PM > > 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. > > Also, a vdev without an ops->dma_unmap implementation cannot replace its > access->ioas pointer. So add an iommufd_access_ioas_is_attached() helper > to sanity that. > Presumably a driver which doesn't implement ops->dma_unmap shouldn't be allowed to do pin/unpin. But it could use vfio_dma_rw() to access an iova range. In the latter case I don't see why replace cannot work. Probably what's required here is to deny !ops->dma_unmap in vfio_pin/unpin_pages then making here replace always allowed?
On Fri, Feb 03, 2023 at 10:10:45AM +0000, Tian, Kevin wrote: > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Thursday, February 2, 2023 3:05 PM > > > > 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. > > > > Also, a vdev without an ops->dma_unmap implementation cannot replace its > > access->ioas pointer. So add an iommufd_access_ioas_is_attached() helper > > to sanity that. > > > > Presumably a driver which doesn't implement ops->dma_unmap shouldn't > be allowed to do pin/unpin. But it could use vfio_dma_rw() to access an > iova range. In the latter case I don't see why replace cannot work. > > Probably what's required here is to deny !ops->dma_unmap in > vfio_pin/unpin_pages then making here replace always allowed? That makes sense Jason
On Fri, Feb 03, 2023 at 08:31:52AM -0400, Jason Gunthorpe wrote: > On Fri, Feb 03, 2023 at 10:10:45AM +0000, Tian, Kevin wrote: > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Thursday, February 2, 2023 3:05 PM > > > > > > 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. > > > > > > Also, a vdev without an ops->dma_unmap implementation cannot replace its > > > access->ioas pointer. So add an iommufd_access_ioas_is_attached() helper > > > to sanity that. > > > > > > > Presumably a driver which doesn't implement ops->dma_unmap shouldn't > > be allowed to do pin/unpin. But it could use vfio_dma_rw() to access an > > iova range. In the latter case I don't see why replace cannot work. > > > > Probably what's required here is to deny !ops->dma_unmap in > > vfio_pin/unpin_pages then making here replace always allowed? > > That makes sense Will change that in v2. Thanks Nic
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index f4bd6f532a90..8118d5262800 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); @@ -527,6 +539,18 @@ int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) } EXPORT_SYMBOL_NS_GPL(iommufd_access_set_ioas, IOMMUFD); +bool iommufd_access_ioas_is_attached(struct iommufd_access *access) +{ + bool attached; + + mutex_lock(&access->ioas_lock); + attached = !!access->ioas; + mutex_unlock(&access->ioas_lock); + + return attached; +} +EXPORT_SYMBOL_NS_GPL(iommufd_access_ioas_is_attached, IOMMUFD); + /** * iommufd_access_notify_unmap - Notify users of an iopt to stop using it * @iopt: iopt to work on @@ -587,11 +611,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; diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 78a8e4641cbf..7e09defbcffe 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -187,6 +187,10 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id) if (!vdev->iommufd_access) return -ENOENT; + if (!vdev->ops->dma_unmap && pt_id && + iommufd_access_ioas_is_attached(vdev->iommufd_access)) + return -EBUSY; + iommufd_access_set_ioas(vdev->iommufd_access, *pt_id); return 0; } diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 0e30f2ce27cd..e8d764b2c14c 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -44,6 +44,7 @@ iommufd_access_create(struct iommufd_ctx *ictx, const struct iommufd_access_ops *ops, void *data); void iommufd_access_destroy(struct iommufd_access *access); int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id); +bool iommufd_access_ioas_is_attached(struct iommufd_access *access); void iommufd_ctx_get(struct iommufd_ctx *ictx);
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. Also, a vdev without an ops->dma_unmap implementation cannot replace its access->ioas pointer. So add an iommufd_access_ioas_is_attached() helper to sanity that. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/device.c | 28 +++++++++++++++++++++++-- drivers/iommu/iommufd/iommufd_private.h | 1 + drivers/vfio/iommufd.c | 4 ++++ include/linux/iommufd.h | 1 + 4 files changed, 32 insertions(+), 2 deletions(-)