From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmyGZ-0005vh-Tl for qemu-devel@nongnu.org; Thu, 22 Sep 2016 03:17:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmyGV-0004eW-Ms for qemu-devel@nongnu.org; Thu, 22 Sep 2016 03:17:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48722) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmyGV-0004eN-Di for qemu-devel@nongnu.org; Thu, 22 Sep 2016 03:17:51 -0400 Date: Thu, 22 Sep 2016 15:17:46 +0800 From: Peter Xu Message-ID: <20160922071746.GP5134@pxdev.xzpeter.org> References: <1474433936-19617-1-git-send-email-peterx@redhat.com> <1474433936-19617-2-git-send-email-peterx@redhat.com> <20160922052048.GF2085@umbus.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160922052048.GF2085@umbus.fritz.box> Subject: Re: [Qemu-devel] [PATCH v6 1/3] memory: introduce IOMMUNotifier and its caps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, vkaplans@redhat.com, alex.williamson@redhat.com, wexu@redhat.com, pbonzini@redhat.com, cornelia.huck@de.ibm.com, dgibson@redhat.com On Thu, Sep 22, 2016 at 03:20:48PM +1000, David Gibson wrote: > On Wed, Sep 21, 2016 at 12:58:54PM +0800, Peter Xu wrote: > > IOMMU Notifier list is used for notifying IO address mapping changes. > > Currently VFIO is the only user. > > > > However it is possible that future consumer like vhost would like to > > only listen to part of its notifications (e.g., cache invalidations). > > > > This patch introduced IOMMUNotifier and IOMMUNotfierFlag bits for a > > finer grained control of it. > > > > IOMMUNotifier contains a bitfield for the notify consumer describing > > what kind of notification it is interested in. Currently two kinds of > > notifications are defined: > > > > - IOMMU_NOTIFIER_MAP: for newly mapped entries (additions) > > - IOMMU_NOTIFIER_UNMAP: for entries to be removed (cache invalidates) > > > > When registering the IOMMU notifier, we need to specify one or multiple > > types of messages to listen to. > > > > When notifications are triggered, its type will be checked against the > > notifier's type bits, and only notifiers with registered bits will be > > notified. > > > > (For any IOMMU implementation, an in-place mapping change should be > > notified with an UNMAP followed by a MAP.) > > Ok, I wasn't clear. I meant a big fat comment in the *code*, not just > in the commit message. It should not be necessary to look at the > commit history to figure out how to use an interface correctly > > Even a comment in the code is barely adequate, compared to designing > the interface signatures such that it's obvious. > > Please bear in mind: > http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html > and http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html > Thanks for the links. Maybe a better solution is to re-design the IOTLB interface. However that's out of the scope of this series, and another patchset can be opened for the refactoring work IMHO if there is a strong willingness. For now I can add this into comments: ---------8<----------- @@ -607,6 +628,15 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr); /** * memory_region_notify_iommu: notify a change in an IOMMU translation entry. * + * The notification type will be decided by entry.perm bits: + * + * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. + * - For MAP (newly added entry) notifies: set entry.perm to the + * permission of the page (which is definitely !IOMMU_NONE). + * + * Note: for any IOMMU implementation, an in-place mapping change + * should be notified with an UNMAP followed by a MAP. + * * @mr: the memory region that was changed * @entry: the new entry in the IOMMU translation table. The entry * replaces all old entries for the same virtual I/O address range. --------->8----------- [...] > > -static void vfio_iommu_map_notify(Notifier *n, void *data) > > +static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *data) > > This change leaves a now pointless IOMMUTLBEntry *iotlb = data a few > lines below. Yes, will fix. > > > { > > VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n); > > VFIOContainer *container = giommu->container; > > @@ -454,6 +454,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > > section->offset_within_region; > > giommu->container = container; > > giommu->n.notify = vfio_iommu_map_notify; > > + giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > > > > memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 3e4d416..a3ec7aa 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -67,6 +67,27 @@ struct IOMMUTLBEntry { > > IOMMUAccessFlags perm; > > }; > > > > +/* > > + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can > > s/differnet/different/ Will fix. Thanks. -- peterx