On 06/09/2016 15:24, 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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions)
> - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates)
>
> When registering the IOMMU notifier, we need to specify one or multiple
> capability bit(s) to listen to.
>
> When notifications are triggered, it will be checked against the
> notifier's capability bits, and only notifiers with registered bits will
> be notified.
Since you're walking the notifier list manually anyway, I think it's
simpler to get rid of Notifier completely. Otherwise, this looks pretty
good!
Paolo
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> hw/vfio/common.c | 5 +++--
> include/exec/memory.h | 37 +++++++++++++++++++++++++++++++------
> include/hw/vfio/vfio-common.h | 2 +-
> memory.c | 35 ++++++++++++++++++++++++++++-------
> 4 files changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b313e7c..04e1cb4 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -295,7 +295,7 @@ static bool
> vfio_listener_skipped_section(MemoryRegionSection *section)
>
> static void vfio_iommu_map_notify(Notifier *n, void *data)
> {
> - VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> + VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n.notifier);
> VFIOContainer *container = giommu->container;
> IOMMUTLBEntry *iotlb = data;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> @@ -453,7 +453,8 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> giommu->iommu_offset = section->offset_within_address_space -
> section->offset_within_region;
> giommu->container = container;
> - giommu->n.notify = vfio_iommu_map_notify;
> + giommu->n.notifier.notify = vfio_iommu_map_notify;
> + giommu->n.notifier_caps = 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..52914c1 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
> + * register with one or multiple IOMMU Notifier capability bit(s).
> + */
> +typedef enum {
> + IOMMU_NOTIFIER_NONE = 0,
> + /* Notify cache invalidations */
> + IOMMU_NOTIFIER_INVALIDATION = 0x1,
> + /* Notify entry changes (newly created entries) */
> + IOMMU_NOTIFIER_CHANGE = 0x2,
> +} IOMMUNotifierCap;
> +
> +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_INVALIDATION | \
> + IOMMU_NOTIFIER_CHANGE)
> +
> +struct IOMMUNotifier {
> + Notifier notifier;
> + IOMMUNotifierCap notifier_caps;
> +};
> +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
> @@ -620,11 +641,13 @@ 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 Notifier within the
> + * IOMMUNotifier 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);
>
> /**
> * memory_region_iommu_replay: replay existing IOMMU translations to
> @@ -636,7 +659,8 @@ void memory_region_register_iommu_notifier(MemoryRegion
> *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_write);
> +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
> + bool is_write);
>
> /**
> * memory_region_unregister_iommu_notifier: unregister a notifier for
> @@ -646,7 +670,8 @@ void memory_region_iommu_replay(MemoryRegion *mr,
> Notifier *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);
>
> /**
> * 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;
>
> diff --git a/memory.c b/memory.c
> index 0eb6895..f513c5a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1513,13 +1513,16 @@ bool memory_region_is_logging(MemoryRegion *mr,
> uint8_t client)
> return memory_region_get_dirty_log_mask(mr) & (1 << client);
> }
>
> -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 != IOMMU_NOTIFIER_NONE);
> if (mr->iommu_ops->notify_started &&
> QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
> mr->iommu_ops->notify_started(mr);
> }
> - notifier_list_add(&mr->iommu_notify, n);
> + notifier_list_add(&mr->iommu_notify, &n->notifier);
> }
>
> uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr)
> @@ -1531,7 +1534,8 @@ uint64_t
> memory_region_iommu_get_min_page_size(MemoryRegion *mr)
> return TARGET_PAGE_SIZE;
> }
>
> -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write)
> +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n,
> + bool is_write)
> {
> hwaddr addr, granularity;
> IOMMUTLBEntry iotlb;
> @@ -1541,7 +1545,7 @@ void memory_region_iommu_replay(MemoryRegion *mr,
> Notifier *n, bool is_write)
> for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> iotlb = mr->iommu_ops->translate(mr, addr, is_write);
> if (iotlb.perm != IOMMU_NONE) {
> - n->notify(n, &iotlb);
> + n->notifier.notify(&n->notifier, &iotlb);
> }
>
> /* if (2^64 - MR size) < granularity, it's possible to get an
> @@ -1552,9 +1556,10 @@ void memory_region_iommu_replay(MemoryRegion *mr,
> Notifier *n, bool is_write)
> }
> }
>
> -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
> +void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
> + IOMMUNotifier *n)
> {
> - notifier_remove(n);
> + notifier_remove(&n->notifier);
> if (mr->iommu_ops->notify_stopped &&
> QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
> mr->iommu_ops->notify_stopped(mr);
> @@ -1564,8 +1569,24 @@ void
> memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
> void memory_region_notify_iommu(MemoryRegion *mr,
> IOMMUTLBEntry entry)
> {
> + IOMMUNotifier *iommu_notify;
> + IOMMUNotifierCap request_cap;
> + Notifier *notifier, *next;
> +
> assert(memory_region_is_iommu(mr));
> - notifier_list_notify(&mr->iommu_notify, &entry);
> +
> + if (entry.perm & IOMMU_RW) {
> + request_cap = IOMMU_NOTIFIER_CHANGE;
> + } else {
> + request_cap = IOMMU_NOTIFIER_INVALIDATION;
> + }
> +
> + QLIST_FOREACH_SAFE(notifier, &mr->iommu_notify.notifiers, node, next) {
> + iommu_notify = container_of(notifier, IOMMUNotifier, notifier);
> + if (iommu_notify->notifier_caps & request_cap) {
> + notifier->notify(notifier, &entry);
> + }
> + }
> }
>
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>
