Re: [PATCH v2 1/2] vfio: Replace the DMA unmapping notifier with a callback

On Fri, 17 Jun 2022 16:42:30 -0600 Alex Williamson alex.williamson@redhat.com wrote:

On Tue, 7 Jun 2022 20:02:11 -0300 Jason Gunthorpe jgg@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,

Ok, this all seems cleared up in the next patch, maybe there's a better intermediate step, but not worth bike shedding. 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

*/