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

[Alex Williamson <alex.williamson@redhat.com>]

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
> >   */  
>