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

[Kirti Wankhede]

On 10/28/2016 1:03 PM, Jike Song wrote:
> On 10/27/2016 05:29 AM, Kirti Wankhede wrote:
>>  
>> +/*
>> + * 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. 

That because there could be multiple mdev devices from different vendor
driver in a domain, and pin request for a page can come from multiple
vendor driver for same page and in pin/unpin page interface we don't
keep track of who had pinned pages. So its vendor driver's
responsibility to unpin pages which he had pinned.

> 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.
>

That's right.

> 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.
> 

Don't know what the reserve mapping is, vendor should know the pfn that
he has previously pinned to call the unpin anyway so there is no need to
duplicate the logic here.

Kirti

> 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?
>