Re: [Qemu-devel] [PATCH v2 1/3] memory: introduce IOMMUNotifier and its caps

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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)
>