[Qemu-devel] [RFC 0/5] Memory transaction attributes API

[Qemu-devel] [RFC 0/5] Memory transaction attributes API

[Peter Maydell]

This is an RFC patchset aimed at getting comment on
some memory API changes to support "transaction attributes",
ie sideband information that goes along with a memory read
or write access to define things like ARM secure/nonsecure,
CPU/transaction master ID, privileged/nonprivileged.

(See also previous discussion:
https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg01026.html )

Patch 1 is the API changes themselves:
1) new read_with_attrs and write_with_attrs fields in the
   MemoryRegionOps struct; the old read/write still exist
   for backwards compatibility, but devices that care about
   attributes can register with these function pointers instead
2) the functions return a success/failure status, so a device
   can actually fail bad transactions rather than merely
   returning bogus data. (This isn't wired up in this patchset
   but I include it to avoid revving the memory API twice.)

Patches 2, 3 and 4 then plumb the memory attribute parameters
through the various functions, working upwards to being able
to put them in the iotlb. Patch 5 implements the target-arm
changes to provide a secure/nonsecure tx attribute based on
the page table walk, as a demonstration.

There are obviously more APIs within QEMU for memory access
functions which need to change to either always take a tx
attribute, or to have extra with-tx-attribute versions of the
functions. For the moment things are stubbed out with passing
in "no attributes specified" values.

I've modelled the transaction attributes as a (typedefed)
uint64_t, whose bits will be defined as we find requirements
for them (the meaning will not be per-architecture). When
we originally discussed this on-list, Edgar suggested making
the attributes be a (pointer to a) struct; however I found
the ownership/copying semantics on this too awkward, because
the access path needs to take attributes set up in the TLB
and then modify them according to details of the access
actually being made before passing them to the device, so
took the simpler implementation route.

I intend to continue working on this (filling in the gaps,
etc), but wanted to send this series out early for comment
on the memory API changes in particular.

thanks
-- PMM

Peter Maydell (5):
  memory: Define API for MemoryRegionOps to take attrs and return status
  memory: Add MemTxAttrs argument to io_mem_read and io_mem_write
  Make CPU iotlb a structure rather than a plain hwaddr
  Add MemTxAttrs to the IOTLB
  target-arm: Honour NS bits in page tables

 cputlb.c                 |  22 +++++++---
 exec.c                   |  29 +++++++-------
 hw/s390x/s390-pci-inst.c |   7 ++--
 hw/vfio/pci.c            |   4 +-
 include/exec/cpu-defs.h  |  15 ++++++-
 include/exec/exec-all.h  |   8 +++-
 include/exec/memattrs.h  |  37 +++++++++++++++++
 include/exec/memory.h    |  22 ++++++++++
 memory.c                 | 102 +++++++++++++++++++++++++++++++++++++----------
 softmmu_template.h       |  36 +++++++++--------
 target-arm/helper.c      |  83 ++++++++++++++++++++++++++++++++------
 11 files changed, 288 insertions(+), 77 deletions(-)
 create mode 100644 include/exec/memattrs.h

-- 
1.9.1

[Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Peter Maydell]

Define an API so that devices can register MemoryRegionOps whose read
and write callback functions are passed an arbitrary pointer to some
transaction attributes and can return a success-or-failure status code.
This will allow us to model devices which:
 * behave differently for ARM Secure/NonSecure memory accesses
 * behave differently for privileged/unprivileged accesses
 * may return a transaction failure (causing a guest exception)
   for erroneous accesses

This patch defines the new API and plumbs the attributes parameter through
to the memory.c public level functions io_mem_read() and io_mem_write(),
where it is currently dummied out.

The success/failure response indication is currently ignored; it is
provided so we can implement it later without having to change the
callback function API yet again in future.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memattrs.h | 34 +++++++++++++++++
 include/exec/memory.h   | 22 +++++++++++
 memory.c                | 99 ++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 137 insertions(+), 18 deletions(-)
 create mode 100644 include/exec/memattrs.h

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
new file mode 100644
index 0000000..b8d7808
--- /dev/null
+++ b/include/exec/memattrs.h
@@ -0,0 +1,34 @@
+/*
+ * Memory transaction attributes
+ *
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Authors:
+ *  Peter Maydell <peter.maydell@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef MEMATTRS_H
+#define MEMATTRS_H
+
+/* Every memory transaction has associated with it a set of
+ * attributes. Some of these are generic (such as the ID of
+ * the bus master); some are specific to a particular kind of
+ * bus (such as the ARM Secure/NonSecure bit). We define them
+ * all as non-overlapping bits in a single integer to avoid
+ * confusion if different parts of QEMU used the same bit for
+ * different semantics.
+ */
+typedef uint64_t MemTxAttrs;
+
+/* Bus masters which don't specify any attributes will get this,
+ * which has all attribute bits clear except the topmost one
+ * (so that we can distinguish "all attributes deliberately clear"
+ * from "didn't specify" if necessary).
+ */
+#define MEMTXATTRS_UNSPECIFIED (1ULL << 63)
+
+#endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 06ffa1d..642e59f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -28,6 +28,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "exec/hwaddr.h"
 #endif
+#include "exec/memattrs.h"
 #include "qemu/queue.h"
 #include "qemu/int128.h"
 #include "qemu/notify.h"
@@ -68,6 +69,16 @@ struct IOMMUTLBEntry {
     IOMMUAccessFlags perm;
 };
 
+/* New-style MMIO accessors can indicate that the transaction failed.
+ * This is currently ignored, but provided in the API to allow us to add
+ * support later without changing the MemoryRegionOps functions yet again.
+ */
+typedef enum {
+    MemTx_OK = 0,
+    MemTx_DecodeError = 1, /* "nothing at that address" */
+    MemTx_SlaveError = 2,  /* "device unhappy with request" (eg misalignment) */
+} MemTxResult;
+
 /*
  * Memory region callbacks
  */
@@ -84,6 +95,17 @@ struct MemoryRegionOps {
                   uint64_t data,
                   unsigned size);
 
+    MemTxResult (*read_with_attrs)(void *opaque,
+                                   hwaddr addr,
+                                   uint64_t *data,
+                                   unsigned size,
+                                   MemTxAttrs attrs);
+    MemTxResult (*write_with_attrs)(void *opaque,
+                                    hwaddr addr,
+                                    uint64_t data,
+                                    unsigned size,
+                                    MemTxAttrs attrs);
+
     enum device_endian endianness;
     /* Guest-visible constraints: */
     struct {
diff --git a/memory.c b/memory.c
index 20f6d9e..76f95b7 100644
--- a/memory.c
+++ b/memory.c
@@ -373,7 +373,8 @@ static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
                                                 uint64_t *value,
                                                 unsigned size,
                                                 unsigned shift,
-                                                uint64_t mask)
+                                                uint64_t mask,
+                                                MemTxAttrs attrs)
 {
     uint64_t tmp;
 
@@ -387,7 +388,8 @@ static void memory_region_read_accessor(MemoryRegion *mr,
                                         uint64_t *value,
                                         unsigned size,
                                         unsigned shift,
-                                        uint64_t mask)
+                                        uint64_t mask,
+                                        MemTxAttrs attrs)
 {
     uint64_t tmp;
 
@@ -399,12 +401,31 @@ static void memory_region_read_accessor(MemoryRegion *mr,
     *value |= (tmp & mask) << shift;
 }
 
+static void memory_region_read_with_attrs_accessor(MemoryRegion *mr,
+                                                   hwaddr addr,
+                                                   uint64_t *value,
+                                                   unsigned size,
+                                                   unsigned shift,
+                                                   uint64_t mask,
+                                                   MemTxAttrs attrs)
+{
+    uint64_t tmp = 0;
+
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
+    mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
+    trace_memory_region_ops_read(mr, addr, tmp, size);
+    *value |= (tmp & mask) << shift;
+}
+
 static void memory_region_oldmmio_write_accessor(MemoryRegion *mr,
                                                  hwaddr addr,
                                                  uint64_t *value,
                                                  unsigned size,
                                                  unsigned shift,
-                                                 uint64_t mask)
+                                                 uint64_t mask,
+                                                 MemTxAttrs attrs)
 {
     uint64_t tmp;
 
@@ -418,7 +439,8 @@ static void memory_region_write_accessor(MemoryRegion *mr,
                                          uint64_t *value,
                                          unsigned size,
                                          unsigned shift,
-                                         uint64_t mask)
+                                         uint64_t mask,
+                                         MemTxAttrs attrs)
 {
     uint64_t tmp;
 
@@ -430,6 +452,24 @@ static void memory_region_write_accessor(MemoryRegion *mr,
     mr->ops->write(mr->opaque, addr, tmp, size);
 }
 
+static void memory_region_write_with_attrs_accessor(MemoryRegion *mr,
+                                                    hwaddr addr,
+                                                    uint64_t *value,
+                                                    unsigned size,
+                                                    unsigned shift,
+                                                    uint64_t mask,
+                                                    MemTxAttrs attrs)
+{
+    uint64_t tmp;
+
+    if (mr->flush_coalesced_mmio) {
+        qemu_flush_coalesced_mmio_buffer();
+    }
+    tmp = (*value >> shift) & mask;
+    trace_memory_region_ops_write(mr, addr, tmp, size);
+    mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
+}
+
 static void access_with_adjusted_size(hwaddr addr,
                                       uint64_t *value,
                                       unsigned size,
@@ -440,8 +480,10 @@ static void access_with_adjusted_size(hwaddr addr,
                                                      uint64_t *value,
                                                      unsigned size,
                                                      unsigned shift,
-                                                     uint64_t mask),
-                                      MemoryRegion *mr)
+                                                     uint64_t mask,
+                                                     MemTxAttrs attrs),
+                                      MemoryRegion *mr,
+                                      MemTxAttrs attrs)
 {
     uint64_t access_mask;
     unsigned access_size;
@@ -460,11 +502,11 @@ static void access_with_adjusted_size(hwaddr addr,
     if (memory_region_big_endian(mr)) {
         for (i = 0; i < size; i += access_size) {
             access(mr, addr + i, value, access_size,
-                   (size - access_size - i) * 8, access_mask);
+                   (size - access_size - i) * 8, access_mask, attrs);
         }
     } else {
         for (i = 0; i < size; i += access_size) {
-            access(mr, addr + i, value, access_size, i * 8, access_mask);
+            access(mr, addr + i, value, access_size, i * 8, access_mask, attrs);
         }
     }
 }
@@ -1055,7 +1097,8 @@ bool memory_region_access_valid(MemoryRegion *mr,
 
 static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
                                              hwaddr addr,
-                                             unsigned size)
+                                             unsigned size,
+                                             MemTxAttrs attrs)
 {
     uint64_t data = 0;
 
@@ -1063,10 +1106,18 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
         access_with_adjusted_size(addr, &data, size,
                                   mr->ops->impl.min_access_size,
                                   mr->ops->impl.max_access_size,
-                                  memory_region_read_accessor, mr);
+                                  memory_region_read_accessor, mr,
+                                  attrs);
+    } else if (mr->ops->read_with_attrs) {
+        access_with_adjusted_size(addr, &data, size,
+                                  mr->ops->impl.min_access_size,
+                                  mr->ops->impl.max_access_size,
+                                  memory_region_read_with_attrs_accessor, mr,
+                                  attrs);
     } else {
         access_with_adjusted_size(addr, &data, size, 1, 4,
-                                  memory_region_oldmmio_read_accessor, mr);
+                                  memory_region_oldmmio_read_accessor, mr,
+                                  attrs);
     }
 
     return data;
@@ -1075,14 +1126,15 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
 static bool memory_region_dispatch_read(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t *pval,
-                                        unsigned size)
+                                        unsigned size,
+                                        MemTxAttrs attrs)
 {
     if (!memory_region_access_valid(mr, addr, size, false)) {
         *pval = unassigned_mem_read(mr, addr, size);
         return true;
     }
 
-    *pval = memory_region_dispatch_read1(mr, addr, size);
+    *pval = memory_region_dispatch_read1(mr, addr, size, attrs);
     adjust_endianness(mr, pval, size);
     return false;
 }
@@ -1090,7 +1142,8 @@ static bool memory_region_dispatch_read(MemoryRegion *mr,
 static bool memory_region_dispatch_write(MemoryRegion *mr,
                                          hwaddr addr,
                                          uint64_t data,
-                                         unsigned size)
+                                         unsigned size,
+                                         MemTxAttrs attrs)
 {
     if (!memory_region_access_valid(mr, addr, size, true)) {
         unassigned_mem_write(mr, addr, data, size);
@@ -1103,10 +1156,18 @@ static bool memory_region_dispatch_write(MemoryRegion *mr,
         access_with_adjusted_size(addr, &data, size,
                                   mr->ops->impl.min_access_size,
                                   mr->ops->impl.max_access_size,
-                                  memory_region_write_accessor, mr);
+                                  memory_region_write_accessor, mr,
+                                  attrs);
+    } else if (mr->ops->write_with_attrs) {
+        access_with_adjusted_size(addr, &data, size,
+                                  mr->ops->impl.min_access_size,
+                                  mr->ops->impl.max_access_size,
+                                  memory_region_write_with_attrs_accessor, mr,
+                                  attrs);
     } else {
         access_with_adjusted_size(addr, &data, size, 1, 4,
-                                  memory_region_oldmmio_write_accessor, mr);
+                                  memory_region_oldmmio_write_accessor, mr,
+                                  attrs);
     }
     return false;
 }
@@ -1994,13 +2055,15 @@ void address_space_destroy(AddressSpace *as)
 
 bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned size)
 {
-    return memory_region_dispatch_read(mr, addr, pval, size);
+    return memory_region_dispatch_read(mr, addr, pval, size,
+                                       MEMTXATTRS_UNSPECIFIED);
 }
 
 bool io_mem_write(MemoryRegion *mr, hwaddr addr,
                   uint64_t val, unsigned size)
 {
-    return memory_region_dispatch_write(mr, addr, val, size);
+    return memory_region_dispatch_write(mr, addr, val, size,
+                                        MEMTXATTRS_UNSPECIFIED);
 }
 
 typedef struct MemoryRegionList MemoryRegionList;
-- 
1.9.1

[Qemu-devel] [RFC 2/5] memory: Add MemTxAttrs argument to io_mem_read and io_mem_write

[Peter Maydell]

Add a MemTxAttrs argument to the io_mem_read() and io_mem_write()
functions. In this commit this is simply pushing the ability to
specify attributes out one level of the callstack, and all the
callers currently pass MEMTXATTRS_UNSPECIFIED.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c                   | 29 +++++++++++++++--------------
 hw/s390x/s390-pci-inst.c |  7 ++++---
 hw/vfio/pci.c            |  4 ++--
 include/exec/exec-all.h  |  5 +++--
 memory.c                 | 11 +++++------
 softmmu_template.h       |  4 ++--
 6 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index e97071a..4bd1f97 100644
--- a/exec.c
+++ b/exec.c
@@ -2314,6 +2314,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
     hwaddr addr1;
     MemoryRegion *mr;
     bool error = false;
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
 
     while (len > 0) {
         l = len;
@@ -2328,22 +2329,22 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 case 8:
                     /* 64 bit write access */
                     val = ldq_p(buf);
-                    error |= io_mem_write(mr, addr1, val, 8);
+                    error |= io_mem_write(mr, addr1, val, 8, attrs);
                     break;
                 case 4:
                     /* 32 bit write access */
                     val = ldl_p(buf);
-                    error |= io_mem_write(mr, addr1, val, 4);
+                    error |= io_mem_write(mr, addr1, val, 4, attrs);
                     break;
                 case 2:
                     /* 16 bit write access */
                     val = lduw_p(buf);
-                    error |= io_mem_write(mr, addr1, val, 2);
+                    error |= io_mem_write(mr, addr1, val, 2, attrs);
                     break;
                 case 1:
                     /* 8 bit write access */
                     val = ldub_p(buf);
-                    error |= io_mem_write(mr, addr1, val, 1);
+                    error |= io_mem_write(mr, addr1, val, 1, attrs);
                     break;
                 default:
                     abort();
@@ -2362,22 +2363,22 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 switch (l) {
                 case 8:
                     /* 64 bit read access */
-                    error |= io_mem_read(mr, addr1, &val, 8);
+                    error |= io_mem_read(mr, addr1, &val, 8, attrs);
                     stq_p(buf, val);
                     break;
                 case 4:
                     /* 32 bit read access */
-                    error |= io_mem_read(mr, addr1, &val, 4);
+                    error |= io_mem_read(mr, addr1, &val, 4, attrs);
                     stl_p(buf, val);
                     break;
                 case 2:
                     /* 16 bit read access */
-                    error |= io_mem_read(mr, addr1, &val, 2);
+                    error |= io_mem_read(mr, addr1, &val, 2, attrs);
                     stw_p(buf, val);
                     break;
                 case 1:
                     /* 8 bit read access */
-                    error |= io_mem_read(mr, addr1, &val, 1);
+                    error |= io_mem_read(mr, addr1, &val, 1, attrs);
                     stb_p(buf, val);
                     break;
                 default:
@@ -2670,7 +2671,7 @@ static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
     mr = address_space_translate(as, addr, &addr1, &l, false);
     if (l < 4 || !memory_access_is_direct(mr, false)) {
         /* I/O case */
-        io_mem_read(mr, addr1, &val, 4);
+        io_mem_read(mr, addr1, &val, 4, MEMTXATTRS_UNSPECIFIED);
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
             val = bswap32(val);
@@ -2729,7 +2730,7 @@ static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
                                  false);
     if (l < 8 || !memory_access_is_direct(mr, false)) {
         /* I/O case */
-        io_mem_read(mr, addr1, &val, 8);
+        io_mem_read(mr, addr1, &val, 8, MEMTXATTRS_UNSPECIFIED);
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
             val = bswap64(val);
@@ -2796,7 +2797,7 @@ static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
                                  false);
     if (l < 2 || !memory_access_is_direct(mr, false)) {
         /* I/O case */
-        io_mem_read(mr, addr1, &val, 2);
+        io_mem_read(mr, addr1, &val, 2, MEMTXATTRS_UNSPECIFIED);
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
             val = bswap16(val);
@@ -2854,7 +2855,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
     mr = address_space_translate(as, addr, &addr1, &l,
                                  true);
     if (l < 4 || !memory_access_is_direct(mr, true)) {
-        io_mem_write(mr, addr1, val, 4);
+        io_mem_write(mr, addr1, val, 4, MEMTXATTRS_UNSPECIFIED);
     } else {
         addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
         ptr = qemu_get_ram_ptr(addr1);
@@ -2893,7 +2894,7 @@ static inline void stl_phys_internal(AddressSpace *as,
             val = bswap32(val);
         }
 #endif
-        io_mem_write(mr, addr1, val, 4);
+        io_mem_write(mr, addr1, val, 4, MEMTXATTRS_UNSPECIFIED);
     } else {
         /* RAM case */
         addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
@@ -2956,7 +2957,7 @@ static inline void stw_phys_internal(AddressSpace *as,
             val = bswap16(val);
         }
 #endif
-        io_mem_write(mr, addr1, val, 2);
+        io_mem_write(mr, addr1, val, 2, MEMTXATTRS_UNSPECIFIED);
     } else {
         /* RAM case */
         addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 08d8aa6..78ff9c0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -331,7 +331,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             return 0;
         }
         MemoryRegion *mr = pbdev->pdev->io_regions[pcias].memory;
-        io_mem_read(mr, offset, &data, len);
+        io_mem_read(mr, offset, &data, len, MEMTXATTRS_UNSPECIFIED);
     } else if (pcias == 15) {
         if ((4 - (offset & 0x3)) < len) {
             program_interrupt(env, PGM_OPERAND, 4);
@@ -456,7 +456,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             mr = pbdev->pdev->io_regions[pcias].memory;
         }
 
-        io_mem_write(mr, offset, data, len);
+        io_mem_write(mr, offset, data, len, MEMTXATTRS_UNSPECIFIED);
     } else if (pcias == 15) {
         if ((4 - (offset & 0x3)) < len) {
             program_interrupt(env, PGM_OPERAND, 4);
@@ -606,7 +606,8 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr)
     }
 
     for (i = 0; i < len / 8; i++) {
-        io_mem_write(mr, env->regs[r3] + i * 8, ldq_p(buffer + i * 8), 8);
+        io_mem_write(mr, env->regs[r3] + i * 8, ldq_p(buffer + i * 8), 8,
+                     MEMTXATTRS_UNSPECIFIED);
     }
 
     setcc(cpu, ZPCI_PCI_LS_OK);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 6b80539..3e1df0c 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1533,7 +1533,7 @@ static uint64_t vfio_rtl8168_window_quirk_read(void *opaque,
 
             io_mem_read(&vdev->pdev.msix_table_mmio,
                         (hwaddr)(quirk->data.address_match & 0xfff),
-                        &val, size);
+                        &val, size, MEMTXATTRS_UNSPECIFIED);
             return val;
         }
     }
@@ -1563,7 +1563,7 @@ static void vfio_rtl8168_window_quirk_write(void *opaque, hwaddr addr,
 
                 io_mem_write(&vdev->pdev.msix_table_mmio,
                              (hwaddr)(quirk->data.address_match & 0xfff),
-                             data, size);
+                             data, size, MEMTXATTRS_UNSPECIFIED);
             }
 
             quirk->data.flags = 1;
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 8eb0db3..a6271ea 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -21,6 +21,7 @@
 #define _EXEC_ALL_H_
 
 #include "qemu-common.h"
+#include "exec/memattrs.h"
 
 /* allow to see translation results - the slowdown should be negligible, so we leave it */
 #define DEBUG_DISAS
@@ -342,9 +343,9 @@ void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
 struct MemoryRegion *iotlb_to_region(CPUState *cpu,
                                      hwaddr index);
 bool io_mem_read(struct MemoryRegion *mr, hwaddr addr,
-                 uint64_t *pvalue, unsigned size);
+                 uint64_t *pvalue, unsigned size, MemTxAttrs attrs);
 bool io_mem_write(struct MemoryRegion *mr, hwaddr addr,
-                  uint64_t value, unsigned size);
+                  uint64_t value, unsigned size, MemTxAttrs attrs);
 
 void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
               uintptr_t retaddr);
diff --git a/memory.c b/memory.c
index 76f95b7..5b6324c 100644
--- a/memory.c
+++ b/memory.c
@@ -2053,17 +2053,16 @@ void address_space_destroy(AddressSpace *as)
     call_rcu(as, do_address_space_destroy, rcu);
 }
 
-bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned size)
+bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned size,
+                 MemTxAttrs attrs)
 {
-    return memory_region_dispatch_read(mr, addr, pval, size,
-                                       MEMTXATTRS_UNSPECIFIED);
+    return memory_region_dispatch_read(mr, addr, pval, size, attrs);
 }
 
 bool io_mem_write(MemoryRegion *mr, hwaddr addr,
-                  uint64_t val, unsigned size)
+                  uint64_t val, unsigned size, MemTxAttrs attrs)
 {
-    return memory_region_dispatch_write(mr, addr, val, size,
-                                        MEMTXATTRS_UNSPECIFIED);
+    return memory_region_dispatch_write(mr, addr, val, size, attrs);
 }
 
 typedef struct MemoryRegionList MemoryRegionList;
diff --git a/softmmu_template.h b/softmmu_template.h
index 0e3dd35..4b9bae7 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -158,7 +158,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
     }
 
     cpu->mem_io_vaddr = addr;
-    io_mem_read(mr, physaddr, &val, 1 << SHIFT);
+    io_mem_read(mr, physaddr, &val, 1 << SHIFT, MEMTXATTRS_UNSPECIFIED);
     return val;
 }
 #endif
@@ -378,7 +378,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
 
     cpu->mem_io_vaddr = addr;
     cpu->mem_io_pc = retaddr;
-    io_mem_write(mr, physaddr, val, 1 << SHIFT);
+    io_mem_write(mr, physaddr, val, 1 << SHIFT, MEMTXATTRS_UNSPECIFIED);
 }
 
 void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
-- 
1.9.1

[Qemu-devel] [RFC 3/5] Make CPU iotlb a structure rather than a plain hwaddr

[Peter Maydell]

Make the CPU iotlb a structure rather than a plain hwaddr;
this will allow us to add transaction attributes to it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cputlb.c                |  4 ++--
 include/exec/cpu-defs.h | 13 +++++++++++--
 softmmu_template.h      | 32 +++++++++++++++++---------------
 3 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 38f2151..5e1cb8f 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -301,7 +301,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
     env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index];
 
     /* refill the tlb */
-    env->iotlb[mmu_idx][index] = iotlb - vaddr;
+    env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
     te->addend = addend - vaddr;
     if (prot & PAGE_READ) {
         te->addr_read = address;
@@ -349,7 +349,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
                  (addr & TARGET_PAGE_MASK))) {
         cpu_ldub_code(env1, addr);
     }
-    pd = env1->iotlb[mmu_idx][page_index] & ~TARGET_PAGE_MASK;
+    pd = env1->iotlb[mmu_idx][page_index].addr & ~TARGET_PAGE_MASK;
     mr = iotlb_to_region(cpu, pd);
     if (memory_region_is_unassigned(mr)) {
         CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 0ca6f0b..7f88185 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -102,12 +102,21 @@ typedef struct CPUTLBEntry {
 
 QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
 
+/* The IOTLB is not accessed directly inline by generated TCG code,
+ * so the CPUIOTLBEntry layout is not as critical as that of the
+ * CPUTLBEntry. (This is also why we don't want to combine the two
+ * structs into one.)
+ */
+typedef struct CPUIOTLBEntry {
+    hwaddr addr;
+} CPUIOTLBEntry;
+
 #define CPU_COMMON_TLB \
     /* The meaning of the MMU modes is defined in the target code. */   \
     CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE];                  \
     CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE];               \
-    hwaddr iotlb[NB_MMU_MODES][CPU_TLB_SIZE];                           \
-    hwaddr iotlb_v[NB_MMU_MODES][CPU_VTLB_SIZE];                        \
+    CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];                    \
+    CPUIOTLBEntry iotlb_v[NB_MMU_MODES][CPU_VTLB_SIZE];                 \
     target_ulong tlb_flush_addr;                                        \
     target_ulong tlb_flush_mask;                                        \
     target_ulong vtlb_index;                                            \
diff --git a/softmmu_template.h b/softmmu_template.h
index 4b9bae7..7a36550 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -123,7 +123,7 @@
      * victim tlb. try to refill from the victim tlb before walking the       \
      * page table. */                                                         \
     int vidx;                                                                 \
-    hwaddr tmpiotlb;                                                          \
+    CPUIOTLBEntry tmpiotlb;                                                   \
     CPUTLBEntry tmptlb;                                                       \
     for (vidx = CPU_VTLB_SIZE-1; vidx >= 0; --vidx) {                         \
         if (env->tlb_v_table[mmu_idx][vidx].ty == (addr & TARGET_PAGE_MASK)) {\
@@ -143,12 +143,13 @@
 
 #ifndef SOFTMMU_CODE_ACCESS
 static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
-                                              hwaddr physaddr,
+                                              CPUIOTLBEntry *iotlbentry,
                                               target_ulong addr,
                                               uintptr_t retaddr)
 {
     uint64_t val;
     CPUState *cpu = ENV_GET_CPU(env);
+    hwaddr physaddr = iotlbentry->addr;
     MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
@@ -195,15 +196,15 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 
     /* Handle an IO access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
-        hwaddr ioaddr;
+        CPUIOTLBEntry *iotlbentry;
         if ((addr & (DATA_SIZE - 1)) != 0) {
             goto do_unaligned_access;
         }
-        ioaddr = env->iotlb[mmu_idx][index];
+        iotlbentry = &env->iotlb[mmu_idx][index];
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
-        res = glue(io_read, SUFFIX)(env, ioaddr, addr, retaddr);
+        res = glue(io_read, SUFFIX)(env, iotlbentry, addr, retaddr);
         res = TGT_LE(res);
         return res;
     }
@@ -283,15 +284,15 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 
     /* Handle an IO access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
-        hwaddr ioaddr;
+        CPUIOTLBEntry *iotlbentry;
         if ((addr & (DATA_SIZE - 1)) != 0) {
             goto do_unaligned_access;
         }
-        ioaddr = env->iotlb[mmu_idx][index];
+        iotlbentry = &env->iotlb[mmu_idx][index];
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
-        res = glue(io_read, SUFFIX)(env, ioaddr, addr, retaddr);
+        res = glue(io_read, SUFFIX)(env, iotlbentry, addr, retaddr);
         res = TGT_BE(res);
         return res;
     }
@@ -363,12 +364,13 @@ WORD_TYPE helper_be_lds_name(CPUArchState *env, target_ulong addr,
 #endif
 
 static inline void glue(io_write, SUFFIX)(CPUArchState *env,
-                                          hwaddr physaddr,
+                                          CPUIOTLBEntry *iotlbentry,
                                           DATA_TYPE val,
                                           target_ulong addr,
                                           uintptr_t retaddr)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+    hwaddr physaddr = iotlbentry->addr;
     MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
@@ -408,16 +410,16 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 
     /* Handle an IO access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
-        hwaddr ioaddr;
+        CPUIOTLBEntry *iotlbentry;
         if ((addr & (DATA_SIZE - 1)) != 0) {
             goto do_unaligned_access;
         }
-        ioaddr = env->iotlb[mmu_idx][index];
+        iotlbentry = &env->iotlb[mmu_idx][index];
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
         val = TGT_LE(val);
-        glue(io_write, SUFFIX)(env, ioaddr, val, addr, retaddr);
+        glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
         return;
     }
 
@@ -489,16 +491,16 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 
     /* Handle an IO access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
-        hwaddr ioaddr;
+        CPUIOTLBEntry *iotlbentry;
         if ((addr & (DATA_SIZE - 1)) != 0) {
             goto do_unaligned_access;
         }
-        ioaddr = env->iotlb[mmu_idx][index];
+        iotlbentry = &env->iotlb[mmu_idx][index];
 
         /* ??? Note that the io helpers always read data in the target
            byte ordering.  We should push the LE/BE request down into io.  */
         val = TGT_BE(val);
-        glue(io_write, SUFFIX)(env, ioaddr, val, addr, retaddr);
+        glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
         return;
     }
 
-- 
1.9.1

[Qemu-devel] [RFC 4/5] Add MemTxAttrs to the IOTLB

[Peter Maydell]

Add a MemTxAttrs field to the IOTLB, and allow target-specific
code to set it via a new tlb_set_page_with_attrs() function;
pass the attributes through to the device when making IO accesses.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cputlb.c                | 18 +++++++++++++++---
 include/exec/cpu-defs.h |  2 ++
 include/exec/exec-all.h |  3 +++
 softmmu_template.h      |  4 ++--
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 5e1cb8f..7606548 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -249,9 +249,9 @@ static void tlb_add_large_page(CPUArchState *env, target_ulong vaddr,
  * Called from TCG-generated code, which is under an RCU read-side
  * critical section.
  */
-void tlb_set_page(CPUState *cpu, target_ulong vaddr,
-                  hwaddr paddr, int prot,
-                  int mmu_idx, target_ulong size)
+void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
+                             hwaddr paddr, MemTxAttrs attrs, int prot,
+                             int mmu_idx, target_ulong size)
 {
     CPUArchState *env = cpu->env_ptr;
     MemoryRegionSection *section;
@@ -302,6 +302,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
 
     /* refill the tlb */
     env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
+    env->iotlb[mmu_idx][index].attrs = attrs;
     te->addend = addend - vaddr;
     if (prot & PAGE_READ) {
         te->addr_read = address;
@@ -331,6 +332,17 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
     }
 }
 
+/* Add a new TLB entry, but without specifying the memory
+ * transaction attributes to be used.
+ */
+void tlb_set_page(CPUState *cpu, target_ulong vaddr,
+                  hwaddr paddr, int prot,
+                  int mmu_idx, target_ulong size)
+{
+    tlb_set_page_with_attrs(cpu, vaddr, paddr, MEMTXATTRS_UNSPECIFIED,
+                            prot, mmu_idx, size);
+}
+
 /* NOTE: this function can trigger an exception */
 /* NOTE2: the returned address is not exactly the physical address: it
  * is actually a ram_addr_t (in system mode; the user mode emulation
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 7f88185..3f56546 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -30,6 +30,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "exec/hwaddr.h"
 #endif
+#include "exec/memattrs.h"
 
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
@@ -109,6 +110,7 @@ QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
  */
 typedef struct CPUIOTLBEntry {
     hwaddr addr;
+    MemTxAttrs attrs;
 } CPUIOTLBEntry;
 
 #define CPU_COMMON_TLB \
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index a6271ea..34400f7 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -106,6 +106,9 @@ void tlb_flush(CPUState *cpu, int flush_global);
 void tlb_set_page(CPUState *cpu, target_ulong vaddr,
                   hwaddr paddr, int prot,
                   int mmu_idx, target_ulong size);
+void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
+                             hwaddr paddr, MemTxAttrs attrs,
+                             int prot, int mmu_idx, target_ulong size);
 void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr);
 #else
 static inline void tlb_flush_page(CPUState *cpu, target_ulong addr)
diff --git a/softmmu_template.h b/softmmu_template.h
index 7a36550..7310a93 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -159,7 +159,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
     }
 
     cpu->mem_io_vaddr = addr;
-    io_mem_read(mr, physaddr, &val, 1 << SHIFT, MEMTXATTRS_UNSPECIFIED);
+    io_mem_read(mr, physaddr, &val, 1 << SHIFT, iotlbentry->attrs);
     return val;
 }
 #endif
@@ -380,7 +380,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
 
     cpu->mem_io_vaddr = addr;
     cpu->mem_io_pc = retaddr;
-    io_mem_write(mr, physaddr, val, 1 << SHIFT, MEMTXATTRS_UNSPECIFIED);
+    io_mem_write(mr, physaddr, val, 1 << SHIFT, iotlbentry->attrs);
 }
 
 void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
-- 
1.9.1

[Qemu-devel] [RFC 5/5] target-arm: Honour NS bits in page tables

[Peter Maydell]

Honour the NS bit in ARM page tables:
 * when adding entries to the TLB, include the Secure/NonSecure
   transaction attribute
 * set the NS bit in the PAR when doing ATS operations

Note that we don't yet correctly use the NSTable bit to
cause the page table walk itself to use the right attributes.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memattrs.h |  3 ++
 target-arm/helper.c     | 83 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 74 insertions(+), 12 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index b8d7808..5df180e 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -24,6 +24,9 @@
  */
 typedef uint64_t MemTxAttrs;
 
+/* ARM/AMBA TrustZone Secure access */
+#define MEMTXATTRS_SECURE (1ULL << 0)
+
 /* Bus masters which don't specify any attributes will get this,
  * which has all attribute bits clear except the topmost one
  * (so that we can distinguish "all attributes deliberately clear"
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 10886c5..c3be9d9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -14,7 +14,7 @@
 #ifndef CONFIG_USER_ONLY
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
                                 int access_type, ARMMMUIdx mmu_idx,
-                                hwaddr *phys_ptr, int *prot,
+                                hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
                                 target_ulong *page_size);
 
 /* Definitions for the PMCCNTR and PMCR registers */
@@ -1466,9 +1466,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     int prot;
     int ret;
     uint64_t par64;
+    MemTxAttrs attrs;
 
     ret = get_phys_addr(env, value, access_type, mmu_idx,
-                        &phys_addr, &prot, &page_size);
+                        &phys_addr, &attrs, &prot, &page_size);
     if (extended_addresses_enabled(env)) {
         /* ret is a DFSR/IFSR value for the long descriptor
          * translation table format, but with WnR always clear.
@@ -1477,6 +1478,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
         par64 = (1 << 11); /* LPAE bit always set */
         if (ret == 0) {
             par64 |= phys_addr & ~0xfffULL;
+            if (!(attrs & MEMTXATTRS_SECURE)) {
+                par64 |= (1 << 9); /* NS */
+            }
             /* We don't set the ATTR or SH fields in the PAR. */
         } else {
             par64 |= 1; /* F */
@@ -1499,6 +1503,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
             } else {
                 par64 = phys_addr & 0xfffff000;
             }
+            if (!(attrs & MEMTXATTRS_SECURE)) {
+                par64 |= (1 << 9); /* NS */
+            }
         } else {
             par64 = ((ret & (1 << 10)) >> 5) | ((ret & (1 << 12)) >> 6) |
                     ((ret & 0xf) << 1) | 1;
@@ -4858,6 +4865,26 @@ static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
     }
 }
 
+/* Return true if this address translation regime is secure */
+static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_S1NSE1:
+    case ARMMMUIdx_S1E2:
+    case ARMMMUIdx_S2NS:
+        return false;
+    case ARMMMUIdx_S1E3:
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1SE1:
+        return true;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /* Return the SCTLR value which controls this address translation regime */
 static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
 {
@@ -5210,6 +5237,7 @@ do_fault:
 
 static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
                             ARMMMUIdx mmu_idx, hwaddr *phys_ptr,
+                            MemTxAttrs *attrs,
                             int *prot, target_ulong *page_size)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
@@ -5224,6 +5252,7 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
     int domain_prot;
     hwaddr phys_addr;
     uint32_t dacr;
+    bool ns;
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
@@ -5273,10 +5302,12 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
         xn = desc & (1 << 4);
         pxn = desc & 1;
         code = 13;
+        ns = extract32(desc, 19, 1);
     } else {
         if (arm_feature(env, ARM_FEATURE_PXN)) {
             pxn = (desc >> 2) & 1;
         }
+        ns = extract32(desc, 3, 1);
         /* Lookup l2 entry.  */
         table = (desc & 0xfffffc00) | ((address >> 10) & 0x3fc);
         desc = ldl_phys(cs->as, table);
@@ -5330,6 +5361,13 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type,
             goto do_fault;
         }
     }
+    if (ns) {
+        /* The NS bit will (as required by the architecture) have no effect if
+         * the CPU doesn't support TZ or this is a non-secure translation
+         * regime, because the attribute will already be non-secure.
+         */
+        *attrs &= ~MEMTXATTRS_SECURE;
+    }
     *phys_ptr = phys_addr;
     return 0;
 do_fault:
@@ -5347,7 +5385,7 @@ typedef enum {
 
 static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
                               int access_type, ARMMMUIdx mmu_idx,
-                              hwaddr *phys_ptr, int *prot,
+                              hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
                               target_ulong *page_size_ptr)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
@@ -5552,6 +5590,13 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         goto do_fault;
     }
 
+    if (ns) {
+        /* The NS bit will (as required by the architecture) have no effect if
+         * the CPU doesn't support TZ or this is a non-secure translation
+         * regime, because the attribute will already be non-secure.
+         */
+        *txattrs &= ~MEMTXATTRS_SECURE;
+    }
     *phys_ptr = descaddr;
     *page_size_ptr = page_size;
     return 0;
@@ -5635,8 +5680,8 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
  * by doing a translation table walk on MMU based systems or using the
  * MPU state on MPU based systems.
  *
- * Returns 0 if the translation was successful. Otherwise, phys_ptr,
- * prot and page_size are not filled in, and the return value provides
+ * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs,
+ * prot and page_size may not be filled in, and the return value provides
  * information on why the translation aborted, in the format of a
  * DFSR/IFSR fault register, with the following caveats:
  *  * we honour the short vs long DFSR format differences.
@@ -5649,12 +5694,13 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
  * @access_type: 0 for read, 1 for write, 2 for execute
  * @mmu_idx: MMU index indicating required translation regime
  * @phys_ptr: set to the physical address corresponding to the virtual address
+ * @attrs: set to the memory transaction attributes to use
  * @prot: set to the permissions for the page containing phys_ptr
  * @page_size: set to the size of the page containing phys_ptr
  */
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
                                 int access_type, ARMMMUIdx mmu_idx,
-                                hwaddr *phys_ptr, int *prot,
+                                hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
                                 target_ulong *page_size)
 {
     if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
@@ -5667,6 +5713,16 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
         mmu_idx += ARMMMUIdx_S1NSE0;
     }
 
+    if (regime_is_secure(env, mmu_idx)) {
+        /* The page table entries may downgrade this to non-secure, but
+         * cannot upgrade an non-secure translation regime's attributes
+         * to secure.
+         */
+        *attrs = MEMTXATTRS_SECURE;
+    } else {
+        *attrs = 0;
+    }
+
     /* Fast Context Switch Extension. This doesn't exist at all in v8.
      * In v7 and earlier it affects all stage 1 translations.
      */
@@ -5695,10 +5751,10 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
 
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
-                                  prot, page_size);
+                                  attrs, prot, page_size);
     } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
         return get_phys_addr_v6(env, address, access_type, mmu_idx, phys_ptr,
-                                prot, page_size);
+                                attrs, prot, page_size);
     } else {
         return get_phys_addr_v5(env, address, access_type, mmu_idx, phys_ptr,
                                 prot, page_size);
@@ -5716,14 +5772,16 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
     int ret;
     uint32_t syn;
     bool same_el = (arm_current_el(env) != 0);
+    MemTxAttrs attrs;
 
-    ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr, &prot,
-                        &page_size);
+    ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
+                        &attrs, &prot, &page_size);
     if (ret == 0) {
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
         address &= TARGET_PAGE_MASK;
-        tlb_set_page(cs, address, phys_addr, prot, mmu_idx, page_size);
+        tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
+                                prot, mmu_idx, page_size);
         return 0;
     }
 
@@ -5758,9 +5816,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
     target_ulong page_size;
     int prot;
     int ret;
+    MemTxAttrs attrs;
 
     ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env), &phys_addr,
-                        &prot, &page_size);
+                        &attrs, &prot, &page_size);
 
     if (ret != 0) {
         return -1;
-- 
1.9.1

Re: [Qemu-devel] [RFC 0/5] Memory transaction attributes API

[Edgar E. Iglesias]

On Mon, Mar 16, 2015 at 05:20:17PM +0000, Peter Maydell wrote:
> This is an RFC patchset aimed at getting comment on
> some memory API changes to support "transaction attributes",
> ie sideband information that goes along with a memory read
> or write access to define things like ARM secure/nonsecure,
> CPU/transaction master ID, privileged/nonprivileged.

Hi Peter,

Generally I think the direction of this looks very good, thanks!

A few comments:

1. Would it make sense to instead of having the MemTxAttrs be
a uint64_t instead have them be a struct with bitfields?
We could still pass the struct by value I think and as long
as it doesn't grow large the compiler will emit similar or
the same code IIUC.

2. The series introduces the read and write _with_attrs. Another
approach could be to add a (for example) single .access callback.
The callback could take a structure pointer as input, e.g:

struct MemTx {
    bool rw;
    hwaddr addr;
    int size;
    uint64_t data;
    MemTxAttrs attrs;
}

MemTxResult access(MemTx *tx)

The benefit I see is that this will make it easier for us in the
future to add new fields if needed.

Best regards,
Edgar


> 
> (See also previous discussion:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg01026.html )
> 
> Patch 1 is the API changes themselves:
> 1) new read_with_attrs and write_with_attrs fields in the
>    MemoryRegionOps struct; the old read/write still exist
>    for backwards compatibility, but devices that care about
>    attributes can register with these function pointers instead
> 2) the functions return a success/failure status, so a device
>    can actually fail bad transactions rather than merely
>    returning bogus data. (This isn't wired up in this patchset
>    but I include it to avoid revving the memory API twice.)
> 
> Patches 2, 3 and 4 then plumb the memory attribute parameters
> through the various functions, working upwards to being able
> to put them in the iotlb. Patch 5 implements the target-arm
> changes to provide a secure/nonsecure tx attribute based on
> the page table walk, as a demonstration.
> 
> There are obviously more APIs within QEMU for memory access
> functions which need to change to either always take a tx
> attribute, or to have extra with-tx-attribute versions of the
> functions. For the moment things are stubbed out with passing
> in "no attributes specified" values.
> 
> I've modelled the transaction attributes as a (typedefed)
> uint64_t, whose bits will be defined as we find requirements
> for them (the meaning will not be per-architecture). When
> we originally discussed this on-list, Edgar suggested making
> the attributes be a (pointer to a) struct; however I found
> the ownership/copying semantics on this too awkward, because
> the access path needs to take attributes set up in the TLB
> and then modify them according to details of the access
> actually being made before passing them to the device, so
> took the simpler implementation route.
> 
> I intend to continue working on this (filling in the gaps,
> etc), but wanted to send this series out early for comment
> on the memory API changes in particular.
> 
> thanks
> -- PMM
> 
> Peter Maydell (5):
>   memory: Define API for MemoryRegionOps to take attrs and return status
>   memory: Add MemTxAttrs argument to io_mem_read and io_mem_write
>   Make CPU iotlb a structure rather than a plain hwaddr
>   Add MemTxAttrs to the IOTLB
>   target-arm: Honour NS bits in page tables
> 
>  cputlb.c                 |  22 +++++++---
>  exec.c                   |  29 +++++++-------
>  hw/s390x/s390-pci-inst.c |   7 ++--
>  hw/vfio/pci.c            |   4 +-
>  include/exec/cpu-defs.h  |  15 ++++++-
>  include/exec/exec-all.h  |   8 +++-
>  include/exec/memattrs.h  |  37 +++++++++++++++++
>  include/exec/memory.h    |  22 ++++++++++
>  memory.c                 | 102 +++++++++++++++++++++++++++++++++++++----------
>  softmmu_template.h       |  36 +++++++++--------
>  target-arm/helper.c      |  83 ++++++++++++++++++++++++++++++++------
>  11 files changed, 288 insertions(+), 77 deletions(-)
>  create mode 100644 include/exec/memattrs.h
> 
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [RFC 0/5] Memory transaction attributes API

[Peter Maydell]

On 18 March 2015 at 08:38, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 2. The series introduces the read and write _with_attrs. Another
> approach could be to add a (for example) single .access callback.
> The callback could take a structure pointer as input, e.g:
>
> struct MemTx {
>     bool rw;
>     hwaddr addr;
>     int size;
>     uint64_t data;
>     MemTxAttrs attrs;
> }
>
> MemTxResult access(MemTx *tx)
>
> The benefit I see is that this will make it easier for us in the
> future to add new fields if needed.

I see the reasoning, but I looked at this and it just felt
awkward when I tried writing the code that way -- the code
calling the access function ends up having to create and
mutate this MemTx struct as it loops round calling the
access callback. In particular, memory.c has kept the
read and write paths separate all the way through, so
squashing them back together into a single callback
which will then immediately want to split them back out
into read vs write seemed a bit daft.

-- PMM

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Peter Maydell]

On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> Define an API so that devices can register MemoryRegionOps whose read
> and write callback functions are passed an arbitrary pointer to some
> transaction attributes and can return a success-or-failure status code.
> This will allow us to model devices which:
>  * behave differently for ARM Secure/NonSecure memory accesses
>  * behave differently for privileged/unprivileged accesses
>  * may return a transaction failure (causing a guest exception)
>    for erroneous accesses

> The success/failure response indication is currently ignored; it is
> provided so we can implement it later without having to change the
> callback function API yet again in future.

> +/* New-style MMIO accessors can indicate that the transaction failed.
> + * This is currently ignored, but provided in the API to allow us to add
> + * support later without changing the MemoryRegionOps functions yet again.
> + */
> +typedef enum {
> +    MemTx_OK = 0,
> +    MemTx_DecodeError = 1, /* "nothing at that address" */
> +    MemTx_SlaveError = 2,  /* "device unhappy with request" (eg misalignment) */
> +} MemTxResult;

So I was looking at how this would actually get plumbed through
the memory subsystem code, and there are some awkwardnesses
with this simple enum approach. In particular, functions like
address_space_rw want to combine the error returns from
several io_mem_read/write calls into a single response to
return to the caller. With an enum we'd need some pretty
ugly code to prioritise particular failure types, or to
do something arbitrary like "return first failure code".
Alternatively we could:
(a) make MemTxResult a uint32_t, where all-bits zero indicates
"OK" and any bit set indicates some kind of error, eg
bit 0 set for "device returned an error", and bit 1 for
"decode error", and higher bits available for other kinds
of extra info about errors in future. Then address_space_rw
just ORs together all the bits in all the return codes it
receives.
(b) give up and say "just use a bool"

Opinions?

-- PMM

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote:
> On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Define an API so that devices can register MemoryRegionOps whose read
> > and write callback functions are passed an arbitrary pointer to some
> > transaction attributes and can return a success-or-failure status code.
> > This will allow us to model devices which:
> >  * behave differently for ARM Secure/NonSecure memory accesses
> >  * behave differently for privileged/unprivileged accesses
> >  * may return a transaction failure (causing a guest exception)
> >    for erroneous accesses
> 
> > The success/failure response indication is currently ignored; it is
> > provided so we can implement it later without having to change the
> > callback function API yet again in future.
> 
> > +/* New-style MMIO accessors can indicate that the transaction failed.
> > + * This is currently ignored, but provided in the API to allow us to add
> > + * support later without changing the MemoryRegionOps functions yet again.
> > + */
> > +typedef enum {
> > +    MemTx_OK = 0,
> > +    MemTx_DecodeError = 1, /* "nothing at that address" */
> > +    MemTx_SlaveError = 2,  /* "device unhappy with request" (eg misalignment) */
> > +} MemTxResult;
> 
> So I was looking at how this would actually get plumbed through
> the memory subsystem code, and there are some awkwardnesses
> with this simple enum approach. In particular, functions like
> address_space_rw want to combine the error returns from
> several io_mem_read/write calls into a single response to
> return to the caller. With an enum we'd need some pretty
> ugly code to prioritise particular failure types, or to
> do something arbitrary like "return first failure code".
> Alternatively we could:
> (a) make MemTxResult a uint32_t, where all-bits zero indicates
> "OK" and any bit set indicates some kind of error, eg
> bit 0 set for "device returned an error", and bit 1 for
> "decode error", and higher bits available for other kinds
> of extra info about errors in future. Then address_space_rw
> just ORs together all the bits in all the return codes it
> receives.
> (b) give up and say "just use a bool"
> 
> Opinions?

Hi Peter,

Is this related to masters relying on the memory frameworks magic
handling of unaliged accesses?

I guess that masters that really care about accurate the error
handling would need to issue transactions without relying on
the intermediate "magic" that splits unaligned accesses...

Anyway, I think your option a sounds the most flexible...

Cheers,
Edgar

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Paolo Bonzini]

On 27/03/2015 13:02, Edgar E. Iglesias wrote:
> On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote:
>> On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Define an API so that devices can register MemoryRegionOps whose read
>>> and write callback functions are passed an arbitrary pointer to some
>>> transaction attributes and can return a success-or-failure status code.
>>> This will allow us to model devices which:
>>>  * behave differently for ARM Secure/NonSecure memory accesses
>>>  * behave differently for privileged/unprivileged accesses
>>>  * may return a transaction failure (causing a guest exception)
>>>    for erroneous accesses
>>
>>> The success/failure response indication is currently ignored; it is
>>> provided so we can implement it later without having to change the
>>> callback function API yet again in future.
>>
>>> +/* New-style MMIO accessors can indicate that the transaction failed.
>>> + * This is currently ignored, but provided in the API to allow us to add
>>> + * support later without changing the MemoryRegionOps functions yet again.
>>> + */
>>> +typedef enum {
>>> +    MemTx_OK = 0,
>>> +    MemTx_DecodeError = 1, /* "nothing at that address" */
>>> +    MemTx_SlaveError = 2,  /* "device unhappy with request" (eg misalignment) */
>>> +} MemTxResult;
>>
>> So I was looking at how this would actually get plumbed through
>> the memory subsystem code, and there are some awkwardnesses
>> with this simple enum approach. In particular, functions like
>> address_space_rw want to combine the error returns from
>> several io_mem_read/write calls into a single response to
>> return to the caller. With an enum we'd need some pretty
>> ugly code to prioritise particular failure types, or to
>> do something arbitrary like "return first failure code".
>> Alternatively we could:
>> (a) make MemTxResult a uint32_t, where all-bits zero indicates
>> "OK" and any bit set indicates some kind of error, eg
>> bit 0 set for "device returned an error", and bit 1 for
>> "decode error", and higher bits available for other kinds
>> of extra info about errors in future. Then address_space_rw
>> just ORs together all the bits in all the return codes it
>> receives.
>> (b) give up and say "just use a bool"
>>
>> Opinions?
> 
> Hi Peter,
> 
> Is this related to masters relying on the memory frameworks magic
> handling of unaliged accesses?

Not necessarily, you can get the same just by doing a large write that
spans multiple MemoryRegions.  See the loop in address_space_rw.

> Anyway, I think your option a sounds the most flexible...

ACK

Paolo

> Cheers,
> Edgar
> 

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Peter Maydell]

On 27 March 2015 at 12:02, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote:
>> So I was looking at how this would actually get plumbed through
>> the memory subsystem code, and there are some awkwardnesses
>> with this simple enum approach. In particular, functions like
>> address_space_rw want to combine the error returns from
>> several io_mem_read/write calls into a single response to
>> return to the caller. With an enum we'd need some pretty
>> ugly code to prioritise particular failure types, or to
>> do something arbitrary like "return first failure code".
>> Alternatively we could:
>> (a) make MemTxResult a uint32_t, where all-bits zero indicates
>> "OK" and any bit set indicates some kind of error, eg
>> bit 0 set for "device returned an error", and bit 1 for
>> "decode error", and higher bits available for other kinds
>> of extra info about errors in future. Then address_space_rw
>> just ORs together all the bits in all the return codes it
>> receives.
>> (b) give up and say "just use a bool"

> Is this related to masters relying on the memory frameworks magic
> handling of unaliged accesses?

Well, that, and masters that just want to say "write
this entire buffer" or otherwise access at larger
than the destination's access size.

> I guess that masters that really care about accurate the error
> handling would need to issue transactions without relying on
> the intermediate "magic" that splits unaligned accesses...

Yes, I think this is probably true. (I suspect we don't
actually care at that level of detail.)

> Anyway, I think your option a sounds the most flexible...

Yes, it's the best thing I can think of currently.

-- PMM

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 01:10:07PM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/03/2015 13:02, Edgar E. Iglesias wrote:
> > On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote:
> >> On 16 March 2015 at 17:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> Define an API so that devices can register MemoryRegionOps whose read
> >>> and write callback functions are passed an arbitrary pointer to some
> >>> transaction attributes and can return a success-or-failure status code.
> >>> This will allow us to model devices which:
> >>>  * behave differently for ARM Secure/NonSecure memory accesses
> >>>  * behave differently for privileged/unprivileged accesses
> >>>  * may return a transaction failure (causing a guest exception)
> >>>    for erroneous accesses
> >>
> >>> The success/failure response indication is currently ignored; it is
> >>> provided so we can implement it later without having to change the
> >>> callback function API yet again in future.
> >>
> >>> +/* New-style MMIO accessors can indicate that the transaction failed.
> >>> + * This is currently ignored, but provided in the API to allow us to add
> >>> + * support later without changing the MemoryRegionOps functions yet again.
> >>> + */
> >>> +typedef enum {
> >>> +    MemTx_OK = 0,
> >>> +    MemTx_DecodeError = 1, /* "nothing at that address" */
> >>> +    MemTx_SlaveError = 2,  /* "device unhappy with request" (eg misalignment) */
> >>> +} MemTxResult;
> >>
> >> So I was looking at how this would actually get plumbed through
> >> the memory subsystem code, and there are some awkwardnesses
> >> with this simple enum approach. In particular, functions like
> >> address_space_rw want to combine the error returns from
> >> several io_mem_read/write calls into a single response to
> >> return to the caller. With an enum we'd need some pretty
> >> ugly code to prioritise particular failure types, or to
> >> do something arbitrary like "return first failure code".
> >> Alternatively we could:
> >> (a) make MemTxResult a uint32_t, where all-bits zero indicates
> >> "OK" and any bit set indicates some kind of error, eg
> >> bit 0 set for "device returned an error", and bit 1 for
> >> "decode error", and higher bits available for other kinds
> >> of extra info about errors in future. Then address_space_rw
> >> just ORs together all the bits in all the return codes it
> >> receives.
> >> (b) give up and say "just use a bool"
> >>
> >> Opinions?
> > 
> > Hi Peter,
> > 
> > Is this related to masters relying on the memory frameworks magic
> > handling of unaliged accesses?
> 
> Not necessarily, you can get the same just by doing a large write that
> spans multiple MemoryRegions.  See the loop in address_space_rw.

Right, this is another case of "magic" memory handling that allows masters
to issue unnatural transactions and rely on the memory framework to
split things up.
In these cases aren't the masters trading accuracy (including error
handling accuracy) for performance or model simplicity?

It could maybe be useful to have a flag so masters can say one of the
following (could be encoded in the memattrs):
1. Stop at first error and return.
2. Keep going after errors and give me the OR result of all errors.

For 1 to be useful, I think it would have to be combined with some
kind of return info that can point out where in the magic splitting
of large or unaligned transactions that things went wrong.

Probably overkill at the moment...

Cheers,
Edgar

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Paolo Bonzini]

On 27/03/2015 13:32, Edgar E. Iglesias wrote:
>>> Is this related to masters relying on the memory frameworks magic
>>> handling of unaliged accesses?
>>
>> Not necessarily, you can get the same just by doing a large write that
>> spans multiple MemoryRegions.  See the loop in address_space_rw.
> 
> Right, this is another case of "magic" memory handling that allows masters
> to issue unnatural transactions and rely on the memory framework to
> split things up.
> In these cases aren't the masters trading accuracy (including error
> handling accuracy) for performance or model simplicity?

Yes.  There are no "natural" transactions beyond 32 or 64-bit accesses.

> It could maybe be useful to have a flag so masters can say one of the
> following (could be encoded in the memattrs):
> 1. Stop at first error and return.
> 2. Keep going after errors and give me the OR result of all errors.

It could just be a length pointer in the same vein as
address_space_map's.  If NULL, keep going.  If not NULL, stop and return.

Paolo

> 
> For 1 to be useful, I think it would have to be combined with some
> kind of return info that can point out where in the magic splitting
> of large or unaligned transactions that things went wrong.
> 
> Probably overkill at the moment...
> 
> Cheers,
> Edgar
> 

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 02:16:53PM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/03/2015 13:32, Edgar E. Iglesias wrote:
> >>> Is this related to masters relying on the memory frameworks magic
> >>> handling of unaliged accesses?
> >>
> >> Not necessarily, you can get the same just by doing a large write that
> >> spans multiple MemoryRegions.  See the loop in address_space_rw.
> > 
> > Right, this is another case of "magic" memory handling that allows masters
> > to issue unnatural transactions and rely on the memory framework to
> > split things up.
> > In these cases aren't the masters trading accuracy (including error
> > handling accuracy) for performance or model simplicity?
> 
> Yes.  There are no "natural" transactions beyond 32 or 64-bit accesses.
> 
> > It could maybe be useful to have a flag so masters can say one of the
> > following (could be encoded in the memattrs):
> > 1. Stop at first error and return.
> > 2. Keep going after errors and give me the OR result of all errors.
> 
> It could just be a length pointer in the same vein as
> address_space_map's.  If NULL, keep going.  If not NULL, stop and return.
> 

Good point, thanks.

Cheers,
Edgar