From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37833) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmwVG-0003GZ-Pj for qemu-devel@nongnu.org; Thu, 22 Sep 2016 01:25:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmwVC-0000Ni-IN for qemu-devel@nongnu.org; Thu, 22 Sep 2016 01:24:57 -0400 Received: from ozlabs.org ([103.22.144.67]:33210) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmwVB-0000N5-RG for qemu-devel@nongnu.org; Thu, 22 Sep 2016 01:24:54 -0400 Date: Thu, 22 Sep 2016 15:20:48 +1000 From: David Gibson Message-ID: <20160922052048.GF2085@umbus.fritz.box> References: <1474433936-19617-1-git-send-email-peterx@redhat.com> <1474433936-19617-2-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dgjlcl3Tl+kb3YDk" Content-Disposition: inline In-Reply-To: <1474433936-19617-2-git-send-email-peterx@redhat.com> 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: Peter Xu 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 --dgjlcl3Tl+kb3YDk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > However it is possible that future consumer like vhost would like to > only listen to part of its notifications (e.g., cache invalidations). >=20 > This patch introduced IOMMUNotifier and IOMMUNotfierFlag bits for a > finer grained control of it. >=20 > IOMMUNotifier contains a bitfield for the notify consumer describing > what kind of notification it is interested in. Currently two kinds of > notifications are defined: >=20 > - IOMMU_NOTIFIER_MAP: for newly mapped entries (additions) > - IOMMU_NOTIFIER_UNMAP: for entries to be removed (cache invalidates) >=20 > When registering the IOMMU notifier, we need to specify one or multiple > types of messages to listen to. >=20 > When notifications are triggered, its type will be checked against the > notifier's type bits, and only notifiers with registered bits will be > notified. >=20 > (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 Couple of other tiny nits that I wouldn't bother with it weren't for the above. >=20 > Signed-off-by: Peter Xu > --- > hw/vfio/common.c | 3 ++- > include/exec/memory.h | 38 +++++++++++++++++++++++++++++++------- > include/hw/vfio/vfio-common.h | 2 +- > memory.c | 37 ++++++++++++++++++++++++++++--------- > 4 files changed, 62 insertions(+), 18 deletions(-) >=20 > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b313e7c..89a993b 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -293,7 +293,7 @@ static bool vfio_listener_skipped_section(MemoryRegio= nSection *section) > section->offset_within_address_space & (1ULL << 63); > } > =20 > -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 =3D data a few lines below. > { > VFIOGuestIOMMU *giommu =3D container_of(n, VFIOGuestIOMMU, n); > VFIOContainer *container =3D giommu->container; > @@ -454,6 +454,7 @@ static void vfio_listener_region_add(MemoryListener *= listener, > section->offset_within_region; > giommu->container =3D container; > giommu->n.notify =3D vfio_iommu_map_notify; > + giommu->n.notifier_flags =3D IOMMU_NOTIFIER_ALL; > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > =20 > 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; > }; > =20 > +/* > + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can s/differnet/different/ > + * register with one or multiple IOMMU Notifier capability bit(s). > + */ > +typedef enum { > + IOMMU_NOTIFIER_NONE =3D 0, > + /* Notify cache invalidations */ > + IOMMU_NOTIFIER_UNMAP =3D 0x1, > + /* Notify entry changes (newly created entries) */ > + IOMMU_NOTIFIER_MAP =3D 0x2, > +} IOMMUNotifierFlag; > + > +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) > + > +struct IOMMUNotifier { > + void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data); > + IOMMUNotifierFlag notifier_flags; > + QLIST_ENTRY(IOMMUNotifier) node; > +}; > +typedef struct IOMMUNotifier IOMMUNotifier; > + > /* New-style MMIO accessors can indicate that the transaction failed. > * A zero (MEMTX_OK) response means success; anything else is a failure > * of some kind. The memory subsystem will bitwise-OR together results > @@ -201,7 +222,7 @@ struct MemoryRegion { > const char *name; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > - NotifierList iommu_notify; > + QLIST_HEAD(, IOMMUNotifier) iommu_notify; > }; > =20 > /** > @@ -620,11 +641,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, > * IOMMU translation entries. > * > * @mr: the memory region to observe > - * @n: the notifier to be added; the notifier receives a pointer to an > - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > - * valid on exit from the notifier. > + * @n: the IOMMUNotifier to be added; the notify callback receives a > + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer > + * ceases to be valid on exit from the notifier. > */ > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n= ); > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n); > =20 > /** > * memory_region_iommu_replay: replay existing IOMMU translations to > @@ -636,7 +658,8 @@ void memory_region_register_iommu_notifier(MemoryRegi= on *mr, Notifier *n); > * @is_write: Whether to treat the replay as a translate "write" > * through the iommu > */ > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_w= rite); > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > + bool is_write); > =20 > /** > * memory_region_unregister_iommu_notifier: unregister a notifier for > @@ -646,7 +669,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Not= ifier *n, bool is_write); > * needs to be called > * @n: the notifier to be removed. > */ > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier = *n); > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n); > =20 > /** > * memory_region_name: get a memory region's name > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 94dfae3..c17602e 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -93,7 +93,7 @@ typedef struct VFIOGuestIOMMU { > VFIOContainer *container; > MemoryRegion *iommu; > hwaddr iommu_offset; > - Notifier n; > + IOMMUNotifier n; > QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > } VFIOGuestIOMMU; > =20 > diff --git a/memory.c b/memory.c > index 1a1baf5..69d9d9a 100644 > --- a/memory.c > +++ b/memory.c > @@ -1413,7 +1413,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > memory_region_init(mr, owner, name, size); > mr->iommu_ops =3D ops, > mr->terminates =3D true; /* then re-forwards */ > - notifier_list_init(&mr->iommu_notify); > + QLIST_INIT(&mr->iommu_notify); > } > =20 > static void memory_region_finalize(Object *obj) > @@ -1508,13 +1508,16 @@ bool memory_region_is_logging(MemoryRegion *mr, u= int8_t client) > return memory_region_get_dirty_log_mask(mr) & (1 << client); > } > =20 > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n) > { > + /* We need to register for at least one bitfield */ > + assert(n->notifier_flags !=3D IOMMU_NOTIFIER_NONE); > if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + QLIST_EMPTY(&mr->iommu_notify)) { > mr->iommu_ops->notify_started(mr); > } > - notifier_list_add(&mr->iommu_notify, n); > + QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > } > =20 > uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > @@ -1526,7 +1529,8 @@ uint64_t memory_region_iommu_get_min_page_size(Memo= ryRegion *mr) > return TARGET_PAGE_SIZE; > } > =20 > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_w= rite) > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > + bool is_write) > { > hwaddr addr, granularity; > IOMMUTLBEntry iotlb; > @@ -1547,11 +1551,12 @@ void memory_region_iommu_replay(MemoryRegion *mr,= Notifier *n, bool is_write) > } > } > =20 > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier = *n) > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n) > { > - notifier_remove(n); > + QLIST_REMOVE(n, node); > if (mr->iommu_ops->notify_stopped && > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + QLIST_EMPTY(&mr->iommu_notify)) { > mr->iommu_ops->notify_stopped(mr); > } > } > @@ -1559,8 +1564,22 @@ void memory_region_unregister_iommu_notifier(Memor= yRegion *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 =3D IOMMU_NOTIFIER_MAP; > + } else { > + request_flags =3D IOMMU_NOTIFIER_UNMAP; > + } > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + if (iommu_notifier->notifier_flags & request_flags) { > + iommu_notifier->notify(iommu_notifier, &entry); > + } > + } > } > =20 > void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --dgjlcl3Tl+kb3YDk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX42otAAoJEGw4ysog2bOSXZgP/A9Kak7pOn4AOTspUOLFtJx+ u0ABEKjg7etEo378fBSR8bLANlSqEQxmZ0pNbKSGseHKn5PE/ootGdH8wbiQ2C2C P86ocjuMzSIQJ9IzWF0mW2Uldt/hbdz8wuj1Vj0FssgFWj1nw1QzgmZDmnPL+1DC 3oOHyP6A+xnfAKUJ60c/D32BmZ2Ag1yE3nXFKVWPv9HI2WngxEhP83QizkrjugFj ZX+v+fHqf2zjNGmNmodrolGq1kO8UuG0ki+ZSbkDSmokZZM2xQ126y9R60DRdCIT 5jaRz3e7d3PImg53SmGhOJMRUmQBYqyhRYTnDRXhRzhzBA7WfcPUqcwURbk7porL tMKJxYHoj1XmKmmxWjdLpDW/hofQYwLOfsdnIZXjUEGnPHPMb2tCYra8ZLEisu4G /Mgjo3PjMq8n31aPVslSeh50cPEY6n3UQeABBvaZEZ92B/SYry3P2hUUjS2UHsYP cuHxSqUgv7+VzFBUvuT/lzcA5gox1kNwVYVA0sjjZ6omD75HSNxHAu8Vm/BfaVoD udmrfsOt5Y3VwnhU0wfbfCTIT4fuV6u2G1V0V1IocYzsWXbubPnizESrvpQbBzFj C6uIHalMcL5y7vZOUrSrIYKbqbKZQ3qMW0tNUlyDK9DvqLeW+nt7ZzQocJIVPwUM 44O2n9dFgXvd/Wjwyj0f =WOww -----END PGP SIGNATURE----- --dgjlcl3Tl+kb3YDk--