On Fri, 17 Jun 2022 16:42:30 -0600
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> On Tue, 7 Jun 2022 20:02:11 -0300
> Jason Gunthorpe <jgg@xxxxxxxxxx> 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
> > */
>
