Re: [Qemu-devel] [PATCH v3 2/3] memory: generalize iommu_ops.notify_started to notifier_add

[David Gibson]

On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote:
> Considering that we may have multiple IOMMU notifier consumers in the
> future, converting iommu_ops.notify_{started|stopped} into some more
> general form. Now we can trap all notifier registerations and
> deregistrations, rather than only the first ones.
> 
> Power was leveraging the notifier_{started|stopped}, adding iommu_user
> field for counting on Power guests to achieve the same goal.

Requiring each vIOMMU frontend to reference count or whatever seems
like a pain.  The semantics of notify_started() were designed to avoid
that.

Instead I'd suggest a callback which gets tripped any time the logical
OR of the requested notifications for all current notifiers changes.

> Suggested-by:  Paolo Bonzini <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/i386/intel_iommu.c  |  4 ++--
>  hw/ppc/spapr_iommu.c   | 18 ++++++++++++------
>  include/exec/memory.h  |  8 ++++----
>  include/hw/ppc/spapr.h |  1 +
>  memory.c               | 10 ++++------
>  5 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 28c31a2..c6bd8f6 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1974,7 +1974,7 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion 
> *iommu, hwaddr addr,
>      return ret;
>  }
>  
> -static void vtd_iommu_notify_started(MemoryRegion *iommu)
> +static void vtd_iommu_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n)
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>  
> @@ -2348,7 +2348,7 @@ static void vtd_init(IntelIOMMUState *s)
>      memset(s->womask, 0, DMAR_REG_SIZE);
>  
>      s->iommu_ops.translate = vtd_iommu_translate;
> -    s->iommu_ops.notify_started = vtd_iommu_notify_started;
> +    s->iommu_ops.notifier_add = vtd_iommu_notifier_add;
>      s->root = 0;
>      s->root_extended = false;
>      s->dmar_enabled = false;
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 6bc4d4d..99c83a4 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -156,14 +156,20 @@ static uint64_t 
> spapr_tce_get_min_page_size(MemoryRegion *iommu)
>      return 1ULL << tcet->page_shift;
>  }
>  
> -static void spapr_tce_notify_started(MemoryRegion *iommu)
> +static void spapr_tce_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n)
>  {
> -    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
> +    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
> +    if (tcet->iommu_users++ == 0) {
> +        spapr_tce_set_need_vfio(tcet, true);
> +    }
>  }
>  
> -static void spapr_tce_notify_stopped(MemoryRegion *iommu)
> +static void spapr_tce_notifier_del(MemoryRegion *iommu, IOMMUNotifier *n)
>  {
> -    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), 
> false);
> +    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
> +    if (--tcet->iommu_users == 0) {
> +        spapr_tce_set_need_vfio(tcet, false);
> +    }
>  }
>  
>  static int spapr_tce_table_post_load(void *opaque, int version_id)
> @@ -246,8 +252,8 @@ static const VMStateDescription vmstate_spapr_tce_table = 
> {
>  static MemoryRegionIOMMUOps spapr_iommu_ops = {
>      .translate = spapr_tce_translate_iommu,
>      .get_min_page_size = spapr_tce_get_min_page_size,
> -    .notify_started = spapr_tce_notify_started,
> -    .notify_stopped = spapr_tce_notify_stopped,
> +    .notifier_add = spapr_tce_notifier_add,
> +    .notifier_del = spapr_tce_notifier_del,
>  };
>  
>  static int spapr_tce_table_realize(DeviceState *dev)
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 92f14db..9efcf1b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -175,10 +175,10 @@ struct MemoryRegionIOMMUOps {
>      IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool 
> is_write);
>      /* Returns minimum supported page size */
>      uint64_t (*get_min_page_size)(MemoryRegion *iommu);
> -    /* Called when the first notifier is set */
> -    void (*notify_started)(MemoryRegion *iommu);
> -    /* Called when the last notifier is removed */
> -    void (*notify_stopped)(MemoryRegion *iommu);
> +    /* Called when someone registers to the notify list */
> +    void (*notifier_add)(MemoryRegion *iommu, IOMMUNotifier *n);
> +    /* Called when someone unregisters from the notify list */
> +    void (*notifier_del)(MemoryRegion *iommu, IOMMUNotifier *n);
>  };
>  
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index caf7be9..08627ec 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -564,6 +564,7 @@ struct sPAPRTCETable {
>      MemoryRegion root, iommu;
>      struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only 
> */
>      QLIST_ENTRY(sPAPRTCETable) list;
> +    int iommu_users;
>  };
>  
>  sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
> diff --git a/memory.c b/memory.c
> index 45a3902..d913043 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1518,9 +1518,8 @@ void memory_region_register_iommu_notifier(MemoryRegion 
> *mr,
>  {
>      /* 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)) {
> -        mr->iommu_ops->notify_started(mr);
> +    if (mr->iommu_ops->notifier_add) {
> +        mr->iommu_ops->notifier_add(mr, n);
>      }
>      QLIST_INSERT_HEAD(&mr->iommu_notify, n, node);
>  }
> @@ -1560,9 +1559,8 @@ void 
> memory_region_unregister_iommu_notifier(MemoryRegion *mr,
>                                               IOMMUNotifier *n)
>  {
>      QLIST_REMOVE(n, node);
> -    if (mr->iommu_ops->notify_stopped &&
> -        QLIST_EMPTY(&mr->iommu_notify)) {
> -        mr->iommu_ops->notify_stopped(mr);
> +    if (mr->iommu_ops->notifier_del) {
> +        mr->iommu_ops->notifier_del(mr, n);
>      }
>  }
>  

-- 
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

signature.asc
Description: PGP signature