From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55002) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk4nv-00039l-4m for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:40:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk4np-0006zN-5N for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:40:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45614) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk4no-0006z6-Ss for qemu-devel@nongnu.org; Wed, 14 Sep 2016 03:40:17 -0400 Date: Wed, 14 Sep 2016 15:40:08 +0800 From: Peter Xu Message-ID: <20160914074008.GN3776@pxdev.xzpeter.org> References: <1473389864-19694-1-git-send-email-peterx@redhat.com> <1473389864-19694-2-git-send-email-peterx@redhat.com> <20160914054832.GL15077@voom.fritz.box> <20160914071503.GO15077@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160914071503.GO15077@voom.fritz.box> Subject: Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mst@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, cornelia.huck@de.ibm.com, alex.williamson@redhat.com, wexu@redhat.com, dgibson@redhat.com, vkaplans@redhat.com, pbonzini@redhat.com On Wed, Sep 14, 2016 at 05:15:03PM +1000, David Gibson wrote: [...] > > > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > > > void memory_region_notify_iommu(MemoryRegion *mr, > > > IOMMUTLBEntry entry) > > > { > > > + IOMMUNotifier *iommu_notifier; > > > + IOMMUNotifierFlag request_flags; > > > + > > > assert(memory_region_is_iommu(mr)); > > > - notifier_list_notify(&mr->iommu_notify, &entry); > > > + > > > + if (entry.perm & IOMMU_RW) { > > > + request_flags = IOMMU_NOTIFIER_MAP; > > > + } else { > > > + request_flags = IOMMU_NOTIFIER_UNMAP; > > > + } > > > > This is still wrong. UNMAP depends on the *previous* state of the > > mapping, not the new state. > > Peter pointed out to be on IRC that VFIO already assumes that it's > only an unmap if the new permissions are NONE. So one can argue that > it's an existing constraint of the IOMMUTLBEntry interface that a > mapping can only ever transition from valid->invalid or > invalid->valid. Changing one valid entry to another would require two > notifications one switching it to a blank entry with NONE permissions, > then another notifying the new valid mapping. > > Assuming that constraint, Peter's patch is correct. > > I'm pretty uneasy about that constraint, because it's not necessarily > obvious to someone implementing a new vIOMMU device, which is > responsible for triggering the notifies. From just the callback, it > looks like it should be fine to just fire the notify with the new > mapping which replaced the old. > > Peter suggested commenting this next to the IOTLBEntry definition, and > I think that's probably ok for now. I do think we should consider > changing the notify interface to make this more obvious. I can see > one of two ways to do that: > > * Fully allow in-place changes to be notified - the callback would > need to be passed both the new entry and at least the old > permissions, if not the old entry. > > * Instead have separate map and unmap notifier chains with > separate callbacks. That should make it obvious to a vIOMMU > author that an in-place change would need first an unmap > notify, then a map notify. Thanks for the summary! Since we are at this... I am still curious about when we will need this CHANGE interface. Not to talk about Linux kernel, yes we can have other guest OS running. However for any guests, IMHO changing IOMMU PTE is extremely dangerous. For example, if we have mapped an area of memory, e.g. three DMA pages, each 4K (which really doesn't matter): page1 (0-4k) page2 (4k-8k) page3 (8k-12k) If we want to modify the 12K mapping (e.g., change in-place from page1 to page3 in order), the result can be undefined. Since IOMMU might still be using these page mappings during the modification. The problem is that, we cannot do this change for the three pages in an atomic operation. So if IOMMU uses these pages during the modification (e.g., CPU just changed page1, but not yet for page2 and page3), IOMMU will see an inconsistent view of memory. That's trouble. I guess this is why Linux is using unmap_page() and map_page() for it? Or say, we just do not allow to change the content of it directly. Not sure. Also, I assume we may need something like "quiesce" IOMMU operation (or not operation, but existing procedures?) to finally make sure that the pages we are removing will never be touched by IOMMU any more before freeing them. All above is wild guess of me, just want to know when and why we will need this CHANGE stuff. And before we finally realize we need this, I would still suggest to keep the old interface (as long as it can work for us, no extra effort needed), and as David has mentioned, we can add comment for IOMMUTLBEntry to make sure people can know its meaning easier (before starting to read vfio_iommu_map_notify() codes). Thanks, -- peterx