Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6948 bytes --]

On Fri, Sep 09, 2016 at 10:57:43AM +0800, Peter Xu wrote:
> The new interface can be used to replace the old notify_started() and
> notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs
> can know what kind of notifications it is requested for.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c |  6 ++++--
>  hw/ppc/spapr_iommu.c  | 14 ++++++++++++--
>  include/exec/memory.h |  9 +++++----
>  memory.c              | 29 +++++++++++++++++++++--------
>  4 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 28c31a2..9d49be7 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
>      return ret;
>  }
>  
> -static void vtd_iommu_notify_started(MemoryRegion *iommu)
> +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
> +                                          IOMMUNotifierFlag old,
> +                                          IOMMUNotifierFlag new)
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);

Shouldn't this have a sanity check that the new flags doesn't include
MAP actions?

> @@ -2348,7 +2350,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.notify_flag_changed = vtd_iommu_notify_flag_changed;
>      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..313f53f 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *iommu)
>      spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
>  }
>  
> +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu,
> +                                          IOMMUNotifierFlag old,
> +                                          IOMMUNotifierFlag new)
> +{
> +    if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) {
> +        spapr_tce_notify_started(iommu);
> +    } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) {
> +        spapr_tce_notify_stopped(iommu);
> +    }

This is wrong.  We need to do the notify_start and stop actions if
*any* bits are set in the new/old flags, not just if all of them are
set.

I'd also prefer to see notify_started and notify_stopped folded into
this function.

> +}
> +
>  static int spapr_tce_table_post_load(void *opaque, int version_id)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> @@ -246,8 +257,7 @@ 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,
> +    .notify_flag_changed = spapr_tce_notify_flag_changed,
>  };
>  
>  static int spapr_tce_table_realize(DeviceState *dev)
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e69e984..e04b752 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -174,10 +174,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 IOMMU Notifier flag changed */
> +    void (*notify_flag_changed)(MemoryRegion *iommu,
> +                                IOMMUNotifierFlag old_flags,
> +                                IOMMUNotifierFlag new_flags);
>  };
>  
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> @@ -223,6 +223,7 @@ struct MemoryRegion {
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
>      QLIST_HEAD(, IOMMUNotifier) iommu_notify;
> +    IOMMUNotifierFlag iommu_notify_flags;
>  };
>  
>  /**
> diff --git a/memory.c b/memory.c
> index f65c600..4faf1d9 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>      mr->iommu_ops = ops,
>      mr->terminates = true;  /* then re-forwards */
>      QLIST_INIT(&mr->iommu_notify);
> +    mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE;
>  }
>  
>  static void memory_region_finalize(Object *obj)
> @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
>      return memory_region_get_dirty_log_mask(mr) & (1 << client);
>  }
>  
> +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr)
> +{
> +    IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE;
> +    IOMMUNotifier *iommu_notifier;
> +
> +    QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
> +        flags |= iommu_notifier->notifier_flags;
> +    }
> +
> +    if (flags != mr->iommu_notify_flags &&
> +        mr->iommu_ops->notify_flag_changed) {
> +        mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags,
> +                                            flags);
> +    }
> +
> +    mr->iommu_notify_flags = flags;
> +}
> +
>  void memory_region_register_iommu_notifier(MemoryRegion *mr,
>                                             IOMMUNotifier *n)
>  {
>      /* We need to register for at least one bitfield */
>      assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
> -    if (mr->iommu_ops->notify_started &&
> -        QLIST_EMPTY(&mr->iommu_notify)) {
> -        mr->iommu_ops->notify_started(mr);
> -    }
>      QLIST_INSERT_HEAD(&mr->iommu_notify, n, node);
> +    memory_region_update_iommu_notify_flags(mr);
>  }
>  
>  uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr)
> @@ -1560,10 +1576,7 @@ 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);
> -    }
> +    memory_region_update_iommu_notify_flags(mr);
>  }
>  
>  void memory_region_notify_iommu(MemoryRegion *mr,

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]