On Tue, 15 Nov 2016 20:16:12 -0700
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> On Wed, 16 Nov 2016 08:16:15 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>
> > On 11/16/2016 3:49 AM, Alex Williamson wrote:
> > > On Tue, 15 Nov 2016 20:59:54 +0530
> > > Kirti Wankhede <kwankhede@xxxxxxxxxx> 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:
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
