[Qemu-devel] [PATCH v5 0/3] Introduce IOMMUNotifier struct

[Qemu-devel] [PATCH v5 0/3] Introduce IOMMUNotifier struct

[Peter Xu]

(Note: we still have pending discussions on how IOMMU notifier
 interface should be, assuming that is not a blocker for posting v5 of
 this one)

V5:
- squash spapr_tce_notify_{started|stopped} into
  spapr_tce_notify_flag_changed [David]
- in spapr_tce_notify_flag_changed: check flags against "!=
  IOMMU_NOTIFIER_NONE", but not "== IOMMU_NOTIFIER_ALL" [David]
- add r-b for David on patch 3

V4:
- change "notifier_caps" into "notifier_flags" [David]
- rename IOMMU_NOTIFIER_{CHANGE|INVALIDATION} with MAP/UNMAP [David]
- introduce IOMMUOps.notify_flag_changed, to replace notify_started
  and notify_stopped [David, Paolo]

V3:
- use QLIST instead of embedding Notifier into IOMMUNotifier [Paolo]
- fix a build error for ppc64-softmmu

The idea originates from one of Alex's reply:

  https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00254.html

But after further discussions, it seems that only adding a simple type
for notifier is not enough. This series introduced IOMMUNotifier
struct to replace the old Notifier interface. Along with it, we can
provide registration for one (or multiple) of the IOMMU notifications:

- cache invalidations
- entry changes

This is a support material for Jason's vhost dmar patchset.

Please read commit messages for detailed information. Thanks,

Peter Xu (3):
  memory: introduce IOMMUNotifier and its caps
  memory: introduce IOMMUOps.notify_flag_changed
  intel_iommu: allow UNMAP notifiers

 hw/i386/intel_iommu.c         | 18 ++++++++-----
 hw/ppc/spapr_iommu.c          | 18 +++++++------
 hw/vfio/common.c              |  3 ++-
 include/exec/memory.h         | 47 +++++++++++++++++++++++++--------
 include/hw/vfio/vfio-common.h |  2 +-
 memory.c                      | 60 +++++++++++++++++++++++++++++++++----------
 6 files changed, 107 insertions(+), 41 deletions(-)

-- 
2.7.4

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

[Peter Xu]

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 IOMMUNotfierFlag 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_MAP:    for newly mapped entries (additions)
- IOMMU_NOTIFIER_UNMAP:  for entries to be removed (cache invalidates)

When registering the IOMMU notifier, we need to specify one or multiple
types of messages to listen to.

When notifications are triggered, its type will be checked against the
notifier's type bits, and only notifiers with registered bits will be
notified.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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(-)

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(MemoryRegionSection *section)
            section->offset_within_address_space & (1ULL << 63);
 }
 
-static void vfio_iommu_map_notify(Notifier *n, void *data)
+static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data)
 {
     VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
     VFIOContainer *container = giommu->container;
@@ -454,6 +454,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
                                section->offset_within_region;
         giommu->container = container;
         giommu->n.notify = vfio_iommu_map_notify;
+        giommu->n.notifier_flags = 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..e69e984 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_UNMAP = 0x1,
+    /* Notify entry changes (newly created entries) */
+    IOMMU_NOTIFIER_MAP = 0x2,
+} IOMMUNotifierFlag;
+
+#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
+
+struct IOMMUNotifier {
+    void (*notify)(struct IOMMUNotifier *notifier, void *data);
+    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;
 };
 
 /**
@@ -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);
 
 /**
  * memory_region_iommu_replay: replay existing IOMMU translations to
@@ -636,7 +658,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 +669,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..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 = ops,
     mr->terminates = true;  /* then re-forwards */
-    notifier_list_init(&mr->iommu_notify);
+    QLIST_INIT(&mr->iommu_notify);
 }
 
 static void memory_region_finalize(Object *obj)
@@ -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_flags != 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);
 }
 
 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;
@@ -1552,11 +1556,12 @@ 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);
+    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(MemoryRegion *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 = IOMMU_NOTIFIER_MAP;
+    } else {
+        request_flags = IOMMU_NOTIFIER_UNMAP;
+    }
+
+    QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
+        if (iommu_notifier->notifier_flags & request_flags) {
+            iommu_notifier->notify(iommu_notifier, &entry);
+        }
+    }
 }
 
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
-- 
2.7.4

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

[Peter Xu]

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  | 18 ++++++++++--------
 include/exec/memory.h |  9 +++++----
 memory.c              | 29 +++++++++++++++++++++--------
 4 files changed, 40 insertions(+), 22 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);
 
@@ -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 f20b0b8..ae30bbe 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -156,14 +156,17 @@ 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_notify_flag_changed(MemoryRegion *iommu,
+                                          IOMMUNotifierFlag old,
+                                          IOMMUNotifierFlag new)
 {
-    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
-}
+    struct sPAPRTCETable *tbl = container_of(iommu, sPAPRTCETable, iommu);
 
-static void spapr_tce_notify_stopped(MemoryRegion *iommu)
-{
-    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
+    if (old == IOMMU_NOTIFIER_NONE && new != IOMMU_NOTIFIER_NONE) {
+        spapr_tce_set_need_vfio(tbl, true);
+    } else if (old != IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_NONE) {
+        spapr_tce_set_need_vfio(tbl, false);
+    }
 }
 
 static int spapr_tce_table_post_load(void *opaque, int version_id)
@@ -246,8 +249,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..b1e9664 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,
-- 
2.7.4

[Qemu-devel] [PATCH v5 3/3] intel_iommu: allow UNMAP notifiers

[Peter Xu]

Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
Before that is achieved, let's open a door for vhost DMAR support, which
only requires cache invalidations (UNMAP operations).

Meanwhile, converting hw_error() to error_report() and exit(1), to make
the error messages clean and obvious (so no CPU registers will be
dumped).

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9d49be7..e4c3681 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1980,10 +1980,14 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
 
-    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
-             "is currently not supported by intel-iommu emulation",
-             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
-             PCI_FUNC(vtd_as->devfn));
+    if (new & IOMMU_NOTIFIER_MAP) {
+        error_report("Device at bus %s addr %02x.%d requires iommu "
+                     "notifier which is currently not supported by "
+                     "intel-iommu emulation",
+                     vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
+                     PCI_FUNC(vtd_as->devfn));
+        exit(1);
+    }
 }
 
 static const VMStateDescription vtd_vmstate = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 0/3] Introduce IOMMUNotifier struct

[Paolo Bonzini]

On 14/09/2016 10:25, Peter Xu wrote:
> (Note: we still have pending discussions on how IOMMU notifier
>  interface should be, assuming that is not a blocker for posting v5 of
>  this one)
> 
> V5:
> - squash spapr_tce_notify_{started|stopped} into
>   spapr_tce_notify_flag_changed [David]
> - in spapr_tce_notify_flag_changed: check flags against "!=
>   IOMMU_NOTIFIER_NONE", but not "== IOMMU_NOTIFIER_ALL" [David]
> - add r-b for David on patch 3

Looks good, thanks.

If David doesn't reply further I'll merge this.

Paolo

> V4:
> - change "notifier_caps" into "notifier_flags" [David]
> - rename IOMMU_NOTIFIER_{CHANGE|INVALIDATION} with MAP/UNMAP [David]
> - introduce IOMMUOps.notify_flag_changed, to replace notify_started
>   and notify_stopped [David, Paolo]
> 
> V3:
> - use QLIST instead of embedding Notifier into IOMMUNotifier [Paolo]
> - fix a build error for ppc64-softmmu
> 
> The idea originates from one of Alex's reply:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00254.html
> 
> But after further discussions, it seems that only adding a simple type
> for notifier is not enough. This series introduced IOMMUNotifier
> struct to replace the old Notifier interface. Along with it, we can
> provide registration for one (or multiple) of the IOMMU notifications:
> 
> - cache invalidations
> - entry changes
> 
> This is a support material for Jason's vhost dmar patchset.
> 
> Please read commit messages for detailed information. Thanks,
> 
> Peter Xu (3):
>   memory: introduce IOMMUNotifier and its caps
>   memory: introduce IOMMUOps.notify_flag_changed
>   intel_iommu: allow UNMAP notifiers
> 
>  hw/i386/intel_iommu.c         | 18 ++++++++-----
>  hw/ppc/spapr_iommu.c          | 18 +++++++------
>  hw/vfio/common.c              |  3 ++-
>  include/exec/memory.h         | 47 +++++++++++++++++++++++++--------
>  include/hw/vfio/vfio-common.h |  2 +-
>  memory.c                      | 60 +++++++++++++++++++++++++++++++++----------
>  6 files changed, 107 insertions(+), 41 deletions(-)
> 

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

[David Gibson]

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

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.
> 
> 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 IOMMUNotfierFlag 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_MAP:    for newly mapped entries (additions)
> - IOMMU_NOTIFIER_UNMAP:  for entries to be removed (cache invalidates)
> 
> When registering the IOMMU notifier, we need to specify one or multiple
> types of messages to listen to.
> 
> When notifications are triggered, its type will be checked against the
> notifier's type bits, and only notifiers with registered bits will be
> notified.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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(-)
> 
> 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(MemoryRegionSection *section)
>             section->offset_within_address_space & (1ULL << 63);
>  }
>  
> -static void vfio_iommu_map_notify(Notifier *n, void *data)
> +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data)
>  {
>      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
>      VFIOContainer *container = giommu->container;
> @@ -454,6 +454,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>                                 section->offset_within_region;
>          giommu->container = container;
>          giommu->n.notify = vfio_iommu_map_notify;
> +        giommu->n.notifier_flags = 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..e69e984 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_UNMAP = 0x1,
> +    /* Notify entry changes (newly created entries) */
> +    IOMMU_NOTIFIER_MAP = 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;
>  };
>  
>  /**
> @@ -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);
>  
>  /**
>   * memory_region_iommu_replay: replay existing IOMMU translations to
> @@ -636,7 +658,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 +669,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..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 = ops,
>      mr->terminates = true;  /* then re-forwards */
> -    notifier_list_init(&mr->iommu_notify);
> +    QLIST_INIT(&mr->iommu_notify);
>  }
>  
>  static void memory_region_finalize(Object *obj)
> @@ -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_flags != 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);
>  }
>  
>  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;
> @@ -1552,11 +1556,12 @@ 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);
> +    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(MemoryRegion *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 = IOMMU_NOTIFIER_MAP;
> +    } else {
> +        request_flags = IOMMU_NOTIFIER_UNMAP;
> +    }
> +
> +    QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) {
> +        if (iommu_notifier->notifier_flags & request_flags) {
> +            iommu_notifier->notify(iommu_notifier, &entry);
> +        }
> +    }
>  }
>  
>  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)

-- 
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: 819 bytes --]

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

[David Gibson]

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

On Wed, Sep 14, 2016 at 04:25:47PM +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>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/intel_iommu.c |  6 ++++--
>  hw/ppc/spapr_iommu.c  | 18 ++++++++++--------
>  include/exec/memory.h |  9 +++++----
>  memory.c              | 29 +++++++++++++++++++++--------
>  4 files changed, 40 insertions(+), 22 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);
>  
> @@ -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 f20b0b8..ae30bbe 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -156,14 +156,17 @@ 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_notify_flag_changed(MemoryRegion *iommu,
> +                                          IOMMUNotifierFlag old,
> +                                          IOMMUNotifierFlag new)
>  {
> -    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
> -}
> +    struct sPAPRTCETable *tbl = container_of(iommu, sPAPRTCETable, iommu);
>  
> -static void spapr_tce_notify_stopped(MemoryRegion *iommu)
> -{
> -    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
> +    if (old == IOMMU_NOTIFIER_NONE && new != IOMMU_NOTIFIER_NONE) {
> +        spapr_tce_set_need_vfio(tbl, true);
> +    } else if (old != IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_NONE) {
> +        spapr_tce_set_need_vfio(tbl, false);

Need to change those now-wrong "need_vfio" names, but that's not in
scope for this patch of course.

> +    }
>  }
>  
>  static int spapr_tce_table_post_load(void *opaque, int version_id)
> @@ -246,8 +249,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..b1e9664 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: 819 bytes --]

Re: [Qemu-devel] [PATCH v5 3/3] intel_iommu: allow UNMAP notifiers

[David Gibson]

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

On Wed, Sep 14, 2016 at 04:25:48PM +0800, Peter Xu wrote:
> Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> Before that is achieved, let's open a door for vhost DMAR support, which
> only requires cache invalidations (UNMAP operations).
> 
> Meanwhile, converting hw_error() to error_report() and exit(1), to make
> the error messages clean and obvious (so no CPU registers will be
> dumped).

Erm... AIUI currently the intel iommu driver doesn't do any
notifications.  Surely it's only valid to allow this once you've
implemented unmap side notifications.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9d49be7..e4c3681 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1980,10 +1980,14 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>  
> -    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
> -             "is currently not supported by intel-iommu emulation",
> -             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> -             PCI_FUNC(vtd_as->devfn));
> +    if (new & IOMMU_NOTIFIER_MAP) {
> +        error_report("Device at bus %s addr %02x.%d requires iommu "
> +                     "notifier which is currently not supported by "
> +                     "intel-iommu emulation",
> +                     vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> +                     PCI_FUNC(vtd_as->devfn));
> +        exit(1);
> +    }
>  }
>  
>  static const VMStateDescription vtd_vmstate = {

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v5 0/3] Introduce IOMMUNotifier struct

[David Gibson]

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

On Mon, Sep 19, 2016 at 02:47:40PM +0200, Paolo Bonzini wrote:
> 
> 
> On 14/09/2016 10:25, Peter Xu wrote:
> > (Note: we still have pending discussions on how IOMMU notifier
> >  interface should be, assuming that is not a blocker for posting v5 of
> >  this one)
> > 
> > V5:
> > - squash spapr_tce_notify_{started|stopped} into
> >   spapr_tce_notify_flag_changed [David]
> > - in spapr_tce_notify_flag_changed: check flags against "!=
> >   IOMMU_NOTIFIER_NONE", but not "== IOMMU_NOTIFIER_ALL" [David]
> > - add r-b for David on patch 3
> 
> Looks good, thanks.
> 
> If David doesn't reply further I'll merge this.

Sorry, I've been caught up downstream, and didn't have a look at this.

I still have some comments.

> 
> Paolo
> 
> > V4:
> > - change "notifier_caps" into "notifier_flags" [David]
> > - rename IOMMU_NOTIFIER_{CHANGE|INVALIDATION} with MAP/UNMAP [David]
> > - introduce IOMMUOps.notify_flag_changed, to replace notify_started
> >   and notify_stopped [David, Paolo]
> > 
> > V3:
> > - use QLIST instead of embedding Notifier into IOMMUNotifier [Paolo]
> > - fix a build error for ppc64-softmmu
> > 
> > The idea originates from one of Alex's reply:
> > 
> >   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00254.html
> > 
> > But after further discussions, it seems that only adding a simple type
> > for notifier is not enough. This series introduced IOMMUNotifier
> > struct to replace the old Notifier interface. Along with it, we can
> > provide registration for one (or multiple) of the IOMMU notifications:
> > 
> > - cache invalidations
> > - entry changes
> > 
> > This is a support material for Jason's vhost dmar patchset.
> > 
> > Please read commit messages for detailed information. Thanks,
> > 
> > Peter Xu (3):
> >   memory: introduce IOMMUNotifier and its caps
> >   memory: introduce IOMMUOps.notify_flag_changed
> >   intel_iommu: allow UNMAP notifiers
> > 
> >  hw/i386/intel_iommu.c         | 18 ++++++++-----
> >  hw/ppc/spapr_iommu.c          | 18 +++++++------
> >  hw/vfio/common.c              |  3 ++-
> >  include/exec/memory.h         | 47 +++++++++++++++++++++++++--------
> >  include/hw/vfio/vfio-common.h |  2 +-
> >  memory.c                      | 60 +++++++++++++++++++++++++++++++++----------
> >  6 files changed, 107 insertions(+), 41 deletions(-)
> > 
> 

-- 
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: 819 bytes --]

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

[Peter Xu]

On Tue, Sep 20, 2016 at 04:12:05PM +1000, David Gibson wrote:
> 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.
> > 
> > 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 IOMMUNotfierFlag 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_MAP:    for newly mapped entries (additions)
> > - IOMMU_NOTIFIER_UNMAP:  for entries to be removed (cache invalidates)
> > 
> > When registering the IOMMU notifier, we need to specify one or multiple
> > types of messages to listen to.
> > 
> > When notifications are triggered, its type will be checked against the
> > notifier's type bits, and only notifiers with registered bits will be
> > notified.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I still don't see the big fat comment saying that in-place changes to
> an IOMMU mapping aren't permitted.

IMHO if we are using MAP and UNMAP here then it's fairly clear even we
will support in-place change in the future. I can add one more
paragraph for in-place change like:

  For any IOMMU implementation, an in-place mapping change should be
  notified with an UNMAP following a MAP.

Do you think this works?

[...]

> > +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 *.

Yep. I can fix that.

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH v5 3/3] intel_iommu: allow UNMAP notifiers

[Peter Xu]

On Tue, Sep 20, 2016 at 04:14:09PM +1000, David Gibson wrote:
> On Wed, Sep 14, 2016 at 04:25:48PM +0800, Peter Xu wrote:
> > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> > Before that is achieved, let's open a door for vhost DMAR support, which
> > only requires cache invalidations (UNMAP operations).
> > 
> > Meanwhile, converting hw_error() to error_report() and exit(1), to make
> > the error messages clean and obvious (so no CPU registers will be
> > dumped).
> 
> Erm... AIUI currently the intel iommu driver doesn't do any
> notifications.  Surely it's only valid to allow this once you've
> implemented unmap side notifications.

Yes, I suppose vhost DMAR patches will be based upon this one. I can
postpone this patch until Jason wants to pick it up, but it actually
does not hurt if we just enable it now, anyway no one is using it.

So... I see no bad to merge this along with the series, so that we can
reduce one entry from Jason's TODO list. :)

Thanks!

-- peterx

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

[David Gibson]

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

On Tue, Sep 20, 2016 at 03:16:33PM +0800, Peter Xu wrote:
> On Tue, Sep 20, 2016 at 04:12:05PM +1000, David Gibson wrote:
> > 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.
> > > 
> > > 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 IOMMUNotfierFlag 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_MAP:    for newly mapped entries (additions)
> > > - IOMMU_NOTIFIER_UNMAP:  for entries to be removed (cache invalidates)
> > > 
> > > When registering the IOMMU notifier, we need to specify one or multiple
> > > types of messages to listen to.
> > > 
> > > When notifications are triggered, its type will be checked against the
> > > notifier's type bits, and only notifiers with registered bits will be
> > > notified.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > I still don't see the big fat comment saying that in-place changes to
> > an IOMMU mapping aren't permitted.
> 
> IMHO if we are using MAP and UNMAP here then it's fairly clear even we
> will support in-place change in the future. I can add one more
> paragraph for in-place change like:
> 
>   For any IOMMU implementation, an in-place mapping change should be
>   notified with an UNMAP following a MAP.
> 
> Do you think this works?

I guess it will do.

> 
> [...]
> 
> > > +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 *.
> 
> Yep. I can fix that.

Great.

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v5 3/3] intel_iommu: allow UNMAP notifiers

[David Gibson]

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

On Tue, Sep 20, 2016 at 03:28:08PM +0800, Peter Xu wrote:
> On Tue, Sep 20, 2016 at 04:14:09PM +1000, David Gibson wrote:
> > On Wed, Sep 14, 2016 at 04:25:48PM +0800, Peter Xu wrote:
> > > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> > > Before that is achieved, let's open a door for vhost DMAR support, which
> > > only requires cache invalidations (UNMAP operations).
> > > 
> > > Meanwhile, converting hw_error() to error_report() and exit(1), to make
> > > the error messages clean and obvious (so no CPU registers will be
> > > dumped).
> > 
> > Erm... AIUI currently the intel iommu driver doesn't do any
> > notifications.  Surely it's only valid to allow this once you've
> > implemented unmap side notifications.
> 
> Yes, I suppose vhost DMAR patches will be based upon this one. I can
> postpone this patch until Jason wants to pick it up, but it actually
> does not hurt if we just enable it now, anyway no one is using it.

Even so, I think it's misleading to implicitly advertise a capability
that's not yet implemented.  Changing this warning should happen after
or at the same time as implementing the notifications in the vIOMMU
model.

> So... I see no bad to merge this along with the series, so that we can
> reduce one entry from Jason's TODO list. :)
> 
> Thanks!
> 
> -- peterx
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v5 3/3] intel_iommu: allow UNMAP notifiers

[Peter Xu]

On Wed, Sep 21, 2016 at 01:49:10PM +1000, David Gibson wrote:
> On Tue, Sep 20, 2016 at 03:28:08PM +0800, Peter Xu wrote:
> > On Tue, Sep 20, 2016 at 04:14:09PM +1000, David Gibson wrote:
> > > On Wed, Sep 14, 2016 at 04:25:48PM +0800, Peter Xu wrote:
> > > > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> > > > Before that is achieved, let's open a door for vhost DMAR support, which
> > > > only requires cache invalidations (UNMAP operations).
> > > > 
> > > > Meanwhile, converting hw_error() to error_report() and exit(1), to make
> > > > the error messages clean and obvious (so no CPU registers will be
> > > > dumped).
> > > 
> > > Erm... AIUI currently the intel iommu driver doesn't do any
> > > notifications.  Surely it's only valid to allow this once you've
> > > implemented unmap side notifications.
> > 
> > Yes, I suppose vhost DMAR patches will be based upon this one. I can
> > postpone this patch until Jason wants to pick it up, but it actually
> > does not hurt if we just enable it now, anyway no one is using it.
> 
> Even so, I think it's misleading to implicitly advertise a capability
> that's not yet implemented.  Changing this warning should happen after
> or at the same time as implementing the notifications in the vIOMMU
> model.

Sure. Then I'll post v6, and I'm okay with either way to merge it. Thanks.

-- peterx