On Tue, 7 Jun 2022 20:02:12 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> Instead of bouncing the function call to the driver op through a blocking
> notifier just have the iommu layer call it directly.
>
> Register each device that is being attached to the iommu with the lower
> driver which then threads them on a linked list and calls the appropriate
> driver op at the right time.
>
> Currently the only use is if dma_unmap() is defined.
>
> Also, fully lock all the debugging tests on the pinning path that a
> dma_unmap is registered.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> drivers/vfio/vfio.c | 42 ++++---------
> drivers/vfio/vfio.h | 14 ++---
> drivers/vfio/vfio_iommu_type1.c | 103 ++++++++++++++++++++------------
> include/linux/vfio.h | 2 +-
> 4 files changed, 83 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index f005b644ab9e69..065b57e601bff7 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -619,6 +619,9 @@ EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> */
> int vfio_register_emulated_iommu_dev(struct vfio_device *device)
> {
> + if (WARN_ON(!device->ops->dma_unmap))
> + return -EINVAL;
> +
> return __vfio_register_dev(device,
> vfio_noiommu_group_alloc(device->dev, VFIO_EMULATED_IOMMU));
> }
> @@ -1077,17 +1080,6 @@ static void vfio_device_unassign_container(struct vfio_device *device)
> up_write(&device->group->group_rwsem);
> }
>
> -static int vfio_iommu_notifier(struct notifier_block *nb, unsigned long action,
> - void *data)
> -{
> - struct vfio_device *vfio_device =
> - container_of(nb, struct vfio_device, iommu_nb);
> - struct vfio_iommu_type1_dma_unmap *unmap = data;
> -
> - vfio_device->ops->dma_unmap(vfio_device, unmap->iova, unmap->size);
> - return NOTIFY_OK;
> -}
> -
> static struct file *vfio_device_open(struct vfio_device *device)
> {
> struct vfio_iommu_driver *iommu_driver;
> @@ -1123,15 +1115,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
> }
>
> iommu_driver = device->group->container->iommu_driver;
> - if (device->ops->dma_unmap && iommu_driver &&
> - iommu_driver->ops->register_notifier) {
> - unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> -
> - device->iommu_nb.notifier_call = vfio_iommu_notifier;
> - iommu_driver->ops->register_notifier(
> - device->group->container->iommu_data, &events,
> - &device->iommu_nb);
> - }
> + if (iommu_driver && iommu_driver->ops->register_device)
> + iommu_driver->ops->register_device(
> + device->group->container->iommu_data, device);
>
> up_read(&device->group->group_rwsem);
> }
> @@ -1171,11 +1157,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
> device->ops->close_device(device);
>
> iommu_driver = device->group->container->iommu_driver;
> - if (device->ops->dma_unmap && iommu_driver &&
> - iommu_driver->ops->register_notifier)
> - iommu_driver->ops->unregister_notifier(
> - device->group->container->iommu_data,
> - &device->iommu_nb);
> + if (iommu_driver && iommu_driver->ops->register_device)
> + iommu_driver->ops->unregister_device(
> + device->group->container->iommu_data, device);
But let's fix this in the next respin too, ie. test register but call
unregister. Got it right below in this one.
> }
> err_undo_count:
> device->open_count--;
> @@ -1380,11 +1364,9 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
> device->ops->close_device(device);
>
> iommu_driver = device->group->container->iommu_driver;
> - if (device->ops->dma_unmap && iommu_driver &&
> - iommu_driver->ops->register_notifier)
> - iommu_driver->ops->unregister_notifier(
> - device->group->container->iommu_data,
> - &device->iommu_nb);
> + if (iommu_driver && iommu_driver->ops->unregister_device)
> + iommu_driver->ops->unregister_device(
> + device->group->container->iommu_data, device);
> up_read(&device->group->group_rwsem);
> device->open_count--;
> if (device->open_count == 0)
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index cb2e4e9baa8fe8..4a7db1f3c33e7e 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -33,11 +33,6 @@ enum vfio_iommu_notify_type {
> VFIO_IOMMU_CONTAINER_CLOSE = 0,
> };
>
> -/* events for register_notifier() */
> -enum {
> - VFIO_IOMMU_NOTIFY_DMA_UNMAP = 1,
> -};
> -
> /**
> * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
> */
> @@ -60,11 +55,10 @@ struct vfio_iommu_driver_ops {
> unsigned long *phys_pfn);
> int (*unpin_pages)(void *iommu_data,
> unsigned long *user_pfn, int npage);
> - int (*register_notifier)(void *iommu_data,
> - unsigned long *events,
> - struct notifier_block *nb);
> - int (*unregister_notifier)(void *iommu_data,
> - struct notifier_block *nb);
> + void (*register_device)(void *iommu_data,
> + struct vfio_device *vdev);
> + void (*unregister_device)(void *iommu_data,
> + struct vfio_device *vdev);
> int (*dma_rw)(void *iommu_data, dma_addr_t user_iova,
> void *data, size_t count, bool write);
> struct iommu_domain *(*group_iommu_domain)(void *iommu_data,
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e35759..4ddb1f1abd238b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -67,7 +67,8 @@ struct vfio_iommu {
> struct list_head iova_list;
> struct mutex lock;
> struct rb_root dma_list;
> - struct blocking_notifier_head notifier;
> + struct list_head device_list;
> + struct mutex device_list_lock;
> unsigned int dma_avail;
> unsigned int vaddr_invalid_count;
> uint64_t pgsize_bitmap;
> @@ -865,8 +866,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> }
> }
>
> - /* Fail if notifier list is empty */
> - if (!iommu->notifier.head) {
> + /* Fail if no dma_umap notifier is registered */
No "notifier" anymore.
Should we even get here if this list is empty? Seems like we can
restrict page pinning to devices supporting unmap_dma now and this
could be a WARN_ON. Thanks,
Alex
> + if (list_empty(&iommu->device_list)) {
> ret = -EINVAL;
> goto pin_done;
> }
