Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-11 Thread Edgar E. Iglesias 
On Fri, Apr 10, 2015 at 03:51:07PM +0100, Peter Maydell wrote:
 On 10 April 2015 at 03:07, Edgar E. Iglesias edgar.igles...@gmail.com wrote:
  On Thu, Apr 09, 2015 at 11:21:26AM +0200, Paolo Bonzini wrote:
  On 09/04/2015 11:04, Peter Maydell wrote:
   We discussed this last time round, I think. Whether structs get
   passed in registers depends on the host CPU ABI/calling convention.
 
  Because of C++, structs up to pointer size are in practice always passed
  in registers.  64-bit structs may or may not.
 
  The main advantage of structs is that it's impossible to mismatch the
  parameter order.  That even trumps readability in my opinion.
 
  I'm ambivalent, but I wouldn't mind at all using structs.
 
  Thanks for clarifying that Paolo.
 
  Yes, the manual bit masking and shifting is easier to get wrong.
  The struct also has stronger type checking in a way, as you cant OR in 
  literals
  that are out of bounds. (IIRC GCC will even warn you for free).
  The struct is also easy to extend if we ever run out of bits in the 
  uint64_t.
 
  Peter, would you consider switching to struct or are you still convinced
  of the pure uint64_t approach?
 
 OK, having thought about this I'm willing to take the struct-and-bitfields
 approach. My preferences are somewhat based on habit and also on some
 of Linus' rants about bitfields for kernel use [eg
 http://yarchive.net/comp/linux/bitfields.html], but I think we are
 not going to hit any of the problem cases (notably, we don't care
 about the arrangement of the bitfields within the word, we aren't
 trying to have bitfields and locks/volatile/atomic info shared in
 one struct, and we don't have a particular need to test multiple
 bits at once).
 
 I'll change over to structs for v2.

Awesome, thanks!

Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-09 Thread Peter Maydell 
On 9 April 2015 at 09:55, Edgar E. Iglesias edgar.igles...@gmail.com wrote:
 Did you consider using a struct here?
 e.g:

 typedef struct MemTxAttrs {
 unsigned int secure : 1;
 unsigned int master_id : 10;
 unsigned int etc : 1;
 } MemTxAttrs;

 I think you could still pass it by value and my understanding is
 that the compiler will generate similar code.

We discussed this last time round, I think. Whether structs get
passed in registers depends on the host CPU ABI/calling convention.

 I find it more readable, you ca go:

 attrs.secure = 1;
 attrs.master_id = 0x77;
 if (!attrs.secure)

 instead of:

 attrs |= MEMTXATTRS_SECURE
 if (!(attrs  MEMTXATTRS_SECURE))

 etc...

 Or do you see any disadvantages with this?

I prefer the traditional integer-and-bitops approach, then you
know what you're getting everywhere...

-- PMM

Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-09 Thread Edgar E. Iglesias 
On Tue, Apr 07, 2015 at 09:09:47PM +0100, Peter Maydell 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
 
 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 also propagated out to
 io_mem_read() and io_mem_write(), which retain the old-style
 boolean true-for-error return.
 
 Signed-off-by: Peter Maydell peter.mayd...@linaro.org
 ---
  include/exec/memattrs.h |  34 
  include/exec/memory.h   |  22 +
  memory.c| 207 
 
  3 files changed, 196 insertions(+), 67 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 000..b8d7808
 --- /dev/null
 +++ b/include/exec/memattrs.h
 @@ -0,0 +1,34 @@
 +/*
 + * Memory transaction attributes
 + *
 + * Copyright (c) 2015 Linaro Limited.
 + *
 + * Authors:
 + *  Peter Maydell peter.mayd...@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;

Hi,

Did you consider using a struct here?
e.g:

typedef struct MemTxAttrs {
unsigned int secure : 1;
unsigned int master_id : 10;
unsigned int etc : 1;
} MemTxAttrs;

I think you could still pass it by value and my understanding is
that the compiler will generate similar code.

I find it more readable, you ca go:

attrs.secure = 1;
attrs.master_id = 0x77;
if (!attrs.secure)

instead of: 

attrs |= MEMTXATTRS_SECURE
if (!(attrs  MEMTXATTRS_SECURE))

etc...

Or do you see any disadvantages with this?



 +
 +/* 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..703d9e5 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.
 + * A zero (MEMTX_OK) response means success; anything else is a failure
 + * of some kind. The memory subsystem will bitwise-OR together results
 + * if it is synthesizing an operation from multiple smaller accesses.
 + */
 +#define MEMTX_OK 0
 +#define MEMTX_ERROR (1U  0) /* device returned an error */
 +#define MEMTX_DECODE_ERROR  (1U  1) /* nothing at that address */
 +typedef uint32_t 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 ee3f2a8..9bb5674 100644
 --- a/memory.c
 +++ b/memory.c
 @@ -368,57 +368,84 @@ static void adjust_endianness(MemoryRegion *mr, 
 uint64_t *data, unsigned size)
  }
  }
  
 -static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 +static MemTxResult

Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-09 Thread Paolo Bonzini 


On 09/04/2015 11:04, Peter Maydell wrote:
 We discussed this last time round, I think. Whether structs get
 passed in registers depends on the host CPU ABI/calling convention.

Because of C++, structs up to pointer size are in practice always passed
in registers.  64-bit structs may or may not.

The main advantage of structs is that it's impossible to mismatch the
parameter order.  That even trumps readability in my opinion.

I'm ambivalent, but I wouldn't mind at all using structs.

Paolo

  I find it more readable, you ca go:
 
  attrs.secure = 1;
  attrs.master_id = 0x77;
  if (!attrs.secure)
 
  instead of:
 
  attrs |= MEMTXATTRS_SECURE
  if (!(attrs  MEMTXATTRS_SECURE))
 
  etc...
 
  Or do you see any disadvantages with this?
 I prefer the traditional integer-and-bitops approach, then you
 know what you're getting everywhere...

Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-09 Thread Edgar E. Iglesias 
On Thu, Apr 09, 2015 at 10:04:39AM +0100, Peter Maydell wrote:
 On 9 April 2015 at 09:55, Edgar E. Iglesias edgar.igles...@gmail.com wrote:
  Did you consider using a struct here?
  e.g:
 
  typedef struct MemTxAttrs {
  unsigned int secure : 1;
  unsigned int master_id : 10;
  unsigned int etc : 1;
  } MemTxAttrs;
 
  I think you could still pass it by value and my understanding is
  that the compiler will generate similar code.
 
 We discussed this last time round, I think. Whether structs get
 passed in registers depends on the host CPU ABI/calling convention.

Ah sorry, I must have missed that response...

 
  I find it more readable, you ca go:
 
  attrs.secure = 1;
  attrs.master_id = 0x77;
  if (!attrs.secure)
 
  instead of:
 
  attrs |= MEMTXATTRS_SECURE
  if (!(attrs  MEMTXATTRS_SECURE))
 
  etc...
 
  Or do you see any disadvantages with this?
 
 I prefer the traditional integer-and-bitops approach, then you
 know what you're getting everywhere...

OK :-)

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-08 Thread Paolo Bonzini 


On 07/04/2015 22:09, Peter Maydell 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
 
 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 also propagated out to
 io_mem_read() and io_mem_write(), which retain the old-style
 boolean true-for-error return.
 
 Signed-off-by: Peter Maydell peter.mayd...@linaro.org
 ---
  include/exec/memattrs.h |  34 
  include/exec/memory.h   |  22 +
  memory.c| 207 
 
  3 files changed, 196 insertions(+), 67 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 000..b8d7808
 --- /dev/null
 +++ b/include/exec/memattrs.h
 @@ -0,0 +1,34 @@
 +/*
 + * Memory transaction attributes
 + *
 + * Copyright (c) 2015 Linaro Limited.
 + *
 + * Authors:
 + *  Peter Maydell peter.mayd...@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..703d9e5 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.
 + * A zero (MEMTX_OK) response means success; anything else is a failure
 + * of some kind. The memory subsystem will bitwise-OR together results
 + * if it is synthesizing an operation from multiple smaller accesses.
 + */
 +#define MEMTX_OK 0
 +#define MEMTX_ERROR (1U  0) /* device returned an error */
 +#define MEMTX_DECODE_ERROR  (1U  1) /* nothing at that address */
 +typedef uint32_t 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 ee3f2a8..9bb5674 100644
 --- a/memory.c
 +++ b/memory.c
 @@ -368,57 +368,84 @@ static void adjust_endianness(MemoryRegion *mr, 
 uint64_t *data, unsigned size)
  }
  }
  
 -static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 +static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 +   hwaddr addr,
 +   uint64_t *value,
 +   unsigned size,
 +   unsigned shift,
 +   uint64_t mask,
 +   MemTxAttrs attrs)
 +{
 +uint64_t tmp;
 +
 +tmp =

[Qemu-devel] [PATCH 01/14] memory: Define API for MemoryRegionOps to take attrs and return status

2015-04-07 Thread 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 also propagated out to
io_mem_read() and io_mem_write(), which retain the old-style
boolean true-for-error return.

Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
 include/exec/memattrs.h |  34 
 include/exec/memory.h   |  22 +
 memory.c| 207 
 3 files changed, 196 insertions(+), 67 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 000..b8d7808
--- /dev/null
+++ b/include/exec/memattrs.h
@@ -0,0 +1,34 @@
+/*
+ * Memory transaction attributes
+ *
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Authors:
+ *  Peter Maydell peter.mayd...@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..703d9e5 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.
+ * A zero (MEMTX_OK) response means success; anything else is a failure
+ * of some kind. The memory subsystem will bitwise-OR together results
+ * if it is synthesizing an operation from multiple smaller accesses.
+ */
+#define MEMTX_OK 0
+#define MEMTX_ERROR (1U  0) /* device returned an error */
+#define MEMTX_DECODE_ERROR  (1U  1) /* nothing at that address */
+typedef uint32_t 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 ee3f2a8..9bb5674 100644
--- a/memory.c
+++ b/memory.c
@@ -368,57 +368,84 @@ static void adjust_endianness(MemoryRegion *mr, uint64_t 
*data, unsigned size)
 }
 }
 
-static void memory_region_oldmmio_read_accessor(MemoryRegion *mr,
+static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
+   hwaddr addr,
+   uint64_t *value,
+   unsigned size,
+   unsigned shift,
+   uint64_t mask,
+   MemTxAttrs attrs)
+{
+uint64_t tmp;
+
+tmp = mr-ops-old_mmio.read[ctz32(size)](mr-opaque, addr);
+trace_memory_region_ops_read(mr, addr, tmp, size);
+*value |= (tmp  mask)  shift;
+return MEMTX_OK;
+}
+
+static MemTxResult