Re: [PATCH v10 10/19] vfio iommu: Add blocking notifier to notify DMA_UNMAP

[Kirti Wankhede <kwankhede@nvidia.com>]

On 10/28/2016 6:10 PM, Alex Williamson wrote:
> On Fri, 28 Oct 2016 15:33:58 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 10/27/2016 05:29 AM, Kirti Wankhede 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().
>>> Vendor driver should register notifer using these APIs.
>>> Vendor driver should use VFIO_IOMMU_NOTIFY_DMA_UNMAP action to invalidate
>>> mappings.
>>>
>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>>> Change-Id: I5910d0024d6be87f3e8d3e0ca0eaeaaa0b17f271
>>> ---
>>>  drivers/vfio/vfio.c             | 73 +++++++++++++++++++++++++++++++++
>>>  drivers/vfio/vfio_iommu_type1.c | 89 ++++++++++++++++++++++++++++++++++++-----
>>>  include/linux/vfio.h            | 11 +++++
>>>  3 files changed, 163 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index 28b50ca14c52..ff05ac6b1e90 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -1891,6 +1891,79 @@ err_unpin_pages:
>>>  }
>>>  EXPORT_SYMBOL(vfio_unpin_pages);
>>>  
>>> +int vfio_register_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_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;
>>> +
>>> +	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);
>>> +
>>> +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 5add11a147e1..a4bd331ac0fd 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 <alex.williamson@redhat.com>"
>>> @@ -59,6 +60,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;
>>>  };
>>> @@ -549,7 +551,8 @@ static long 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;
>>>  	}
>>> @@ -768,6 +771,50 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
>>>  	return bitmap;
>>>  }
>>>  
>>> +/*
>>> + * This function finds pfn in domain->external_addr_space->pfn_list for given
>>> + * iova range. If pfn exist, notify pfn to registered notifier list. On
>>> + * receiving notifier callback, vendor driver should invalidate the mapping and
>>> + * call vfio_unpin_pages() to unpin this pfn. With that vfio_pfn for this pfn
>>> + * gets removed from rb tree of pfn_list. That re-arranges rb tree, so while
>>> + * searching for next vfio_pfn in rb tree, start search from first node again.
>>> + * If any vendor driver doesn't unpin that pfn, vfio_pfn would not get removed
>>> + * from rb tree and so in next search vfio_pfn would be same as previous
>>> + * vfio_pfn. In that case, exit from loop.
>>> + */
>>> +static void vfio_notifier_call_chain(struct vfio_iommu *iommu,
>>> +				     struct vfio_iommu_type1_dma_unmap *unmap)
>>> +{
>>> +	struct vfio_domain *domain = iommu->external_domain;
>>> +	struct rb_node *n;
>>> +	struct vfio_pfn *vpfn = NULL, *prev_vpfn;
>>> +
>>> +	do {
>>> +		prev_vpfn = vpfn;
>>> +		mutex_lock(&domain->external_addr_space->pfn_list_lock);
>>> +
>>> +		n = rb_first(&domain->external_addr_space->pfn_list);
>>> +
>>> +		for (; n; n = rb_next(n), vpfn = NULL) {
>>> +			vpfn = rb_entry(n, struct vfio_pfn, node);
>>> +
>>> +			if ((vpfn->iova >= unmap->iova) &&
>>> +			    (vpfn->iova < unmap->iova + unmap->size))
>>> +				break;
>>> +		}
>>> +
>>> +		mutex_unlock(&domain->external_addr_space->pfn_list_lock);
>>> +
>>> +		/* Notify any listeners about DMA_UNMAP */
>>> +		if (vpfn)
>>> +			blocking_notifier_call_chain(&iommu->notifier,
>>> +						    VFIO_IOMMU_NOTIFY_DMA_UNMAP,
>>> +						    &vpfn->pfn);  
>>
>> Hi Kirti, 
>>
>> The information carried by notifier is only a pfn.
>>
>> Since your pin/unpin interfaces design, it's the vendor driver who should
>> guarantee pin/unpin same times. To achieve that, the vendor driver must
>> cache it's iova->pfn mapping on its side, to avoid pinning a same page
>> for multiple times.
>>
>> With the notifier carrying only a pfn, to find the iova by this pfn,
>> the vendor driver must *also* keep a reverse-mapping. That's a bit
>> too much.
>>
>> Since the vendor could also suffer from IOMMU-compatible problem,
>> which means a local cache is always helpful, so I'd like to have the
>> iova carried to the notifier.
>>
>> What'd you say?
> 
> I agree, the pfn is not unique, multiple guest pfns (iovas) might be
> backed by the same host pfn.  DMA_UNMAP calls are based on iova, the
> notifier through to the vendor driver must be based on the same.

Host pfn should be unique, right?

Kirti.