| Message ID | 20230513132136.15021-2-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance vfio PCI hot reset for vfio cdev device | expand |
On Sat, 13 May 2023 06:21:27 -0700 Yi Liu <yi.l.liu@intel.com> wrote: > This binds noiommu device to iommufd and creates iommufd_access for this > bond. This is useful for adding an iommufd-based device ownership check > for VFIO_DEVICE_PCI_HOT_RESET since this model requires all the other > affected devices bound to the same iommufd as the device to be reset. > For noiommu devices, there is no backend iommu, so create iommufd_access > instead of iommufd_device. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/vfio/iommufd.c | 43 ++++++++++++++++++++++++++++++++++++++++-- > include/linux/vfio.h | 1 + > 2 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c > index 88b00c501015..c1379e826052 100644 > --- a/drivers/vfio/iommufd.c > +++ b/drivers/vfio/iommufd.c > @@ -10,6 +10,42 @@ > MODULE_IMPORT_NS(IOMMUFD); > MODULE_IMPORT_NS(IOMMUFD_VFIO); > > +static void vfio_noiommu_access_unmap(void *data, unsigned long iova, > + unsigned long length) > +{ Should this WARN_ON if called? > +} > + > +static const struct iommufd_access_ops vfio_user_noiommu_ops = { > + .needs_pin_pages = 1, But it doesn't. > + .unmap = vfio_noiommu_access_unmap, > +}; > + > +static int vfio_iommufd_noiommu_bind(struct vfio_device *vdev, > + struct iommufd_ctx *ictx, > + u32 *out_device_id) > +{ > + struct iommufd_access *user; > + > + lockdep_assert_held(&vdev->dev_set->lock); > + > + user = iommufd_access_create(ictx, &vfio_user_noiommu_ops, > + vdev, out_device_id); > + if (IS_ERR(user)) > + return PTR_ERR(user); > + vdev->noiommu_access = user; > + return 0; > +} > + > +static void vfio_iommufd_noiommu_unbind(struct vfio_device *vdev) > +{ > + lockdep_assert_held(&vdev->dev_set->lock); > + > + if (vdev->noiommu_access) { > + iommufd_access_destroy(vdev->noiommu_access); > + vdev->noiommu_access = NULL; > + } > +} > + > int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > { > u32 ioas_id; > @@ -29,7 +65,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > */ > if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id)) > return -EPERM; > - return 0; > + > + return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id); > } > > ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id); > @@ -59,8 +96,10 @@ void vfio_iommufd_unbind(struct vfio_device *vdev) > { > lockdep_assert_held(&vdev->dev_set->lock); > > - if (vfio_device_is_noiommu(vdev)) > + if (vfio_device_is_noiommu(vdev)) { > + vfio_iommufd_noiommu_unbind(vdev); > return; > + } > > if (vdev->ops->unbind_iommufd) > vdev->ops->unbind_iommufd(vdev); > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 2c137ea94a3e..16fd04490550 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -57,6 +57,7 @@ struct vfio_device { > struct list_head group_next; > struct list_head iommu_entry; > struct iommufd_access *iommufd_access; > + struct iommufd_access *noiommu_access; It's not clear to me why we need a separate iommufd_access for noiommu. Can't we add a vfio_device_is_noiommu() check to the vfio_{un}pin_pages() and vfio_dma_rw() interfaces and reuse the existing pointer for both emulated and noiommu cases? Maybe even the iommufd_access* functions should test needs_pin_pages and generate an error/warning if an access that was registered without reporting that it needs page pinning makes use of such an interface. Thanks, Alex > void (*put_kvm)(struct kvm *kvm); > #if IS_ENABLED(CONFIG_IOMMUFD) > struct iommufd_device *iommufd_device;
On Wed, May 17, 2023 at 11:26:09AM -0600, Alex Williamson wrote: > It's not clear to me why we need a separate iommufd_access for > noiommu. The point was to allocate an ID for the device so we can use that ID with the other interfaces in all cases. Otherwise it is a too weird special case that is probably going to keep causing trouble down the road... Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, May 18, 2023 2:21 AM > > On Wed, May 17, 2023 at 11:26:09AM -0600, Alex Williamson wrote: > > > It's not clear to me why we need a separate iommufd_access for > > noiommu. > > The point was to allocate an ID for the device so we can use that ID > with the other interfaces in all cases. I guess Alex's question is why adding a new pointer named noiommu_access while there is already the iommufd_access pointer named iommufd_access. Maybe we shall reuse the iommufd_access pointer? Regards, Yi Liu
On Thu, 18 May 2023 12:23:29 +0000 "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Thursday, May 18, 2023 2:21 AM > > > > On Wed, May 17, 2023 at 11:26:09AM -0600, Alex Williamson wrote: > > > > > It's not clear to me why we need a separate iommufd_access for > > > noiommu. > > > > The point was to allocate an ID for the device so we can use that ID > > with the other interfaces in all cases. > > I guess Alex's question is why adding a new pointer named noiommu_access > while there is already the iommufd_access pointer named iommufd_access. Yes, precisely. Sorry that was confusing, we need the access for noiommu but it's not clear why that access needs to be stored in a separate pointer when we can already differentiate noiommu devices otherwise. Thanks, Alex
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 88b00c501015..c1379e826052 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -10,6 +10,42 @@ MODULE_IMPORT_NS(IOMMUFD); MODULE_IMPORT_NS(IOMMUFD_VFIO); +static void vfio_noiommu_access_unmap(void *data, unsigned long iova, + unsigned long length) +{ +} + +static const struct iommufd_access_ops vfio_user_noiommu_ops = { + .needs_pin_pages = 1, + .unmap = vfio_noiommu_access_unmap, +}; + +static int vfio_iommufd_noiommu_bind(struct vfio_device *vdev, + struct iommufd_ctx *ictx, + u32 *out_device_id) +{ + struct iommufd_access *user; + + lockdep_assert_held(&vdev->dev_set->lock); + + user = iommufd_access_create(ictx, &vfio_user_noiommu_ops, + vdev, out_device_id); + if (IS_ERR(user)) + return PTR_ERR(user); + vdev->noiommu_access = user; + return 0; +} + +static void vfio_iommufd_noiommu_unbind(struct vfio_device *vdev) +{ + lockdep_assert_held(&vdev->dev_set->lock); + + if (vdev->noiommu_access) { + iommufd_access_destroy(vdev->noiommu_access); + vdev->noiommu_access = NULL; + } +} + int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) { u32 ioas_id; @@ -29,7 +65,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) */ if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id)) return -EPERM; - return 0; + + return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id); } ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id); @@ -59,8 +96,10 @@ void vfio_iommufd_unbind(struct vfio_device *vdev) { lockdep_assert_held(&vdev->dev_set->lock); - if (vfio_device_is_noiommu(vdev)) + if (vfio_device_is_noiommu(vdev)) { + vfio_iommufd_noiommu_unbind(vdev); return; + } if (vdev->ops->unbind_iommufd) vdev->ops->unbind_iommufd(vdev); diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 2c137ea94a3e..16fd04490550 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -57,6 +57,7 @@ struct vfio_device { struct list_head group_next; struct list_head iommu_entry; struct iommufd_access *iommufd_access; + struct iommufd_access *noiommu_access; void (*put_kvm)(struct kvm *kvm); #if IS_ENABLED(CONFIG_IOMMUFD) struct iommufd_device *iommufd_device;
This binds noiommu device to iommufd and creates iommufd_access for this bond. This is useful for adding an iommufd-based device ownership check for VFIO_DEVICE_PCI_HOT_RESET since this model requires all the other affected devices bound to the same iommufd as the device to be reset. For noiommu devices, there is no backend iommu, so create iommufd_access instead of iommufd_device. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/vfio/iommufd.c | 43 ++++++++++++++++++++++++++++++++++++++++-- include/linux/vfio.h | 1 + 2 files changed, 42 insertions(+), 2 deletions(-)