From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhVxl-0005zY-U2 for qemu-devel@nongnu.org; Wed, 07 Sep 2016 02:03:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhVxj-00016H-3X for qemu-devel@nongnu.org; Wed, 07 Sep 2016 02:03:56 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:34462) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhVxh-00015b-KE for qemu-devel@nongnu.org; Wed, 07 Sep 2016 02:03:55 -0400 Date: Wed, 7 Sep 2016 16:02:39 +1000 From: David Gibson Message-ID: <20160907060239.GP2780@voom.fritz.box> References: <1473226344-28520-1-git-send-email-peterx@redhat.com> <1473226344-28520-2-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="cmVHo2jXx4bdYlgS" Content-Disposition: inline In-Reply-To: <1473226344-28520-2-git-send-email-peterx@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 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 --cmVHo2jXx4bdYlgS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 07, 2016 at 01:32:22PM +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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) > - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) As noted on the other thread, I think the correct options for your bitmap here are "map" and "unmap". Which are triggered depends on the permissions / existence of the *previous* mapping, as well as the new one. You could in fact have "map-read", "map-write", "unmap-read", "unmap-write" as separate bitmap options (e.g. changing a mapping from RO to WO would be both a read-unmap and write-map event). I can't see any real use for that though, so just "map" and "unmap" are probably sufficient. > When registering the IOMMU notifier, we need to specify one or multiple > capability bit(s) to listen to. >=20 > When notifications are triggered, it will be checked against the > notifier's capability bits, and only notifiers with registered bits will > be notified. >=20 > Signed-off-by: Peter Xu > --- > hw/vfio/common.c | 3 ++- > include/exec/memory.h | 39 ++++++++++++++++++++++++++++++++-----= -- > include/hw/vfio/vfio-common.h | 2 +- > memory.c | 37 ++++++++++++++++++++++++++++--------- > 4 files changed, 63 insertions(+), 18 deletions(-) >=20 > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b313e7c..b0cea2c 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, void *data) > { > 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_caps =3D IOMMU_NOTIFIER_ALL; "caps" isn't really right. It's a *requirement* that VFIO get all the notifications, not a capability. "caps" would only make sense on the other side (the vIOMMU implementation). > 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..92f14db 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -67,6 +67,28 @@ struct IOMMUTLBEntry { > IOMMUAccessFlags perm; > }; > =20 > +/* > + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can > + * register with one or multiple IOMMU Notifier capability bit(s). > + */ > +typedef enum { > + IOMMU_NOTIFIER_NONE =3D 0, > + /* Notify cache invalidations */ > + IOMMU_NOTIFIER_INVALIDATION =3D 0x1, > + /* Notify entry changes (newly created entries) */ > + IOMMU_NOTIFIER_CHANGE =3D 0x2, > +} IOMMUNotifierCap; > + > +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_INVALIDATION | \ > + IOMMU_NOTIFIER_CHANGE) > + > +struct IOMMUNotifier { > + void (*notify)(struct IOMMUNotifier *notifier, void *data); > + IOMMUNotifierCap notifier_caps; > + 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 +223,7 @@ struct MemoryRegion { > const char *name; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > - NotifierList iommu_notify; > + QLIST_HEAD(, IOMMUNotifier) iommu_notify; > }; > =20 > /** > @@ -620,11 +642,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); It seems to me that this should be allowed to fail, if the notifier you're trying to register requires notifications that the MR implementation can't supply. That seems cleaner than delaying the checking until the notification actually happens. > /** > * memory_region_iommu_replay: replay existing IOMMU translations to > @@ -636,7 +659,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 +670,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 0eb6895..45a3902 100644 > --- a/memory.c > +++ b/memory.c > @@ -1418,7 +1418,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) > @@ -1513,13 +1513,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_caps !=3D IOMMU_NOTIFIER_NONE); Not sure if it makes sense to implement NOTIFIER_NONE as a no-op just for orthogonality. > if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + QLIST_EMPTY(&mr->iommu_notify)) { > mr->iommu_ops->notify_started(mr); As noted above, I think register_notify should get the ability to fail, which would happen if notify_started() failed (obviously it needs to get a failure mode as well. Basically notify_started is required to check that this vIOMMU is able to supply the notifications that have been requested. > } > - 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) > @@ -1531,7 +1534,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; > @@ -1552,11 +1556,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); > } > } > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(Memor= yRegion *mr, Notifier *n) > void memory_region_notify_iommu(MemoryRegion *mr, > IOMMUTLBEntry entry) > { > + IOMMUNotifier *iommu_notifier; > + IOMMUNotifierCap request_cap; > + > assert(memory_region_is_iommu(mr)); > - notifier_list_notify(&mr->iommu_notify, &entry); > + > + if (entry.perm & IOMMU_RW) { > + request_cap =3D IOMMU_NOTIFIER_CHANGE; > + } else { > + request_cap =3D IOMMU_NOTIFIER_INVALIDATION; > + } As noted right at the top, I don't think this logic is really right. An in-place change should be treated as both a map and unmap. > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + if (iommu_notifier->notifier_caps & request_cap) { > + 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 --cmVHo2jXx4bdYlgS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJXz61/AAoJEGw4ysog2bOS8a4QAMrhouWbUqffcZ3C8uQPrmGN AJ0YkhRVLi7RPose5R85nyV4oCgyx8T1NNsRb2Pv51u+6OkeRY717Las6ofyYr0i viCwDdRIjSBbULZibKyxncsIScBqUh9cDkIEZCsGlQkuzfsYWQwIszijBJJm02xY SRQDn3J2h3q2XeufPq8Rkqg7qEz9fr58YVl7obbUUCiLWZwc3pg2iyKRsAYTnMTm GhMqpG9d+CrdUhxmDkvHVQTaBeDvCg9DYl/KPjeSfKV8yWWuIezxvqYV2G5Vigm2 viLXJxXD0bCH7jOII5/8gMb7ZrNzAZh9fHv0krhwn/mD4GSqxeo59VevIx61Yz5n PBSeh+4w4BeTeNlz4Zfu9HZ3D9www6kULnMyAAK3Exw2K7jv49thBZinOcRLa0DZ WSAMKFYrToFjztkU6grV7iPzrK8nadvUvo4UoV24f852/ToftoPIRfHYq5ul6Om1 B1g4fHWm1NXwcky1xq1gJer9X2vzGvI5gfWByFVLbnjCzh+NWJHLckHfGGz4mm/f dp3jEomRpjUpTDiRV4h8dy9cUq7QHbkP9HqYFMsZpdeGrkpjnw7X1JB6sn/t1wQj qExg+0J+ni9FgokA70/qoz3no3WWUYwUeor1m5DDcywP9Cy6NTTs4i5onkobRfAn n0Vls5Xcxj4Bfj9gBxYz =OjKl -----END PGP SIGNATURE----- --cmVHo2jXx4bdYlgS--