[Intel-gfx] [PATCH v2 1/2] vfio: Replace the DMA unmapping notifier with a callback
Alex Williamson
alex.williamson at redhat.com
Fri Jun 17 22:42:30 UTC 2022
On Tue, 7 Jun 2022 20:02:11 -0300
Jason Gunthorpe <jgg at nvidia.com> wrote:
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 61e71c1154be67..f005b644ab9e69 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1077,8 +1077,20 @@ 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;
> struct file *filep;
> int ret;
>
> @@ -1109,6 +1121,18 @@ static struct file *vfio_device_open(struct vfio_device *device)
> if (ret)
> goto err_undo_count;
> }
> +
> + 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);
> + }
> +
> up_read(&device->group->group_rwsem);
> }
> mutex_unlock(&device->dev_set->lock);
> @@ -1143,8 +1167,16 @@ static struct file *vfio_device_open(struct vfio_device *device)
> err_close_device:
> mutex_lock(&device->dev_set->lock);
> down_read(&device->group->group_rwsem);
> - if (device->open_count == 1 && device->ops->close_device)
> + if (device->open_count == 1 && device->ops->close_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)
Test for register_notifier callback...
> + iommu_driver->ops->unregister_notifier(
> + device->group->container->iommu_data,
> + &device->iommu_nb);
use unregister_notifier callback. Same below.
> + }
> err_undo_count:
> device->open_count--;
> if (device->open_count == 0 && device->kvm)
> @@ -1339,12 +1371,20 @@ static const struct file_operations vfio_group_fops = {
> static int vfio_device_fops_release(struct inode *inode, struct file *filep)
> {
> struct vfio_device *device = filep->private_data;
> + struct vfio_iommu_driver *iommu_driver;
>
> mutex_lock(&device->dev_set->lock);
> vfio_assert_device_open(device);
> down_read(&device->group->group_rwsem);
> if (device->open_count == 1 && device->ops->close_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);
> up_read(&device->group->group_rwsem);
> device->open_count--;
> if (device->open_count == 0)
> @@ -2027,90 +2067,6 @@ int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova, void *data,
> }
> EXPORT_SYMBOL(vfio_dma_rw);
>
> -static int vfio_register_iommu_notifier(struct vfio_group *group,
> - unsigned long *events,
> - struct notifier_block *nb)
> -{
> - struct vfio_container *container;
> - struct vfio_iommu_driver *driver;
> - int ret;
> -
> - lockdep_assert_held_read(&group->group_rwsem);
> -
> - container = group->container;
> - driver = container->iommu_driver;
> - if (likely(driver && driver->ops->register_notifier))
> - ret = driver->ops->register_notifier(container->iommu_data,
> - events, nb);
> - else
> - ret = -ENOTTY;
> -
> - return ret;
> -}
> -
> -static int vfio_unregister_iommu_notifier(struct vfio_group *group,
> - struct notifier_block *nb)
> -{
> - struct vfio_container *container;
> - struct vfio_iommu_driver *driver;
> - int ret;
> -
> - lockdep_assert_held_read(&group->group_rwsem);
> -
> - container = group->container;
> - driver = container->iommu_driver;
> - if (likely(driver && driver->ops->unregister_notifier))
> - ret = driver->ops->unregister_notifier(container->iommu_data,
> - nb);
> - else
> - ret = -ENOTTY;
> -
> - return ret;
> -}
> -
> -int vfio_register_notifier(struct vfio_device *device,
> - enum vfio_notify_type type, unsigned long *events,
> - struct notifier_block *nb)
> -{
> - struct vfio_group *group = device->group;
> - int ret;
> -
> - if (!nb || !events || (*events == 0) ||
> - !vfio_assert_device_open(device))
> - return -EINVAL;
> -
> - switch (type) {
> - case VFIO_IOMMU_NOTIFY:
> - ret = vfio_register_iommu_notifier(group, events, nb);
> - break;
> - default:
> - ret = -EINVAL;
> - }
> - return ret;
> -}
> -EXPORT_SYMBOL(vfio_register_notifier);
> -
> -int vfio_unregister_notifier(struct vfio_device *device,
> - enum vfio_notify_type type,
> - struct notifier_block *nb)
> -{
> - struct vfio_group *group = device->group;
> - int ret;
> -
> - if (!nb || !vfio_assert_device_open(device))
> - return -EINVAL;
> -
> - switch (type) {
> - case VFIO_IOMMU_NOTIFY:
> - ret = vfio_unregister_iommu_notifier(group, nb);
> - break;
> - default:
> - ret = -EINVAL;
> - }
> - return ret;
> -}
> -EXPORT_SYMBOL(vfio_unregister_notifier);
> -
> /*
> * Module/class support
> */
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a6713022115155..cb2e4e9baa8fe8 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -33,6 +33,11 @@ enum vfio_iommu_notify_type {
> VFIO_IOMMU_CONTAINER_CLOSE = 0,
> };
>
> +/* events for register_notifier() */
> +enum {
> + VFIO_IOMMU_NOTIFY_DMA_UNMAP = 1,
> +};
Can't say I understand why this changed from BIT(0) to an enum, the
event mask is meant to be a bitfield. Using the notifier all the way
to the device was meant to avoid future callbacks on the device. If we
now have a dma_unmap on the device, should the whole infrastructure be
tailored to that one task? For example a dma_unmap_nb on the device,
{un}register_dma_unmap_notifier on the iommu ops,
vfio_dma_unmap_notifier, etc? Thanks,
Alex
> +
> /**
> * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
> */
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index aa888cc517578e..b76623e3b92fca 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -44,6 +44,7 @@ struct vfio_device {
> unsigned int open_count;
> struct completion comp;
> struct list_head group_next;
> + struct notifier_block iommu_nb;
> };
>
> /**
> @@ -60,6 +61,8 @@ struct vfio_device {
> * @match: Optional device name match callback (return: 0 for no-match, >0 for
> * match, -errno for abort (ex. match with insufficient or incorrect
> * additional args)
> + * @dma_unmap: Called when userspace unmaps IOVA from the container
> + * this device is attached to.
> * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl
> * @migration_set_state: Optional callback to change the migration state for
> * devices that support migration. It's mandatory for
> @@ -85,6 +88,7 @@ struct vfio_device_ops {
> int (*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
> void (*request)(struct vfio_device *vdev, unsigned int count);
> int (*match)(struct vfio_device *vdev, char *buf);
> + void (*dma_unmap)(struct vfio_device *vdev, u64 iova, u64 length);
> int (*device_feature)(struct vfio_device *device, u32 flags,
> void __user *arg, size_t argsz);
> struct file *(*migration_set_state)(
> @@ -154,23 +158,6 @@ extern int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> extern int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
> void *data, size_t len, bool write);
>
> -/* each type has independent events */
> -enum vfio_notify_type {
> - VFIO_IOMMU_NOTIFY = 0,
> -};
> -
> -/* events for VFIO_IOMMU_NOTIFY */
> -#define VFIO_IOMMU_NOTIFY_DMA_UNMAP BIT(0)
> -
> -extern int vfio_register_notifier(struct vfio_device *device,
> - enum vfio_notify_type type,
> - unsigned long *required_events,
> - struct notifier_block *nb);
> -extern int vfio_unregister_notifier(struct vfio_device *device,
> - enum vfio_notify_type type,
> - struct notifier_block *nb);
> -
> -
> /*
> * Sub-module helpers
> */
More information about the Intel-gfx
mailing list