[Qemu-devel] [RFC 0/3] memory: enhance IOMMU notifier to support USER bit

[Qemu-devel] [RFC 0/3] memory: enhance IOMMU notifier to support USER bit

[Peter Xu]

NOTE: This is a continuous discussion in thread but in patch format:

https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00824.html

Hi, Peter,

Please have a look on whether this tiny series will work for your TCG
work.  I only tested it with x86 with existing qtests.  No further
test is done.  I only want to show what I meant to pass in MemTxAttrs
instead of the new IOMMU index logic.

I only used the USER bit as an example.  Of course in the future we
can add more bits in the MemTxAttrs to monitor upon.  And the series
only provided the interface change, no real use case is provided.

It's very possible that I misunderstood some of the requirement of the
TCG work; please feel free to point it out where I missed.

Thanks,

Peter Xu (3):
  memory: add MemTxAttrs to translate function
  memory: add MemTxAttrs to IOMMUTLBEntry
  memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

 include/exec/memory.h    | 60 ++++++++++++++++++++++++++++++++++++----
 exec.c                   |  2 +-
 hw/alpha/typhoon.c       |  3 +-
 hw/arm/smmuv3.c          |  2 +-
 hw/dma/rc4030.c          |  6 ++--
 hw/i386/amd_iommu.c      |  2 +-
 hw/i386/intel_iommu.c    |  6 ++--
 hw/ppc/spapr_iommu.c     |  3 +-
 hw/s390x/s390-pci-bus.c  |  6 ++--
 hw/sparc/sun4m_iommu.c   |  3 +-
 hw/sparc64/sun4u_iommu.c |  3 +-
 hw/virtio/vhost.c        |  2 +-
 memory.c                 | 13 ++++++++-
 13 files changed, 90 insertions(+), 21 deletions(-)

-- 
2.17.0

[Qemu-devel] [RFC 1/3] memory: add MemTxAttrs to translate function

[Peter Xu]

Add a new MemTxAttrs parameter to the IOMMUMemoryRegionClass.translate()
function, which takes some extra context of the translation request.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h    | 5 ++++-
 exec.c                   | 2 +-
 hw/alpha/typhoon.c       | 3 ++-
 hw/arm/smmuv3.c          | 2 +-
 hw/dma/rc4030.c          | 6 ++++--
 hw/i386/amd_iommu.c      | 2 +-
 hw/i386/intel_iommu.c    | 6 ++++--
 hw/ppc/spapr_iommu.c     | 3 ++-
 hw/s390x/s390-pci-bus.c  | 6 ++++--
 hw/sparc/sun4m_iommu.c   | 3 ++-
 hw/sparc64/sun4u_iommu.c | 3 ++-
 memory.c                 | 3 ++-
 12 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index eb2ba06519..6b0ced554d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -234,9 +234,12 @@ typedef struct IOMMUMemoryRegionClass {
      * @iommu: the IOMMUMemoryRegion
      * @hwaddr: address to be translated within the memory region
      * @flag: requested access permissions
+     * @attrs: MemTxAttrs that was bound to the translation
+     *         operation. If flag==IOMMU_NONE, this field is
+     *         meaningless
      */
     IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr,
-                               IOMMUAccessFlags flag);
+                               IOMMUAccessFlags flag, MemTxAttrs attrs);
     /* Returns minimum supported page size in bytes.
      * If this method is not provided then the minimum is assumed to
      * be TARGET_PAGE_SIZE.
diff --git a/exec.c b/exec.c
index f6645ede0c..a0808ce9bd 100644
--- a/exec.c
+++ b/exec.c
@@ -502,7 +502,7 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm
         hwaddr addr = *xlat;
         IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
         IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ?
-                                              IOMMU_WO : IOMMU_RO);
+                                              IOMMU_WO : IOMMU_RO, attrs);
 
         if (!(iotlb.perm & (1 << is_write))) {
             goto unassigned;
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 6a40869488..49192ab24d 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -666,7 +666,8 @@ static bool window_translate(TyphoonWindow *win, hwaddr addr,
    Pchip and generate a machine check interrupt.  */
 static IOMMUTLBEntry typhoon_translate_iommu(IOMMUMemoryRegion *iommu,
                                              hwaddr addr,
-                                             IOMMUAccessFlags flag)
+                                             IOMMUAccessFlags flag,
+                                             MemTxAttrs attrs)
 {
     TyphoonPchip *pchip = container_of(iommu, TyphoonPchip, iommu);
     IOMMUTLBEntry ret;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 42dc521c13..f50d31c9d1 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -538,7 +538,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
 }
 
 static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
-                                      IOMMUAccessFlags flag)
+                                      IOMMUAccessFlags flag, MemTxAttrs attrs)
 {
     SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
     SMMUv3State *s = sdev->smmu;
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 5d4833eeca..1989bea771 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -490,8 +490,10 @@ static const MemoryRegionOps jazzio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static IOMMUTLBEntry rc4030_dma_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
-                                          IOMMUAccessFlags flag)
+static IOMMUTLBEntry rc4030_dma_translate(IOMMUMemoryRegion *iommu,
+                                          hwaddr addr,
+                                          IOMMUAccessFlags flag,
+                                          MemTxAttrs attrs)
 {
     rc4030State *s = container_of(iommu, rc4030State, dma_mr);
     IOMMUTLBEntry ret = {
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 63d46ff6ee..084bfb7024 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -991,7 +991,7 @@ static inline bool amdvi_is_interrupt_addr(hwaddr addr)
 }
 
 static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
-                                     IOMMUAccessFlags flag)
+                                     IOMMUAccessFlags flag, MemTxAttrs attrs)
 {
     AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
     AMDVIState *s = as->iommu_state;
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b5a09b7908..ffbf19e257 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2470,8 +2470,10 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
     }
 }
 
-static IOMMUTLBEntry vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
-                                         IOMMUAccessFlags flag)
+static IOMMUTLBEntry vtd_iommu_translate(IOMMUMemoryRegion *iommu,
+                                         hwaddr addr,
+                                         IOMMUAccessFlags flag,
+                                         MemTxAttrs attrs)
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
     IntelIOMMUState *s = vtd_as->iommu_state;
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index aaa6010d5c..199612095a 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -112,7 +112,8 @@ static void spapr_tce_free_table(uint64_t *table, int fd, uint32_t nb_table)
 /* Called from RCU critical section */
 static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
                                                hwaddr addr,
-                                               IOMMUAccessFlags flag)
+                                               IOMMUAccessFlags flag,
+                                               MemTxAttrs attrs)
 {
     sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
     uint64_t tce;
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 10da87458e..df321ae102 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -483,8 +483,10 @@ uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
     return error;
 }
 
-static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
-                                          IOMMUAccessFlags flag)
+static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr,
+                                          hwaddr addr,
+                                          IOMMUAccessFlags flag,
+                                          MemTxAttrs attrs)
 {
     S390PCIIOMMU *iommu = container_of(mr, S390PCIIOMMU, iommu_mr);
     S390IOTLBEntry *entry;
diff --git a/hw/sparc/sun4m_iommu.c b/hw/sparc/sun4m_iommu.c
index b677601fc6..f68bcade3c 100644
--- a/hw/sparc/sun4m_iommu.c
+++ b/hw/sparc/sun4m_iommu.c
@@ -282,7 +282,8 @@ static void iommu_bad_addr(IOMMUState *s, hwaddr addr,
 /* Called from RCU critical section */
 static IOMMUTLBEntry sun4m_translate_iommu(IOMMUMemoryRegion *iommu,
                                            hwaddr addr,
-                                           IOMMUAccessFlags flags)
+                                           IOMMUAccessFlags flags,
+                                           MemTxAttrs attrs)
 {
     IOMMUState *is = container_of(iommu, IOMMUState, iommu);
     hwaddr page, pa;
diff --git a/hw/sparc64/sun4u_iommu.c b/hw/sparc64/sun4u_iommu.c
index eb3aaa87e6..7a5a588aed 100644
--- a/hw/sparc64/sun4u_iommu.c
+++ b/hw/sparc64/sun4u_iommu.c
@@ -73,7 +73,8 @@
 /* Called from RCU critical section */
 static IOMMUTLBEntry sun4u_translate_iommu(IOMMUMemoryRegion *iommu,
                                            hwaddr addr,
-                                           IOMMUAccessFlags flag)
+                                           IOMMUAccessFlags flag,
+                                           MemTxAttrs attrs)
 {
     IOMMUState *is = container_of(iommu, IOMMUState, iommu);
     hwaddr baseaddr, offset;
diff --git a/memory.c b/memory.c
index 3212acc7f4..376f72b19c 100644
--- a/memory.c
+++ b/memory.c
@@ -1819,6 +1819,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
     IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
     hwaddr addr, granularity;
     IOMMUTLBEntry iotlb;
+    MemTxAttrs attrs = { 0 };
 
     /* If the IOMMU has its own replay callback, override */
     if (imrc->replay) {
@@ -1829,7 +1830,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
     granularity = memory_region_iommu_get_min_page_size(iommu_mr);
 
     for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
-        iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE);
+        iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, attrs);
         if (iotlb.perm != IOMMU_NONE) {
             n->notify(n, &iotlb);
         }
-- 
2.17.0

[Qemu-devel] [RFC 2/3] memory: add MemTxAttrs to IOMMUTLBEntry

[Peter Xu]

It should never be used for translate() calls since the caller should be
the one who passes in the MemTxAttrs.  However it could be used when we
want to generate an IOMMU translation notification with specific
translation attributes.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6b0ced554d..12865a4890 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -72,6 +72,13 @@ struct IOMMUTLBEntry {
     hwaddr           translated_addr;
     hwaddr           addr_mask;  /* 0xfff = 4k translation */
     IOMMUAccessFlags perm;
+    /*
+     * Attributes that were bound to the DMA translation.  Note that
+     * this field is meaningless when the IOMMUTLBENtry is generated
+     * by a translate() call.  It can be used as a hint when we want
+     * to send IOMMU notifications with specific permission flags.
+     */
+    MemTxAttrs       attrs;
 };
 
 /*
-- 
2.17.0

[Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Xu]

Add two more IOMMU notifier flags to selectively choose whether the
notifier would like to listen to an event that with USER bit set/unset.
Note that all existing notifiers should always been registered with both
of the flags set to make sure they'll receive the notification no matter
whether the USER bit is set or unset in the attributes.  To simplify
this procedure, some new macros are defined.  The old UNMAP-only
notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL
flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old
MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag.

Now if a new notifier would like to register to only UNMAP notifications
with USER bit set, it should register with below flag:

  IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET

Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry
in QEMU), we should do this:

  IOMMUTLBEntry entry;
  ... (set up the fields)
  entry.perm = IOMMU_NONE;
  entry.attrs.user = 1;
  memory_region_notify_iommu(mr, &entry);

Then only the notifiers registered with IOMMU_NOTIFIER_USER_SET will
receive this notification.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h | 48 ++++++++++++++++++++++++++++++++++++++-----
 hw/virtio/vhost.c     |  2 +-
 memory.c              | 10 +++++++++
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 12865a4890..fb9a7059c6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -82,18 +82,56 @@ struct IOMMUTLBEntry {
 };
 
 /*
- * Bitmap for different IOMMUNotifier capabilities. Each notifier can
- * register with one or multiple IOMMU Notifier capability bit(s).
+ * Bitmap for different IOMMUNotifier capabilities.  Please refer to
+ * comments for each notifier capability to know its usage. Note that,
+ * a notifier registered with (UNMAP | USER_SET) does not mean that
+ * it'll notify both MAP notifies and USER_SET notifies, instead it
+ * means it'll only be notified for the events that are UNMAP
+ * meanwhile with USER attribute set.
  */
 typedef enum {
     IOMMU_NOTIFIER_NONE = 0,
-    /* Notify cache invalidations */
+    /*
+     * When set, will notify deleted entries (cache invalidations).
+     * When unset, will not notify deleted entries.
+     */
     IOMMU_NOTIFIER_UNMAP = 0x1,
-    /* Notify entry changes (newly created entries) */
+    /*
+     * When set, will notify newly created entries.  When unset, will
+     * not notify newly created entries.
+     */
     IOMMU_NOTIFIER_MAP = 0x2,
+    /*
+     * When set, will notify when the USER bit is set in
+     * IOMMUTLBEntry.attrs.  When unset, will not notify when the USER
+     * bit is set.
+     */
+    IOMMU_NOTIFIER_USER_SET = 0x4,
+    /*
+     * When set, will notify when the USER bit is cleared in
+     * IOMMUTLBEntry.attrs.  When unset, will not notify when the USER
+     * bit is cleared.
+     */
+    IOMMU_NOTIFIER_USER_UNSET = 0x8,
 } IOMMUNotifierFlag;
 
-#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
+/* Use this when the notifier does not care about USER bit */
+#define IOMMU_NOTIFIER_USER_ALL \
+    (IOMMU_NOTIFIER_USER_SET | IOMMU_NOTIFIER_USER_UNSET)
+
+/* Use this when the notifier does not care about any attribute */
+#define IOMMU_NOTIFIER_ATTRS_ALL \
+    (IOMMU_NOTIFIER_USER_ALL)
+
+/* Use this to notify all UNMAP notifications */
+#define IOMMU_NOTIFIER_UNMAP_ALL \
+    (IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_ATTRS_ALL)
+
+/* Use this to notify all notifications */
+#define IOMMU_NOTIFIER_ALL (                    \
+        IOMMU_NOTIFIER_MAP |                    \
+        IOMMU_NOTIFIER_UNMAP |                  \
+        IOMMU_NOTIFIER_ATTRS_ALL)
 
 struct IOMMUNotifier;
 typedef void (*IOMMUNotify)(struct IOMMUNotifier *notifier,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 96175b214d..da6efeadad 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -672,7 +672,7 @@ static void vhost_iommu_region_add(MemoryListener *listener,
                      section->size);
     end = int128_sub(end, int128_one());
     iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
-                        IOMMU_NOTIFIER_UNMAP,
+                        IOMMU_NOTIFIER_UNMAP_ALL,
                         section->offset_within_region,
                         int128_get64(end));
     iommu->mr = section->mr;
diff --git a/memory.c b/memory.c
index 376f72b19c..9e4617df5a 100644
--- a/memory.c
+++ b/memory.c
@@ -1880,6 +1880,16 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
         return;
     }
 
+    if (!(notifier->notifier_flags & IOMMU_NOTIFIER_USER_SET) &&
+        entry->attrs.user) {
+        return;
+    }
+
+    if (!(notifier->notifier_flags & IOMMU_NOTIFIER_USER_UNSET) &&
+        !entry->attrs.user) {
+        return;
+    }
+
     if (entry->perm & IOMMU_RW) {
         request_flags = IOMMU_NOTIFIER_MAP;
     } else {
-- 
2.17.0

Re: [Qemu-devel] [RFC 1/3] memory: add MemTxAttrs to translate function

[Peter Maydell]

On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote:
> Add a new MemTxAttrs parameter to the IOMMUMemoryRegionClass.translate()
> function, which takes some extra context of the translation request.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/exec/memory.h    | 5 ++++-
>  exec.c                   | 2 +-
>  hw/alpha/typhoon.c       | 3 ++-
>  hw/arm/smmuv3.c          | 2 +-
>  hw/dma/rc4030.c          | 6 ++++--
>  hw/i386/amd_iommu.c      | 2 +-
>  hw/i386/intel_iommu.c    | 6 ++++--
>  hw/ppc/spapr_iommu.c     | 3 ++-
>  hw/s390x/s390-pci-bus.c  | 6 ++++--
>  hw/sparc/sun4m_iommu.c   | 3 ++-
>  hw/sparc64/sun4u_iommu.c | 3 ++-
>  memory.c                 | 3 ++-
>  12 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index eb2ba06519..6b0ced554d 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -234,9 +234,12 @@ typedef struct IOMMUMemoryRegionClass {
>       * @iommu: the IOMMUMemoryRegion
>       * @hwaddr: address to be translated within the memory region
>       * @flag: requested access permissions
> +     * @attrs: MemTxAttrs that was bound to the translation
> +     *         operation. If flag==IOMMU_NONE, this field is
> +     *         meaningless
>       */

This won't work, because the "allow TCG transactions to pass
through an IOMMU" code needs to pass IOMMU_NONE. IOMMU_NONE
means "give me all the permissions info, and don't shortcut it".
For TCG we get the info, and then cache it and use it for
subsequent transactions, both read and write, regardless of
whether the first one was a read or a write.

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/3] memory: add MemTxAttrs to IOMMUTLBEntry

[Peter Maydell]

On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote:
> It should never be used for translate() calls since the caller should be
> the one who passes in the MemTxAttrs.  However it could be used when we
> want to generate an IOMMU translation notification with specific
> translation attributes.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/exec/memory.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6b0ced554d..12865a4890 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -72,6 +72,13 @@ struct IOMMUTLBEntry {
>      hwaddr           translated_addr;
>      hwaddr           addr_mask;  /* 0xfff = 4k translation */
>      IOMMUAccessFlags perm;
> +    /*
> +     * Attributes that were bound to the DMA translation.  Note that
> +     * this field is meaningless when the IOMMUTLBENtry is generated
> +     * by a translate() call.  It can be used as a hint when we want
> +     * to send IOMMU notifications with specific permission flags.
> +     */
> +    MemTxAttrs       attrs;
>  };

How do we say "this applies for more than just this set of
tx attrs" ? eg, if my IOMMU cares only about the secure/nonsecure
attribute, how do I express "this IOMMU TLB entry is valid for
both attrs.user = 1 and attrs.user = 0" ?

thanks
-- PMM

Re: [Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Maydell]

On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote:
> Add two more IOMMU notifier flags to selectively choose whether the
> notifier would like to listen to an event that with USER bit set/unset.
> Note that all existing notifiers should always been registered with both
> of the flags set to make sure they'll receive the notification no matter
> whether the USER bit is set or unset in the attributes.  To simplify
> this procedure, some new macros are defined.  The old UNMAP-only
> notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL
> flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old
> MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag.
>
> Now if a new notifier would like to register to only UNMAP notifications
> with USER bit set, it should register with below flag:
>
>   IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET
>
> Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry
> in QEMU), we should do this:
>
>   IOMMUTLBEntry entry;
>   ... (set up the fields)
>   entry.perm = IOMMU_NONE;
>   entry.attrs.user = 1;
>   memory_region_notify_iommu(mr, &entry);

I don't think this works, because there is not a single unique
MemTxAttrs for a particular invalidation event. Invalidates
will affect classes of transaction attributes. For instance
in the MPC, changing the lookup table can invalidate cached
IOMMU information for any translation that was done either
with attrs.secure = 1, or for translations with attrs.unspecified = 1.

In general, at the point where the IOMMU calls notifiers it
won't have a single MemTxAttrs it can use, so you don't want
to put an attrs into the IOMMUTLBEntry.

thanks
-- PMM

Re: [Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Xu]

On Tue, Jun 05, 2018 at 02:54:18PM +0100, Peter Maydell wrote:
> On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote:
> > Add two more IOMMU notifier flags to selectively choose whether the
> > notifier would like to listen to an event that with USER bit set/unset.
> > Note that all existing notifiers should always been registered with both
> > of the flags set to make sure they'll receive the notification no matter
> > whether the USER bit is set or unset in the attributes.  To simplify
> > this procedure, some new macros are defined.  The old UNMAP-only
> > notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL
> > flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old
> > MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag.
> >
> > Now if a new notifier would like to register to only UNMAP notifications
> > with USER bit set, it should register with below flag:
> >
> >   IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET
> >
> > Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry
> > in QEMU), we should do this:
> >
> >   IOMMUTLBEntry entry;
> >   ... (set up the fields)
> >   entry.perm = IOMMU_NONE;
> >   entry.attrs.user = 1;
> >   memory_region_notify_iommu(mr, &entry);
> 
> I don't think this works, because there is not a single unique
> MemTxAttrs for a particular invalidation event. Invalidates
> will affect classes of transaction attributes. For instance
> in the MPC, changing the lookup table can invalidate cached
> IOMMU information for any translation that was done either
> with attrs.secure = 1, or for translations with attrs.unspecified = 1.
> 
> In general, at the point where the IOMMU calls notifiers it
> won't have a single MemTxAttrs it can use, so you don't want
> to put an attrs into the IOMMUTLBEntry.

So we have a requirement that we want to send an invalidation for
either (1) unspecified, or (2) secure=1.  We can't do that with a
single MemTxAttrs.

Could we just notify twice?  One with unspecified=1 and one with
secure=1?  Is this (reduce the calls to notify functions) the major
reason we introduce this IOMMU index idea into the memory API (besides
"avoid possible programming error" you mentioned in IRC discussion)?

Again, I think the more critical question would be whether we can pass
in MemTxAttrs into translate(), and whether we can avoid introducing
this IOMMU index idea into the memory API layer... For example, would
it add complexity to other architectures without much benefit?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Maydell]

On 5 June 2018 at 15:26, Peter Xu <peterx@redhat.com> wrote:
> So we have a requirement that we want to send an invalidation for
> either (1) unspecified, or (2) secure=1.  We can't do that with a
> single MemTxAttrs.
>
> Could we just notify twice?  One with unspecified=1 and one with
> secure=1?  Is this (reduce the calls to notify functions) the major
> reason we introduce this IOMMU index idea into the memory API (besides
> "avoid possible programming error" you mentioned in IRC discussion)?

How would you handle "this changes mappings for txattrs where
unspecified == 0 && secure == 0" ?

How does the notifier registering API say what it's interested in?
In the exec.c code that deals with sending TCG transactions
through IOMMUs, if I have to register a notifier for every
possible TCG transaction attribute I ever see, that's a lot
of unnecessary notifiers.

> Again, I think the more critical question would be whether we can pass
> in MemTxAttrs into translate(), and whether we can avoid introducing
> this IOMMU index idea into the memory API layer... For example, would
> it add complexity to other architectures without much benefit?

I think the fundamental difference in our points of view here
is that I see the IOMMU index as *reducing* complexity, not
adding it. Yes, it is an extra level of indirection, but I
think it helps us express a useful concept. The patches you've
sent here seem to me to be adding extra complexity to the
notifier API and the core code which isn't required in
the patch set that I sent.

thanks
-- PMM

Re: [Qemu-devel] [RFC 1/3] memory: add MemTxAttrs to translate function

[David Gibson]

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

On Tue, Jun 05, 2018 at 02:38:31PM +0100, Peter Maydell wrote:
> On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote:
> > Add a new MemTxAttrs parameter to the IOMMUMemoryRegionClass.translate()
> > function, which takes some extra context of the translation request.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/exec/memory.h    | 5 ++++-
> >  exec.c                   | 2 +-
> >  hw/alpha/typhoon.c       | 3 ++-
> >  hw/arm/smmuv3.c          | 2 +-
> >  hw/dma/rc4030.c          | 6 ++++--
> >  hw/i386/amd_iommu.c      | 2 +-
> >  hw/i386/intel_iommu.c    | 6 ++++--
> >  hw/ppc/spapr_iommu.c     | 3 ++-
> >  hw/s390x/s390-pci-bus.c  | 6 ++++--
> >  hw/sparc/sun4m_iommu.c   | 3 ++-
> >  hw/sparc64/sun4u_iommu.c | 3 ++-
> >  memory.c                 | 3 ++-
> >  12 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index eb2ba06519..6b0ced554d 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -234,9 +234,12 @@ typedef struct IOMMUMemoryRegionClass {
> >       * @iommu: the IOMMUMemoryRegion
> >       * @hwaddr: address to be translated within the memory region
> >       * @flag: requested access permissions
> > +     * @attrs: MemTxAttrs that was bound to the translation
> > +     *         operation. If flag==IOMMU_NONE, this field is
> > +     *         meaningless
> >       */
> 
> This won't work, because the "allow TCG transactions to pass
> through an IOMMU" code needs to pass IOMMU_NONE. IOMMU_NONE
> means "give me all the permissions info, and don't shortcut it".
> For TCG we get the info, and then cache it and use it for
> subsequent transactions, both read and write, regardless of
> whether the first one was a read or a write.

Likewise the "replay" operations needed for hotplug use IOMMU_NONE.

It also seems like it's kind of abusing the meaning of "AddressSpace",
if other factors are influencing how the translation is done.

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

Re: [Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Xu]

On Tue, Jun 05, 2018 at 03:42:11PM +0100, Peter Maydell wrote:
> On 5 June 2018 at 15:26, Peter Xu <peterx@redhat.com> wrote:
> > So we have a requirement that we want to send an invalidation for
> > either (1) unspecified, or (2) secure=1.  We can't do that with a
> > single MemTxAttrs.
> >
> > Could we just notify twice?  One with unspecified=1 and one with
> > secure=1?  Is this (reduce the calls to notify functions) the major
> > reason we introduce this IOMMU index idea into the memory API (besides
> > "avoid possible programming error" you mentioned in IRC discussion)?
> 
> How would you handle "this changes mappings for txattrs where
> unspecified == 0 && secure == 0" ?

Let's forget this patch.  I was confused about the problem behind when
I wrote this up...  Now I know a bit more.  It's related to how we can
send a notification covers multiple contexts. Let's assume we still
only support MAP and UNMAP in the notifiers. But let's assume
IOMMUTLBEntry has MemTxAttrs field.

If so, for "this changes mappings for txattrs where unspecified == 0
&& secure == 0", we just send one notify with attrs.unspecified=0 and
attrs.secure=0.  Meanwhile in the notifier hook you can parse the
attrs to see whether that's what you need, and skip if not.

Will that work?

> 
> How does the notifier registering API say what it's interested in?
> In the exec.c code that deals with sending TCG transactions
> through IOMMUs, if I have to register a notifier for every
> possible TCG transaction attribute I ever see, that's a lot
> of unnecessary notifiers.

Again, let's assume this patch does not exist.  Then only one notifier
is needed to be registered with UNMAP notify, and in the hook function
it can detect whether it has (attrs.unspecified==0 &&
attrs.unsecure==0).

> 
> > Again, I think the more critical question would be whether we can pass
> > in MemTxAttrs into translate(), and whether we can avoid introducing
> > this IOMMU index idea into the memory API layer... For example, would
> > it add complexity to other architectures without much benefit?
> 
> I think the fundamental difference in our points of view here
> is that I see the IOMMU index as *reducing* complexity, not
> adding it. Yes, it is an extra level of indirection, but I
> think it helps us express a useful concept.

Yes.  And again, I'm also worrying about what we should do when we
want to pass requester ID into translate() too if we have this extra
layer.

> The patches you've
> sent here seem to me to be adding extra complexity to the
> notifier API and the core code which isn't required in
> the patch set that I sent.

Again I didn't really understand the problem behind before.  Now I
don't think it's important on whether we'll need to introduce new
notifier flags, I wanted to know whether we can avoid introducing
IOMMU index into memory API. Let's just ignore this patch.  Sorry for
that confusion.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [RFC 3/3] memory: introduce IOMMU_NOTIFIER_USER_[UN]SET

[Peter Xu]

On Wed, Jun 06, 2018 at 02:56:29PM +0800, Peter Xu wrote:

[...]

> > The patches you've
> > sent here seem to me to be adding extra complexity to the
> > notifier API and the core code which isn't required in
> > the patch set that I sent.
> 
> Again I didn't really understand the problem behind before.  Now I
> don't think it's important on whether we'll need to introduce new
> notifier flags, I wanted to know whether we can avoid introducing
> IOMMU index into memory API. Let's just ignore this patch.  Sorry for
> that confusion.

For this part, I'd say of course this is based on the assumption that
the IOMMU index might not help much with non-tcg use cases.  If
someone could help me understand better on how that could help in
general to memory API then I would be much appreciated, since it
really confused me even until today (and I can't see a good way at
least for VT-d to leverage that idea).  Thanks,

-- 
Peter Xu