From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932808AbcKHQ0o (ORCPT ); Tue, 8 Nov 2016 11:26:44 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:11232 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbcKHQ0m (ORCPT ); Tue, 8 Nov 2016 11:26:42 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Mon, 07 Nov 2016 20:26:06 -0800 Subject: Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP To: Alex Williamson References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-12-git-send-email-kwankhede@nvidia.com> <20161107164544.57ab1a92@t450s.home> CC: , , , , , , , , From: Kirti Wankhede X-Nvconfidentiality: public Message-ID: Date: Tue, 8 Nov 2016 21:56:29 +0530 MIME-Version: 1.0 In-Reply-To: <20161107164544.57ab1a92@t450s.home> X-Originating-IP: [10.24.216.210] X-ClientProxiedBy: DRUKMAIL102.nvidia.com (10.25.59.20) 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/8/2016 5:15 AM, Alex Williamson wrote: > On Sat, 5 Nov 2016 02:40:45 +0530 > Kirti Wankhede wrote: > ... >> >> +int vfio_register_notifier(struct device *dev, struct notifier_block *nb) > > Is the expectation here that this is a generic notifier for all > vfio->mdev signaling? That should probably be made clear in the mdev > API to avoid vendor drivers assuming their notifier callback only > occurs for unmaps, even if that's currently the case. > Ok. Adding comment about notifier callback in mdev_device which is part of next patch. ... >> 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; >> } >> @@ -867,6 +870,11 @@ unlock: >> /* Report how much was unmapped */ >> unmap->size = unmapped; >> >> + if (unmapped && iommu->external_domain) >> + blocking_notifier_call_chain(&iommu->notifier, >> + VFIO_IOMMU_NOTIFY_DMA_UNMAP, >> + unmap); > > This is after the fact, there's already a gap here where pages are > unpinned and the mdev device is still running. Oh, there is a bug here, now unpin_pages() take user_pfn as argument and find vfio_dma. If its not found, it doesn't unpin pages. We have to call this notifier before vfio_remove_dma(). But if we call this before vfio_remove_dma() there will be deadlock since iommu->lock is already held here and vfio_iommu_type1_unpin_pages() will also try to hold iommu->lock. If we want to call blocking_notifier_call_chain() before vfio_remove_dma(), sequence should be: unmapped += dma->size; mutex_unlock(&iommu->lock); if (iommu->external_domain)) { struct vfio_iommu_type1_dma_unmap nb_unmap; nb_unmap.iova = dma->iova; nb_unmap.size = dma->size; blocking_notifier_call_chain(&iommu->notifier, VFIO_IOMMU_NOTIFY_DMA_UNMAP, &nb_unmap); } mutex_lock(&iommu->lock); vfio_remove_dma(iommu, dma); > The notifier needs to > happen prior to that and I suspect that we need to validate that we > have no remaining external pfn references within this vfio_dma block. > It seems like we need to root our pfn tracking in the vfio_dma so that > we can see that it's empty after the notifier chain and BUG_ON if not. There is no way to find pfns from that iova range with current implementation. We can have this validate if we go with linear array of iova to track pfns. > I would also add some enforcement that external pinning is only enabled > when vfio_iommu_type1 is configured for v2 semantics (ie. we only > support unmaps exactly matching previous maps). > Ok I'll add that check. Thanks, Kirti