From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmELP-0006ri-3X for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:15:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmELL-00006p-0O for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:15:50 -0400 Received: from ozlabs.org ([103.22.144.67]:57457) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmELK-0008Uy-9A for qemu-devel@nongnu.org; Tue, 20 Sep 2016 02:15:46 -0400 Date: Tue, 20 Sep 2016 16:12:05 +1000 From: David Gibson Message-ID: <20160920061205.GV20488@umbus> References: <1473841548-26742-1-git-send-email-peterx@redhat.com> <1473841548-26742-2-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="r+TdwuXy+OXS8TUs" Content-Disposition: inline In-Reply-To: <1473841548-26742-2-git-send-email-peterx@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 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 --r+TdwuXy+OXS8TUs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 14, 2016 at 04:25:46PM +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 > Signed-off-by: Peter Xu I still don't see the big fat comment saying that in-place changes to an IOMMU mapping aren't permitted. > --- > 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..41b6a13 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_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..e69e984 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 > + * 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, void *data); Given that we now have a special notifier type for this purpose, we could actually type this to take an IOMMUTLBEntry instead of a void *. > + 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 0eb6895..f65c600 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_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) > @@ -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; > + 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 --r+TdwuXy+OXS8TUs Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX4NMxAAoJEGw4ysog2bOSI+oP/1nTJ/dhGQVhzOy0NhQxHm/q 49F5kv7YpAuso/QSy4knZOPLYix9uUBR0bE8hwSEcZSM/r1p2CTGRSJvaTnwBvVE RnKqgdBUchOUovd8WAmvJku9h8GlC+PLNfnHgpY5KZFlGBYUMrntBatHw4QMhVRg O3eezZcYlKzjAy9aONFIl1dE81DGl3QbEH0L1B2ACqL/X3+fIF3jhNw7R8C9kIoJ t0uJNNCoUq3V27gEGgDvpJvCrxvzIlOBsaaUgWc9/LT+EjkP/1J3WD+8JGX1E6Xh FzMkWU0tsDLuAEzNvj9cEZqUUAcHs2ivOfVFgl8ZKK6605MN0kmuh5Arrq9yVdCM zemZkE8xY0JDD4Kcag0G/m7YrWJHY5tdUXf0tObJusehfyi3SmZFRnhbLhgbUuz/ WAP9P36xRf1jNo2yJNX+DS/c4fjPaEWWUC2hEwjD0NLzhTaqEwLhIAG0jb0FgzVA jYleRZVbP1AiVTPELO21Zo58IszS8/LFWE4I3LieagrtcCw2oM+gjotySgp1xyxu s1+Fja7ZHamHf40/sfjkwZ/bc/svf5GYM8FQxmQqn0zh6lSX+qoBkfgaMHr9bDo8 9arSD2hmONkW++EMPP2W4ZhIyksWWYITpT/O8XiyKXYgRDuf4Fg9ZYyVSGy8CKwN ktmcuoDG+8zGBpXTsptA =7rZe -----END PGP SIGNATURE----- --r+TdwuXy+OXS8TUs--