On Sat, 13 May 2023 06:21:27 -0700
Yi Liu <yi.l.liu@xxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> 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;
