From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bh8v8-0002vM-JA for qemu-devel@nongnu.org; Tue, 06 Sep 2016 01:27:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bh8v5-0000kX-BV for qemu-devel@nongnu.org; Tue, 06 Sep 2016 01:27:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55270) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bh8v5-0000kS-5u for qemu-devel@nongnu.org; Tue, 06 Sep 2016 01:27:39 -0400 Date: Tue, 6 Sep 2016 13:27:33 +0800 From: Peter Xu Message-ID: <20160906052733.GA21051@pxdev.xzpeter.org> References: <1473060081-17835-1-git-send-email-peterx@redhat.com> <1473060081-17835-3-git-send-email-peterx@redhat.com> <2112298c-fe2a-c74f-7a68-a92625cd3533@redhat.com> <20160905083804.GB7761@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 2/3] memory: add iommu_notify_flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, vkaplans@redhat.com, alex.williamson@redhat.com, wexu@redhat.com, cornelia.huck@de.ibm.com, dgibson@redhat.com On Mon, Sep 05, 2016 at 11:56:12AM +0200, Paolo Bonzini wrote: > Yeah, if you really want to have these semantics, you need to define an > enum like this: > > IOMMU_NOTIFIER_NONE = -1, > IOMMU_NOTIFIER_FLUSH = 0, > IOMMU_NOTIFIER_CHANGED_ENTRY = 1, > > But I'm still not convinced of the exclusivity between "flush" and > "entry changed" notifiers. If I saw the above, my first reaction would > be that you need a bit mask: > > IOMMU_NOTIFIER_NONE = -1, > IOMMU_NOTIFIER_FLUSH = 1, > IOMMU_NOTIFIER_CHANGED_ENTRY = 2, > > But perhaps what you're looking for is to change the "notifier" to a > "listener" like > > struct IOMMUListener { > void (*flush)(IOMMUListener *); > void (*entry_changed)(IOMMUListener *, IOMMUTLBEntry *); > QLIST_ENTRY(IOMMUListener) node; > }; > > The patches can start with an IOMMUListener that only has the > entry_changed callback and that replaces the current use of Notifier. > Then notify_started and notify_stopped can be called on every notifier > that is added/removed (see attached prototype), and the Intel IOMMU can > simply reject registration of a listener that has a non-NULL > iotlb_changed member. Thanks for the quick prototyping. :-) Maybe I haven't explained the idea very clearly, but device-IOTLB is not a "flush" of whole device cache. It still needs a IOMMUTLBEntry, and works just like how general IOMMU invalidations. E.g., we can do device-IOTLB invalidation for a single 4K page. However, I agree with you that the namings are confusing, maybe at least we should introduce IOMMU_NOTIFIER_* macros, though instead of a _FLUSH one, we can have: IOMMU_NOTIFIER_NONE = -1, IOMMU_NOTIFIER_DEVICE_INVALIDATE = 0, IOMMU_NOTIFIER_IOTLB_CHANGED = 1, To clarify that these are two non-overlapped cases. Thanks, -- peterx