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

[Alex Williamson]

On Tue, 15 Nov 2016 20:16:12 -0700
Alex Williamson <address@hidden> wrote:

> On Wed, 16 Nov 2016 08:16:15 +0530
> Kirti Wankhede <address@hidden> wrote:
> 
> > On 11/16/2016 3:49 AM, Alex Williamson wrote:  
> > > On Tue, 15 Nov 2016 20:59:54 +0530
> > > Kirti Wankhede <address@hidden> wrote:
> > >     
> > ...
> >   
> > >> @@ -854,7 +857,28 @@ static int vfio_dma_do_unmap(struct vfio_iommu 
> > >> *iommu,
> > >>                   */
> > >>                  if (dma->task->mm != current->mm)
> > >>                          break;
> > >> +
> > >>                  unmapped += dma->size;
> > >> +
> > >> +                if (iommu->external_domain && 
> > >> !RB_EMPTY_ROOT(&dma->pfn_list)) {
> > >> +                        struct vfio_iommu_type1_dma_unmap nb_unmap;
> > >> +
> > >> +                        nb_unmap.iova = dma->iova;
> > >> +                        nb_unmap.size = dma->size;
> > >> +
> > >> +                        /*
> > >> +                         * Notifier callback would call 
> > >> vfio_unpin_pages() which
> > >> +                         * would acquire iommu->lock. Release lock here 
> > >> and
> > >> +                         * reacquire it again.
> > >> +                         */
> > >> +                        mutex_unlock(&iommu->lock);
> > >> +                        blocking_notifier_call_chain(&iommu->notifier,
> > >> +                                                    
> > >> VFIO_IOMMU_NOTIFY_DMA_UNMAP,
> > >> +                                                    &nb_unmap);
> > >> +                        mutex_lock(&iommu->lock);
> > >> +                        if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
> > >> +                                break;
> > >> +                }    
> > > 
> > > 
> > > Why exactly do we need to notify per vfio_dma rather than per unmap
> > > request?  If we do the latter we can send the notify first, limiting us
> > > to races where a page is pinned between the notify and the locking,
> > > whereas here, even our dma pointer is suspect once we re-acquire the
> > > lock, we don't technically know if another unmap could have removed
> > > that already.  Perhaps something like this (untested):
> > >     
> > 
> > There are checks to validate unmap request, like v2 check and who is
> > calling unmap and is it allowed for that task to unmap. Before these
> > checks its not sure that unmap region range which asked for would be
> > unmapped all. Notify call should be at the place where its sure that the
> > range provided to notify call is definitely going to be removed. My
> > change do that.  
> 
> Ok, but that does solve the problem.  What about this (untested):

s/does/does not/

BTW, I like how the retries here fill the gap in my previous proposal
where we could still race re-pinning.  We've given it an honest shot or
someone is not participating if we've retried 10 times.  I don't
understand why the test for iommu->external_domain was there, clearly
if the list is not empty, we need to notify.  Thanks,

Alex

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index ee9a680..50cafdf 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -782,9 +782,9 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>                            struct vfio_iommu_type1_dma_unmap *unmap)
>  {
>       uint64_t mask;
> -     struct vfio_dma *dma;
> +     struct vfio_dma *dma, *dma_last = NULL;
>       size_t unmapped = 0;
> -     int ret = 0;
> +     int ret = 0, retries;
>  
>       mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>  
> @@ -794,7 +794,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>               return -EINVAL;
>  
>       WARN_ON(mask & PAGE_MASK);
> -
> +again:
>       mutex_lock(&iommu->lock);
>  
>       /*
> @@ -851,11 +851,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>               if (dma->task->mm != current->mm)
>                       break;
>  
> -             unmapped += dma->size;
> -
> -             if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) {
> +             if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
>                       struct vfio_iommu_type1_dma_unmap nb_unmap;
>  
> +                     if (dma_last == dma) {
> +                             BUG_ON(++retries > 10);
> +                     } else {
> +                             dma_last = dma;
> +                             retries = 0;
> +                     }
> +
>                       nb_unmap.iova = dma->iova;
>                       nb_unmap.size = dma->size;
>  
> @@ -868,11 +873,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>                       blocking_notifier_call_chain(&iommu->notifier,
>                                                   VFIO_IOMMU_NOTIFY_DMA_UNMAP,
>                                                   &nb_unmap);
> -                     mutex_lock(&iommu->lock);
> -                     if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
> -                             break;
> +                     goto again:
>               }
> +             unmapped += dma->size;
>               vfio_remove_dma(iommu, dma);
> +
>       }
>  
>  unlock: