Re: [Qemu-devel] [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP

[Alex Williamson]

On Sat, 5 Nov 2016 02:40:45 +0530
Kirti Wankhede <address@hidden> wrote:

> Added blocking notifier to IOMMU TYPE1 driver to notify vendor drivers
> about DMA_UNMAP.
> Exported two APIs vfio_register_notifier() and vfio_unregister_notifier().
> Notifier should be registered, if external user wants to use
> vfio_pin_pages()/vfio_unpin_pages() APIs to pin/unpin pages.
> Vendor driver should use VFIO_IOMMU_NOTIFY_DMA_UNMAP action to invalidate
> mappings.
> 
> Signed-off-by: Kirti Wankhede <address@hidden>
> Signed-off-by: Neo Jia <address@hidden>
> Change-Id: I5910d0024d6be87f3e8d3e0ca0eaeaaa0b17f271
> ---
>  drivers/vfio/vfio.c             | 73 
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/vfio/vfio_iommu_type1.c | 47 ++++++++++++++++++++------
>  include/linux/vfio.h            | 11 +++++++
>  3 files changed, 121 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 76d260e98930..4ed1a6a247c6 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1895,6 +1895,79 @@ err_unpin_pages:
>  }
>  EXPORT_SYMBOL(vfio_unpin_pages);
>  
> +int vfio_register_notifier(struct device *dev, struct notifier_block *nb)

Is the expectation here that this is a generic notifier for all
vfio->mdev signaling?  That should probably be made clear in the mdev
API to avoid vendor drivers assuming their notifier callback only
occurs for unmaps, even if that's currently the case.

> +{
> +     struct vfio_container *container;
> +     struct vfio_group *group;
> +     struct vfio_iommu_driver *driver;
> +     ssize_t ret;
> +
> +     if (!dev || !nb)
> +             return -EINVAL;
> +
> +     group = vfio_group_get_from_dev(dev);
> +     if (IS_ERR(group))
> +             return PTR_ERR(group);
> +
> +     ret = vfio_group_add_container_user(group);
> +     if (ret)
> +             goto err_register_nb;
> +
> +     container = group->container;
> +     down_read(&container->group_lock);
> +
> +     driver = container->iommu_driver;
> +     if (likely(driver && driver->ops->register_notifier))
> +             ret = driver->ops->register_notifier(container->iommu_data, nb);
> +     else
> +             ret = -EINVAL;

-ENOTTY again?  And below.

> +
> +     up_read(&container->group_lock);
> +     vfio_group_try_dissolve_container(group);
> +
> +err_register_nb:
> +     vfio_group_put(group);
> +     return ret;
> +}
> +EXPORT_SYMBOL(vfio_register_notifier);
> +
> +int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
> +{
> +     struct vfio_container *container;
> +     struct vfio_group *group;
> +     struct vfio_iommu_driver *driver;
> +     ssize_t ret;
> +
> +     if (!dev || !nb)
> +             return -EINVAL;
> +
> +     group = vfio_group_get_from_dev(dev);
> +     if (IS_ERR(group))
> +             return PTR_ERR(group);
> +
> +     ret = vfio_group_add_container_user(group);
> +     if (ret)
> +             goto err_unregister_nb;
> +
> +     container = group->container;
> +     down_read(&container->group_lock);
> +
> +     driver = container->iommu_driver;
> +     if (likely(driver && driver->ops->unregister_notifier))
> +             ret = driver->ops->unregister_notifier(container->iommu_data,
> +                                                    nb);
> +     else
> +             ret = -EINVAL;
> +
> +     up_read(&container->group_lock);
> +     vfio_group_try_dissolve_container(group);

The concern any time we have an unregister like this is whether the
vendor driver does proper cleanup.  Maybe we don't even need an
unregister, could we track this on the group such that releasing the
group automatically unregisters the notifier?  Maybe a single nb
slot and -EBUSY if already set, cleared on release?  Along those lines,
automatically unpinning anything would also be a nice feature (ie. if
an mdev device is unplugged while other devices are still in the
container), but then we'd need to track pinning per group and we already
have too much overhead in tracking pinning.

> +
> +err_unregister_nb:
> +     vfio_group_put(group);
> +     return ret;
> +}
> +EXPORT_SYMBOL(vfio_unregister_notifier);
> +
>  /**
>   * Module/class support
>   */
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index e511073446a0..c2d3a84c447b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -37,6 +37,7 @@
>  #include <linux/vfio.h>
>  #include <linux/workqueue.h>
>  #include <linux/mdev.h>
> +#include <linux/notifier.h>
>  
>  #define DRIVER_VERSION  "0.2"
>  #define DRIVER_AUTHOR   "Alex Williamson <address@hidden>"
> @@ -60,6 +61,7 @@ struct vfio_iommu {
>       struct vfio_domain      *external_domain; /* domain for external user */
>       struct mutex            lock;
>       struct rb_root          dma_list;
> +     struct blocking_notifier_head notifier;
>       bool                    v2;
>       bool                    nesting;
>  };
> @@ -550,7 +552,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  
>       mutex_lock(&iommu->lock);
>  
> -     if (!iommu->external_domain) {
> +     /* Fail if notifier list is empty */
> +     if ((!iommu->external_domain) || (!iommu->notifier.head)) {
>               ret = -EINVAL;
>               goto pin_done;
>       }
> @@ -867,6 +870,11 @@ unlock:
>       /* Report how much was unmapped */
>       unmap->size = unmapped;
>  
> +     if (unmapped && iommu->external_domain)
> +             blocking_notifier_call_chain(&iommu->notifier,
> +                                          VFIO_IOMMU_NOTIFY_DMA_UNMAP,
> +                                          unmap);

This is after the fact, there's already a gap here where pages are
unpinned and the mdev device is still running.  The notifier needs to
happen prior to that and I suspect that we need to validate that we
have no remaining external pfn references within this vfio_dma block.
It seems like we need to root our pfn tracking in the vfio_dma so that
we can see that it's empty after the notifier chain and BUG_ON if not.
I would also add some enforcement that external pinning is only enabled
when vfio_iommu_type1 is configured for v2 semantics (ie. we only
support unmaps exactly matching previous maps).

> +
>       return ret;
>  }
>  
> @@ -1474,6 +1482,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
>       INIT_LIST_HEAD(&iommu->addr_space_list);
>       iommu->dma_list = RB_ROOT;
>       mutex_init(&iommu->lock);
> +     BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
>  
>       return iommu;
>  }
> @@ -1610,16 +1619,34 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>       return -ENOTTY;
>  }
>  
> +static int vfio_iommu_type1_register_notifier(void *iommu_data,
> +                                           struct notifier_block *nb)
> +{
> +     struct vfio_iommu *iommu = iommu_data;
> +
> +     return blocking_notifier_chain_register(&iommu->notifier, nb);
> +}
> +
> +static int vfio_iommu_type1_unregister_notifier(void *iommu_data,
> +                                             struct notifier_block *nb)
> +{
> +     struct vfio_iommu *iommu = iommu_data;
> +
> +     return blocking_notifier_chain_unregister(&iommu->notifier, nb);
> +}
> +
>  static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
> -     .name           = "vfio-iommu-type1",
> -     .owner          = THIS_MODULE,
> -     .open           = vfio_iommu_type1_open,
> -     .release        = vfio_iommu_type1_release,
> -     .ioctl          = vfio_iommu_type1_ioctl,
> -     .attach_group   = vfio_iommu_type1_attach_group,
> -     .detach_group   = vfio_iommu_type1_detach_group,
> -     .pin_pages      = vfio_iommu_type1_pin_pages,
> -     .unpin_pages    = vfio_iommu_type1_unpin_pages,
> +     .name                   = "vfio-iommu-type1",
> +     .owner                  = THIS_MODULE,
> +     .open                   = vfio_iommu_type1_open,
> +     .release                = vfio_iommu_type1_release,
> +     .ioctl                  = vfio_iommu_type1_ioctl,
> +     .attach_group           = vfio_iommu_type1_attach_group,
> +     .detach_group           = vfio_iommu_type1_detach_group,
> +     .pin_pages              = vfio_iommu_type1_pin_pages,
> +     .unpin_pages            = vfio_iommu_type1_unpin_pages,
> +     .register_notifier      = vfio_iommu_type1_register_notifier,
> +     .unregister_notifier    = vfio_iommu_type1_unregister_notifier,
>  };
>  
>  static int __init vfio_iommu_type1_init(void)
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index ba1b64cb7d4b..dcda8fccefab 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -82,6 +82,10 @@ struct vfio_iommu_driver_ops {
>                                      unsigned long *user_pfn,
>                                      unsigned long *pfn,
>                                      int npage);
> +     int             (*register_notifier)(void *iommu_data,
> +                                          struct notifier_block *nb);
> +     int             (*unregister_notifier)(void *iommu_data,
> +                                            struct notifier_block *nb);
>  };
>  
>  extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops 
> *ops);
> @@ -139,6 +143,13 @@ extern int vfio_pin_pages(struct device *dev, unsigned 
> long *user_pfn,
>  extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
>                           unsigned long *pfn, int npage);
>  
> +#define VFIO_IOMMU_NOTIFY_DMA_UNMAP  1
> +
> +extern int vfio_register_notifier(struct device *dev,
> +                               struct notifier_block *nb);
> +
> +extern int vfio_unregister_notifier(struct device *dev,
> +                                 struct notifier_block *nb);
>  /*
>   * IRQfd - generic
>   */