[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 <address@hidden>
---
 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 <address@hidden>
+ *
+ * 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