From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754349AbcKPCsd (ORCPT ); Tue, 15 Nov 2016 21:48:33 -0500 Received: from hqemgate15.nvidia.com ([216.228.121.64]:15483 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751982AbcKPCsc (ORCPT ); Tue, 15 Nov 2016 21:48:32 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Tue, 15 Nov 2016 06:47:31 -0800 Subject: Re: [PATCH v13 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP To: Alex Williamson References: <1479223805-22895-1-git-send-email-kwankhede@nvidia.com> <1479223805-22895-12-git-send-email-kwankhede@nvidia.com> <20161115151950.1e8ab7d6@t450s.home> CC: , , , , , , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: Date: Wed, 16 Nov 2016 08:16:15 +0530 MIME-Version: 1.0 In-Reply-To: <20161115151950.1e8ab7d6@t450s.home> X-Originating-IP: [10.24.70.148] X-ClientProxiedBy: BGMAIL103.nvidia.com (10.25.59.12) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/16/2016 3:49 AM, Alex Williamson wrote: > On Tue, 15 Nov 2016 20:59:54 +0530 > Kirti Wankhede 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. Thanks, Kirti > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index ee9a680..8504501 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -785,6 +785,8 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > struct vfio_dma *dma; > size_t unmapped = 0; > int ret = 0; > + struct vfio_iommu_type1_dma_unmap nb_unmap = { .iova = unmap->iova, > + .size = unmap->size }; > > mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1; > > @@ -795,6 +797,14 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > > WARN_ON(mask & PAGE_MASK); > > + /* > + * Notify anyone (mdev vendor drivers) to invalidate and unmap > + * iovas within the range we're about to unmap. Vendor drivers MUST > + * unpin pages in response to an invalidation. > + */ > + blocking_notifier_call_chain(&iommu->notifier, > + VFIO_IOMMU_NOTIFY_DMA_UNMAP, &nb_unmap); > + > mutex_lock(&iommu->lock); > > /* > @@ -853,25 +863,8 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > > unmapped += dma->size; > > - if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) { > - struct vfio_iommu_type1_dma_unmap nb_unmap; > + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); > > - 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; > - } > vfio_remove_dma(iommu, dma); > } > > > >> vfio_remove_dma(iommu, dma); >> } >>