[Qemu-devel] [PATCH v2 0/5] Data Driven device registers & Zynq DEVCFG

[Qemu-devel] [PATCH v2 0/5] Data Driven device registers & Zynq DEVCFG

[Peter Crosthwaite]

Hi All. This is a new scheme i've come up with handling device registers in a
data driven way. My motivation for this is to factor out a lot of the access
checking that seems to be replicated in every device. See P2 commit message for
further discussion.

P1 is a trivial addition to bitops.h
P2 is the main patch, adds the register definition functionality
P3 is an example new device (the Xilinx Zynq devcfg) that uses this scheme.
P4 adds devcfg to the Zynq machine model

This devcfg device was particularly finnicky with per-bit restrictions which
prompted all this. Im also looking for a higher-than-usual modelling fidelity
on the register space, with semantics defined for random reserved bits
in-between otherwise consistent fields.

Heres an example of the qemu_log output for the devcfg device. This is produced
by now generic sharable code:

/machine/unattached/device[44]:Addr 0x000008:CFG: write of value 00000508
/machine/unattached/device[44]:Addr 0x000080:MCTRL: write of value 00800010
/machine/unattached/device[44]:Addr 0x000010:INT_MASK: write of value ffffffff
/machine/unattached/device[44]:Addr 00000000:CTRL: write of value 0c00607f

And an example of a rogue guest banging on a bad bit:

/machine/unattached/device[44]:Addr 0x000014:STATUS bits 0x000001 may not be \
								written to 1

Future work: Theres a lot of overlap here with what Peter did with the ARM
coprocessor definitions. We could go further and generalise ARM CP to use this
or some further evolution of it. That and converting existing models to this
scheme. Some device models will lose a lot of weight. Also integrate with
memory API.

Changed from v1:
Added ONES macro patch
Dropped bogus former patch 1 (PMM review)
Addressed Blue, Gerd and MST comments.
Simplified to be more Memory API compatible.
Added Memory API helpers.
Please see discussion already on list and commit msgs for more detail.


Peter A. G. Crosthwaite (2):
  xilinx_devcfg: Zynq devcfg device model
  xilinx_zynq: added devcfg to machine model

Peter Crosthwaite (3):
  bitops: Add ONES macro
  register: Add Register API
  register: Add Memory API glue

 Makefile.target         |    2 +-
 hw/arm/Makefile.objs    |    2 +-
 hw/arm/xilinx_zynq.c    |    8 +
 hw/xilinx_devcfg.c      |  489 +++++++++++++++++++++++++++++++++++++++++++++++
 include/exec/register.h |  147 ++++++++++++++
 include/qemu/bitops.h   |    2 +
 register.c              |  207 ++++++++++++++++++++
 7 files changed, 855 insertions(+), 2 deletions(-)
 create mode 100644 hw/xilinx_devcfg.c
 create mode 100644 include/exec/register.h
 create mode 100644 register.c

[Qemu-devel] [PATCH v2 1/5] bitops: Add ONES macro

[Peter Crosthwaite]

Little macro that just gives you N ones (justified to LSB).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/qemu/bitops.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index affcc96..da47fc8 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -273,4 +273,6 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
     return (value & ~mask) | ((fieldval << start) & mask);
 }
 
+#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1)
+
 #endif
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 2/5] register: Add Register API

[Peter Crosthwaite]

This API provides some encapsulation of registers and factors our some
common functionality to common code. Bits of device state (usually MMIO
registers), often have all sorts of access restrictions and semantics
associated with them. This API allow you to define what those
restrictions are on a bit-by-bit basis.

Helper functions are then used to access the register which observe the
semantics defined by the RegisterAccessInfo struct.

Some features:
Bits can be marked as read_only (ro field)
Bits can be marked as write-1-clear (w1c field)
Bits can be marked as write only (wo)
Bits can be marked as sticky (nw0 and nw1)
Reset values can be defined (reset)
Bits can throw guest errors when written certain values (ge0, ge1)
Bits can throw unimp errors when written certain values (ui0, ui1)
Bits can be marked clear on read (cor)
Pre and post action callbacks can be added to read and write ops
Verbose debugging info can be enabled/disabled

Useful for defining device register spaces in a data driven way. Cuts
down on a lot of the verbosity and repetition in the switch-case blocks
in the standard foo_mmio_read/write functions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v1:
Rebranded as the "Register API" - I think thats probably what it is.
Near total rewrite of implementation.
De-arrayified reset (this is client/Memory APIs job).
Moved out of bitops into its own file (Blue review)
Added debug, the register pointer, and prefix to a struct (Blue Review)
Made 64-bit to play friendlier with memory API (Blue review)
Made backend storage uint8_t (MST review)
Added read/write callbacks (Blue review)
Added ui0, ui1 (Blue review)
Moved re-purposed width (now byte width defining actual storage size)
Arrayified ge0, ge1 (ui0, ui1 too) and added .reason
Added wo field (not an April fools joke - this has genuine meaning here)
Added we mask to write accessor

 Makefile.target         |    2 +-
 include/exec/register.h |  134 ++++++++++++++++++++++++++++++++++++++
 register.c              |  164 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 299 insertions(+), 1 deletions(-)
 create mode 100644 include/exec/register.h
 create mode 100644 register.c

diff --git a/Makefile.target b/Makefile.target
index 2bd6d14..9c35931 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -114,7 +114,7 @@ obj-y += hw/
 obj-$(CONFIG_FDT) += device_tree.o
 obj-$(CONFIG_KVM) += kvm-all.o
 obj-$(CONFIG_NO_KVM) += kvm-stub.o
-obj-y += memory.o savevm.o cputlb.o
+obj-y += memory.o register.o savevm.o cputlb.o
 obj-$(CONFIG_HAVE_GET_MEMORY_MAPPING) += memory_mapping.o
 obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o
 obj-$(CONFIG_NO_GET_MEMORY_MAPPING) += memory_mapping-stub.o
diff --git a/include/exec/register.h b/include/exec/register.h
new file mode 100644
index 0000000..0b05439
--- /dev/null
+++ b/include/exec/register.h
@@ -0,0 +1,134 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2013 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef REGISTER_H
+#define REGISTER_H
+
+#include <stdint.h>
+#include <stdbool.h>
+
+typedef struct RegisterInfo RegisterInfo;
+typedef struct RegisterAccessInfo RegisterAccessInfo;
+
+/**
+ * A register access error message
+ * @mask: Bits in the register the error applies to
+ * @reason: Reason why this access is an error
+ */
+
+typedef struct RegisterAccessError {
+    uint64_t mask;
+    const char *reason;
+} RegisterAccessError;
+
+/**
+ * Access description for a register that is part of guest accessible device
+ * state.
+ *
+ * @name: String name of the register
+ * @ro: whether or not the bit is read-only
+ * @wo: Bits that are write only (read as reset value)
+ * @w1c: bits with the common write 1 to clear semantic.
+ * @nw0: bits that can't be written with a 0 by the guest (sticky 1)
+ * @nw1: bits that can't be written with a 1 by the guest (sticky 0)
+ * @reset: reset value.
+ * @cor: Bits that are clear on read
+ *
+ * @ge1: Bits that when written 1 indicate a guest error
+ * @ge0: Bits that when written 0 indicate a guest error
+ * @ui1: Bits that when written 1 indicate use of an unimplemented feature
+ * @ui0: Bits that when written 0 indicate use of an unimplemented feature
+ *
+ * @pre_write: Pre write callback. Passed the value that's to be written,
+ * immediately before the actual write. The returned value is what is written,
+ * giving the handler a chance to modify the written value.
+ * @post_write: Post write callback. Passed the written value. Most write side
+ * effects should be implemented here.
+ *
+ * @pre_read: Pre read callback.
+ * @post_read: Post read callback. Passes the value that is about to be returned
+ * for a read. The return value from this function is what is ultimately read,
+ * allowing this function to modify the value before return to the client.
+ */
+
+struct RegisterAccessInfo {
+    const char *name;
+    uint64_t ro;
+    uint64_t wo;
+    uint64_t w1c;
+    uint64_t nw0;
+    uint64_t nw1;
+    uint64_t reset;
+    uint64_t cor;
+
+    const RegisterAccessError *ge0;
+    const RegisterAccessError *ge1;
+    const RegisterAccessError *ui0;
+    const RegisterAccessError *ui1;
+
+    uint64_t (*pre_write)(RegisterInfo *reg, uint64_t val);
+    void (*post_write)(RegisterInfo *reg, uint64_t val);
+
+    void (*pre_read)(RegisterInfo *reg);
+    uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
+};
+
+/**
+ * A register that is part of guest accessible state
+ * @data: pointer to the register data
+ * @data_size: Size of the register in bytes
+ * @data_big_endian: Define endianess of data register
+ *
+ * @access: Access desciption of this register
+ *
+ * @debug: Whether or not verbose debug is enabled
+ * @prefix: String prefix for log and debug messages
+ *
+ * @opaque: Opaque data for the register
+ */
+
+struct RegisterInfo {
+    uint8_t *data;
+    int data_size;
+    bool data_big_endian;
+
+    const RegisterAccessInfo *access;
+
+    bool debug;
+    const char *prefix;
+
+    void *opaque;
+};
+
+/**
+ * write a value to a register, subject to its restrictions
+ * @reg: register to write to
+ * @val: value to write
+ * @we: write enable mask
+ */
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
+
+/**
+ * read a value from a register, subject to its restrictions
+ * @reg: register to read from
+ * returns: value read
+ */
+
+uint64_t register_read(RegisterInfo *reg);
+
+/**
+ * reset a register
+ * @reg: register to reset
+ */
+
+void register_reset(RegisterInfo *reg);
+
+#endif
diff --git a/register.c b/register.c
new file mode 100644
index 0000000..439f2f2
--- /dev/null
+++ b/register.c
@@ -0,0 +1,164 @@
+/*
+ * Register Definition API
+ *
+ * Copyright (c) 2013 Xilinx Inc.
+ * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "exec/register.h"
+#include "qemu/log.h"
+
+static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
+                                      int mask, const char *msg,
+                                      const char *reason)
+{
+    qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
+                  reg->prefix, reg->access->name, val, msg, dir,
+                  reason ? ": " : "", reason ? reason : "");
+}
+
+static inline void register_write_val(RegisterInfo *reg, uint64_t val)
+{
+    int i;
+
+    for (i = 0; i < reg->data_size; ++i) {
+        reg->data[i] = val >> (reg->data_big_endian ?
+                    8 * (reg->data_size - 1 - i) : 8 * i);
+    }
+}
+
+static inline uint64_t register_read_val(RegisterInfo *reg)
+{
+    uint64_t ret = 0;
+    int i;
+
+    for (i = 0; i < reg->data_size; ++i) {
+        ret |= (uint64_t)reg->data[i] << (reg->data_big_endian ?
+                    8 * (reg->data_size - 1 - i) : 8 * i);
+    }
+    return ret;
+}
+
+void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
+{
+    uint64_t old_val, new_val, test;
+    const RegisterAccessInfo *ac;
+    const RegisterAccessError *rae;
+
+    assert(reg);
+
+    ac = reg->access;
+    if (!ac || !ac->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
+                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
+        return;
+    }
+
+    uint32_t no_w0_mask = ac->ro | ac->w1c | ac->nw0 | ~we;
+    uint32_t no_w1_mask = ac->ro | ac->w1c | ac->nw1 | ~we;
+
+    if (reg->debug) {
+        fprintf(stderr, "%s:%s: write of value %#" PRIx64 "\n", reg->prefix,
+                ac->name, val);
+    }
+
+    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
+        for (rae = ac->ge1; rae && rae->mask; rae++) {
+            test = val & rae->mask;
+            if (test) {
+                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
+                                   "invalid", rae->reason);
+            }
+        }
+        for (rae = ac->ge0; rae && rae->mask; rae++) {
+            test = val & rae->mask;
+            if (test) {
+                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
+                                   "invalid", rae->reason);
+            }
+        }
+    }
+
+    if (qemu_loglevel_mask(LOG_UNIMP)) {
+        for (rae = ac->ui1; rae && rae->mask; rae++) {
+            test = val & rae->mask;
+            if (test) {
+                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
+                                   "unimplmented", rae->reason);
+            }
+        }
+        for (rae = ac->ui0; rae && rae->mask; rae++) {
+            test = val & rae->mask;
+            if (test) {
+                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
+                                   "unimplemented", rae->reason);
+            }
+        }
+    }
+
+    assert(reg->data);
+    old_val = register_read_val(reg);
+
+    new_val = val & ~(no_w1_mask & val);
+    new_val |= no_w1_mask & old_val & val;
+    new_val |= no_w0_mask & old_val & ~val;
+    new_val &= ~(val & ac->w1c);
+
+    if (ac->pre_write) {
+        new_val = ac->pre_write(reg, new_val);
+    }
+    register_write_val(reg, new_val);
+    if (ac->post_write) {
+        ac->post_write(reg, new_val);
+    }
+}
+
+uint64_t register_read(RegisterInfo *reg)
+{
+    uint64_t ret;
+    const RegisterAccessInfo *ac;
+
+    assert(reg);
+
+    ac = reg->access;
+    if (!ac || !ac->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state\n",
+                      reg->prefix);
+        return 0;
+    }
+
+    assert(reg->data);
+    if (ac->pre_read) {
+        ac->pre_read(reg);
+    }
+    ret = register_read_val(reg);
+
+    register_write_val(reg, ret & ~ac->cor);
+
+    ret &= ~ac->wo;
+    ret |= ac->wo & ac->reset;
+
+    if (ac->post_read) {
+        ret = ac->post_read(reg, ret);
+    }
+    if (reg->debug) {
+        fprintf(stderr, "%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
+                ac->name, ret);
+    }
+
+    return ret;
+}
+
+void register_reset(RegisterInfo *reg)
+{
+    assert(reg);
+
+    if (!reg->data || !reg->access) {
+        return;
+    }
+
+    register_write_val(reg, reg->access->reset);
+}
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 3/5] register: Add Memory API glue

[Peter Crosthwaite]

Add memory io handlers that glue the register API to the memory API.
Just translation functions at this stage. Although it does allow for
devices to be created without all-in-one mmio r/w handlers.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/exec/register.h |   13 +++++++++++++
 register.c              |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/include/exec/register.h b/include/exec/register.h
index 0b05439..f30c98e 100644
--- a/include/exec/register.h
+++ b/include/exec/register.h
@@ -13,6 +13,7 @@
 
 #include <stdint.h>
 #include <stdbool.h>
+#include "exec/memory.h"
 
 typedef struct RegisterInfo RegisterInfo;
 typedef struct RegisterAccessInfo RegisterAccessInfo;
@@ -92,6 +93,8 @@ struct RegisterAccessInfo {
  * @prefix: String prefix for log and debug messages
  *
  * @opaque: Opaque data for the register
+ *
+ * @mem: optional Memory region for the register
  */
 
 struct RegisterInfo {
@@ -105,6 +108,8 @@ struct RegisterInfo {
     const char *prefix;
 
     void *opaque;
+
+    MemoryRegion mem;
 };
 
 /**
@@ -131,4 +136,12 @@ uint64_t register_read(RegisterInfo *reg);
 
 void register_reset(RegisterInfo *reg);
 
+void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
+                              unsigned size);
+void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
+                              unsigned size);
+
+uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
+uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
+
 #endif
diff --git a/register.c b/register.c
index 439f2f2..7b7b6df 100644
--- a/register.c
+++ b/register.c
@@ -162,3 +162,46 @@ void register_reset(RegisterInfo *reg)
 
     register_write_val(reg, reg->access->reset);
 }
+
+static inline void register_write_memory(void *opaque, hwaddr addr,
+                                         uint64_t value, unsigned size, bool be)
+{
+    RegisterInfo *reg = opaque;
+    uint64_t we = (size == 8) ? ~0ull : (1ull << size * 8) - 1;
+    int shift = 8 * (be ? reg->data_size - size - addr : addr);
+
+    assert(size + addr <= reg->data_size);
+    register_write(reg, value << shift, we << shift);
+}
+
+void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
+                              unsigned size)
+{
+    register_write_memory(opaque, addr, value, size, true);
+}
+
+
+void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
+                              unsigned size)
+{
+    register_write_memory(opaque, addr, value, size, false);
+}
+
+static inline uint64_t register_read_memory(void *opaque, hwaddr addr,
+                                            unsigned size, bool be)
+{
+    RegisterInfo *reg = opaque;
+    int shift = 8 * (be ? reg->data_size - size - addr : addr);
+
+    return register_read(reg) >> shift;
+}
+
+uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size)
+{
+    return register_read_memory(opaque, addr, size, true);
+}
+
+uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
+{
+    return register_read_memory(opaque, addr, size, false);
+}
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 4/5] xilinx_devcfg: Zynq devcfg device model

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

Minimal device model for devcfg module of Zynq. DMA capabilities and
interrupt generation supported.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
Changed since v1:
Rebased against new version of Register API.
Use action callbacks for side effects rather than switch.
Documented reasons for ge0, ge1 (Verbatim from TRM)
Added ui1 definitions for unimplemented major features
Removed dead lock code

 hw/arm/Makefile.objs |    2 +-
 hw/xilinx_devcfg.c   |  489 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 490 insertions(+), 1 deletions(-)
 create mode 100644 hw/xilinx_devcfg.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index f5f7d0e..29c2bd2 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += zynq_slcr.o
+obj-y += zynq_slcr.o xilinx_devcfg.o
 obj-y += xilinx_spips.o
 obj-y += arm_gic.o arm_gic_common.o
 obj-y += a9scu.o
diff --git a/hw/xilinx_devcfg.c b/hw/xilinx_devcfg.c
new file mode 100644
index 0000000..c4d7632
--- /dev/null
+++ b/hw/xilinx_devcfg.c
@@ -0,0 +1,489 @@
+/*
+ * QEMU model of the Xilinx Devcfg Interface
+ *
+ * Copyright (c) 2011 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
+#include "sysbus.h"
+#include "ptimer.h"
+#include "qemu/bitops.h"
+#include "exec/register.h"
+#include "qemu/bitops.h"
+
+#define TYPE_XILINX_DEVCFG "xlnx.ps7-dev-cfg"
+
+#define XILINX_DEVCFG(obj) \
+    OBJECT_CHECK(XilinxDevcfg, (obj), TYPE_XILINX_DEVCFG)
+
+/* FIXME: get rid of hardcoded nastiness */
+
+#define FREQ_HZ 900000000
+
+#define BTT_MAX 0x400 /* bytes to transfer per delay inteval */
+#define CYCLES_BTT_MAX 10000 /*approximate 10k cycles per delay interval */
+
+#ifndef XILINX_DEVCFG_ERR_DEBUG
+#define XILINX_DEVCFG_ERR_DEBUG 0
+#endif
+#define DB_PRINT(...) do { \
+    if (XILINX_DEVCFG_ERR_DEBUG) { \
+        fprintf(stderr,  ": %s: ", __func__); \
+        fprintf(stderr, ## __VA_ARGS__); \
+    } \
+} while (0);
+
+#define R_CTRL            (0x00/4)
+    #define FORCE_RST            (1 << 31) /* Not supported, writes ignored */
+    #define PCAP_PR              (1 << 27) /* Forced to 0 on bad unlock */
+    #define PCAP_MODE            (1 << 26)
+    #define MULTIBOOT_EN         (1 << 24)
+    #define USER_MODE            (1 << 15)
+    #define PCFG_AES_FUSE        (1 << 12) /* locked by AES_FUSE_LOCK */
+    #define PCFG_AES_EN_SHIFT    9 /* locked by AES_EN_LOCK */
+    #define PCFG_AES_EN_LEN      3 /* locked by AES_EN_LOCK */
+    #define PCFG_AES_EN_MASK     (((1 << PCFG_AES_EN_LEN) - 1) \
+                                    << PCFG_AES_EN_SHIFT)
+    #define SEU_EN               (1 << 8) /* locked by SEU_LOCK */
+    #define SEC_EN               (1 << 7) /* locked by SEC_LOCK */
+    #define SPNIDEN              (1 << 6) /* locked by DBG_LOCK */
+    #define SPIDEN               (1 << 5) /* locked by DBG_LOCK */
+    #define NIDEN                (1 << 4) /* locked by DBG_LOCK */
+    #define DBGEN                (1 << 3) /* locked by DBG_LOCK */
+    #define DAP_EN               (7 << 0) /* locked by DBG_LOCK */
+
+#define R_LOCK          (0x04/4)
+    #define AES_FUSE_LOCK        4
+    #define AES_EN_LOCK          3
+    #define SEU_LOCK             2
+    #define SEC_LOCK             1
+    #define DBG_LOCK             0
+
+/* mapping bits in R_LOCK to what they lock in R_CTRL */
+static const uint32_t lock_ctrl_map[] = {
+    [AES_FUSE_LOCK] = PCFG_AES_FUSE,
+    [AES_EN_LOCK] = PCFG_AES_EN_MASK,
+    [SEU_LOCK] = SEU_LOCK,
+    [SEC_LOCK] = SEC_LOCK,
+    [DBG_LOCK] =  SPNIDEN | SPIDEN | NIDEN | DBGEN | DAP_EN,
+};
+
+#define R_CFG           (0x08/4)
+    #define RFIFO_TH_SHIFT       10
+    #define RFIFO_TH_LEN         2
+    #define WFIFO_TH_SHIFT       8
+    #define WFIFO_TH_LEN         2
+    #define DISABLE_SRC_INC      (1 << 5)
+    #define DISABLE_DST_INC      (1 << 4)
+#define R_CFG_RO 0xFFFFF800
+#define R_CFG_RESET 0x50B
+
+#define R_INT_STS       (0x0C/4)
+    #define PSS_GTS_USR_B_INT    (1 << 31)
+    #define PSS_FST_CFG_B_INT    (1 << 30)
+    #define PSS_CFG_RESET_B_INT  (1 << 27)
+    #define RX_FIFO_OV_INT       (1 << 18)
+    #define WR_FIFO_LVL_INT      (1 << 17)
+    #define RD_FIFO_LVL_INT      (1 << 16)
+    #define DMA_CMD_ERR_INT      (1 << 15)
+    #define DMA_Q_OV_INT         (1 << 14)
+    #define DMA_DONE_INT         (1 << 13)
+    #define DMA_P_DONE_INT       (1 << 12)
+    #define P2D_LEN_ERR_INT      (1 << 11)
+    #define PCFG_DONE_INT        (1 << 2)
+    #define R_INT_STS_RSVD       ((0x7 << 24) | (0x1 << 19) | (0xF < 7))
+
+#define R_INT_MASK      (0x10/4)
+
+#define R_STATUS        (0x14/4)
+    #define DMA_CMD_Q_F         (1 << 31)
+    #define DMA_CMD_Q_E         (1 << 30)
+    #define DMA_DONE_CNT_SHIFT  28
+    #define DMA_DONE_CNT_LEN    2
+    #define RX_FIFO_LVL_SHIFT   20
+    #define RX_FIFO_LVL_LEN     5
+    #define TX_FIFO_LVL_SHIFT   12
+    #define TX_FIFO_LVL_LEN     7
+    #define TX_FIFO_LVL         (0x7f << 12)
+    #define PSS_GTS_USR_B       (1 << 11)
+    #define PSS_FST_CFG_B       (1 << 10)
+    #define PSS_CFG_RESET_B     (1 << 5)
+
+#define R_DMA_SRC_ADDR  (0x18/4)
+#define R_DMA_DST_ADDR  (0x1C/4)
+#define R_DMA_SRC_LEN   (0x20/4)
+#define R_DMA_DST_LEN   (0x24/4)
+#define R_ROM_SHADOW    (0x28/4)
+#define R_SW_ID         (0x30/4)
+#define R_UNLOCK        (0x34/4)
+
+#define R_UNLOCK_MAGIC 0x757BDF0D
+
+#define R_MCTRL         (0x80/4)
+    #define PS_VERSION_SHIFT    28
+    #define PS_VERSION_MASK     (0xf << PS_VERSION_SHIFT)
+    #define PCFG_POR_B          (1 << 8)
+    #define INT_PCAP_LPBK       (1 << 4)
+
+#define R_MAX (0x118/4+1)
+
+#define RX_FIFO_LEN 32
+#define TX_FIFO_LEN 128
+
+#define DMA_COMMAND_FIFO_LEN 10
+
+typedef struct XilinxDevcfgDMACommand {
+    uint32_t src_addr;
+    uint32_t dest_addr;
+    uint32_t src_len;
+    uint32_t dest_len;
+} XilinxDevcfgDMACommand;
+
+static const VMStateDescription vmstate_xilinx_devcfg_dma_command = {
+    .name = "xilinx_devcfg_dma_command",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(src_addr, XilinxDevcfgDMACommand),
+        VMSTATE_UINT32(dest_addr, XilinxDevcfgDMACommand),
+        VMSTATE_UINT32(src_len, XilinxDevcfgDMACommand),
+        VMSTATE_UINT32(dest_len, XilinxDevcfgDMACommand),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+typedef struct XilinxDevcfg {
+    SysBusDevice busdev;
+    MemoryRegion iomem;
+
+    qemu_irq irq;
+
+    QEMUBH *timer_bh;
+    ptimer_state *timer;
+
+    XilinxDevcfgDMACommand dma_command_fifo[DMA_COMMAND_FIFO_LEN];
+    uint8_t dma_command_fifo_num;
+
+    uint32_t regs[R_MAX];
+    RegisterInfo regs_info[R_MAX];
+} XilinxDevcfg;
+
+static const VMStateDescription vmstate_xilinx_devcfg = {
+    .name = "xilinx_devcfg",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PTIMER(timer, XilinxDevcfg),
+        VMSTATE_STRUCT_ARRAY(dma_command_fifo, XilinxDevcfg,
+                             DMA_COMMAND_FIFO_LEN, 0,
+                             vmstate_xilinx_devcfg_dma_command,
+                             XilinxDevcfgDMACommand),
+        VMSTATE_UINT8(dma_command_fifo_num, XilinxDevcfg),
+        VMSTATE_UINT32_ARRAY(regs, XilinxDevcfg, R_MAX),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void xilinx_devcfg_update_ixr(XilinxDevcfg *s)
+{
+    qemu_set_irq(s->irq, !!(~s->regs[R_INT_MASK] & s->regs[R_INT_STS]));
+}
+
+static void xilinx_devcfg_reset(DeviceState *dev)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(dev);
+    int i;
+
+    for (i = 0; i < R_MAX; ++i) {
+        register_reset(&s->regs_info[i]);
+    }
+}
+
+#define min(a, b) ((a) < (b) ? (a) : (b))
+
+static void xilinx_devcfg_dma_go(void *opaque)
+{
+    XilinxDevcfg *s = opaque;
+    uint8_t buf[BTT_MAX];
+    XilinxDevcfgDMACommand *dmah = s->dma_command_fifo;
+    uint32_t btt = BTT_MAX;
+
+    btt = min(btt, dmah->src_len);
+    if (s->regs[R_MCTRL] & INT_PCAP_LPBK) {
+        btt = min(btt, dmah->dest_len);
+    }
+    DB_PRINT("reading %x bytes from %x\n", btt, dmah->src_addr);
+    dma_memory_read(&dma_context_memory, dmah->src_addr, buf, btt);
+    dmah->src_len -= btt;
+    dmah->src_addr += btt;
+    if (s->regs[R_MCTRL] & INT_PCAP_LPBK) {
+        DB_PRINT("writing %x bytes from %x\n", btt, dmah->dest_addr);
+        dma_memory_write(&dma_context_memory, dmah->dest_addr, buf, btt);
+        dmah->dest_len -= btt;
+        dmah->dest_addr += btt;
+    }
+    if (!dmah->src_len && !dmah->dest_len) {
+        DB_PRINT("dma operation finished\n");
+        s->regs[R_INT_STS] |= DMA_DONE_INT | DMA_P_DONE_INT;
+        s->dma_command_fifo_num = s->dma_command_fifo_num - 1;
+        memcpy(s->dma_command_fifo, &s->dma_command_fifo[1],
+               sizeof(*s->dma_command_fifo) * DMA_COMMAND_FIFO_LEN - 1);
+    }
+    xilinx_devcfg_update_ixr(s);
+    if (s->dma_command_fifo_num) { /* there is still work to do */
+        DB_PRINT("dma work remains, setting up callback ptimer\n");
+        ptimer_set_freq(s->timer, FREQ_HZ);
+        ptimer_set_count(s->timer, CYCLES_BTT_MAX);
+        ptimer_run(s->timer, 1);
+    }
+}
+
+static void r_ixr_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
+
+    xilinx_devcfg_update_ixr(s);
+}
+
+static uint64_t r_ctrl_prewrite(RegisterInfo *reg, uint64_t val)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+        if (s->regs[R_LOCK] & 1 << i) {
+            val &= ~lock_ctrl_map[i];
+            val |= lock_ctrl_map[i] & s->regs[R_CTRL];
+        }
+    }
+    return val;
+}
+
+static void r_ctrl_post_write(RegisterInfo *reg, uint64_t val)
+{
+    uint32_t aes_en = extract32(val, PCFG_AES_EN_SHIFT, PCFG_AES_EN_LEN);
+
+    if (aes_en != 0 && aes_en != 7) {
+        qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits inconsistent,"
+                      "unimplemeneted security reset should happen!\n",
+                      reg->prefix);
+    }
+}
+
+static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
+
+    if (val == R_UNLOCK_MAGIC) {
+        DB_PRINT("successful unlock\n");
+    } else {/* bad unlock attempt */
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", reg->prefix);
+        qemu_log_mask(LOG_UNIMP, "%s: failed unlock, unimplmeneted crash of"
+                      "device should happen\n", reg->prefix);
+        s->regs[R_CTRL] &= ~PCAP_PR;
+        s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
+    }
+}
+
+static void r_dma_dst_len_post_write(RegisterInfo *reg, uint64_t val)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(reg->opaque);
+
+    /* TODO: implement command queue overflow check and interrupt */
+    s->dma_command_fifo[s->dma_command_fifo_num].src_addr =
+            s->regs[R_DMA_SRC_ADDR] & ~0x3UL;
+    s->dma_command_fifo[s->dma_command_fifo_num].dest_addr =
+            s->regs[R_DMA_DST_ADDR] & ~0x3UL;
+    s->dma_command_fifo[s->dma_command_fifo_num].src_len =
+            s->regs[R_DMA_SRC_LEN] << 2;
+    s->dma_command_fifo[s->dma_command_fifo_num].dest_len =
+            s->regs[R_DMA_DST_LEN] << 2;
+    s->dma_command_fifo_num++;
+    DB_PRINT("dma transfer started; %d total transfers pending\n",
+             s->dma_command_fifo_num);
+    xilinx_devcfg_dma_go(s);
+}
+
+static const RegisterAccessInfo xilinx_devcfg_regs_info[] = {
+    [R_CTRL] = { .name = "CTRL",
+        .reset = PCAP_PR | PCAP_MODE | 0x3 << 13,
+        /* flag this wo bit as ro to handle it separately */
+        .ro = 0x107f6000,
+        .wo = USER_MODE | 1 << 15,
+        .ge1 = (RegisterAccessError[]) {
+            { .mask = 0x1 << 15, .reason = "Reserved - do not modify" },
+            {},
+        },
+        .ge0 = (RegisterAccessError[]) {
+            { .mask = 0x3 << 13, .reason = "Reserved - always write with 1" },
+            {},
+        },
+        .ui1 = (RegisterAccessError[]) {
+            { .mask = FORCE_RST, .reason = "PS reset not implemented" },
+            { .mask = PCAP_MODE, .reason = "FPGA Fabric doesnt exist" },
+            { .mask = PCFG_AES_EN_MASK, .reason = "AES not implmented" },
+            {},
+        },
+        .pre_write = r_ctrl_prewrite,
+        .post_write = r_ctrl_post_write,
+    },
+    [R_LOCK] = { .name = "LOCK", .ro = ~ONES(5), .nw0 = ~0 },
+    [R_CFG] = { .name = "CFG",
+        .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
+        .ge1 = (RegisterAccessError[]) {
+            { .mask = 0x7, .reason = "Reserved - do not modify" },
+            {},
+        },
+        .ge0 = (RegisterAccessError[]) {
+            { .mask = 0x8, .reason = "Reserved - do not modify" },
+            {},
+        },
+        .ro = 0x00f | ~ONES(12),
+    },
+    [R_INT_STS] = { .name = "INT_STS",
+        .w1c = ~R_INT_STS_RSVD,
+        .reset = PSS_GTS_USR_B_INT | PSS_CFG_RESET_B_INT | WR_FIFO_LVL_INT,
+        .ro = R_INT_STS_RSVD,
+        .post_write = r_ixr_post_write,
+    },
+    [R_INT_MASK] = { .name = "INT_MASK",
+        .reset = ~0,
+        .ro = R_INT_STS_RSVD,
+        .post_write = r_ixr_post_write,
+    },
+    [R_STATUS] = { .name = "STATUS",
+        .reset = DMA_CMD_Q_E | PSS_GTS_USR_B | PSS_CFG_RESET_B,
+        .ro = ~0,
+        .ge1 = (RegisterAccessError[])  {
+            {.mask = 0x1, .reason = "Reserved - do not modify" },
+            {},
+        },
+    },
+    [R_DMA_SRC_ADDR] = { .name = "DMA_SRC_ADDR" },
+    [R_DMA_DST_ADDR] = { .name = "DMA_DST_ADDR" },
+    [R_DMA_SRC_LEN] = { .name = "DMA_SRC_LEN", .ro = ~ONES(27) },
+    [R_DMA_DST_LEN] = { .name = "DMA_DST_LEN",
+        .ro = ~ONES(27),
+        .post_write = r_dma_dst_len_post_write,
+    },
+    [R_ROM_SHADOW] = { .name = "ROM_SHADOW",
+        .ge1 = (RegisterAccessError[])  {
+            {.mask = ~0, .reason = "Reserved - do not modify" },
+            {},
+        },
+    },
+    [R_SW_ID] = { .name = "SW_ID" },
+    [R_UNLOCK] = { .name = "UNLOCK",
+        .post_write = r_unlock_post_write,
+    },
+    [R_MCTRL] = { .name = "MCTRL",
+        /* Silicon 3.0 for version field, and the mysterious reserved bit 23 */
+        .reset = 0x2 << PS_VERSION_SHIFT | 1 << 23,
+        /* some reserved bits are rw while others are ro */
+        .ro = ~INT_PCAP_LPBK,
+        .ge1 = (RegisterAccessError[]) {
+            { .mask = 0x00F00300, .reason = "Reserved - do not modify" },
+            { .mask = 0x00000003, .reason = "Reserved - always write with 0" },
+            {}
+        },
+        .ge0 = (RegisterAccessError[]) {
+            { .mask = 1 << 23, .reason = "Reserved - always write with 1" },
+            {}
+        },
+    },
+    [R_MAX] = {}
+};
+
+static const MemoryRegionOps devcfg_reg_ops = {
+    .read = register_read_memory_le,
+    .write = register_write_memory_le,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    }
+};
+
+static void xilinx_devcfg_realize(DeviceState *dev, Error **errp)
+{
+    XilinxDevcfg *s = XILINX_DEVCFG(dev);
+    const char *prefix = object_get_canonical_path(OBJECT(dev));
+    int i;
+
+    for (i = 0; i < R_MAX; ++i) {
+        RegisterInfo *r = &s->regs_info[i];
+
+        *r = (RegisterInfo) {
+            .data = (uint8_t *)&s->regs[i],
+            .data_size = sizeof(uint32_t),
+#if defined(HOST_WORDS_BIGENDIAN)
+            .data_big_endian = true,
+#endif
+            .access = &xilinx_devcfg_regs_info[i],
+            .debug = XILINX_DEVCFG_ERR_DEBUG,
+            .prefix = prefix,
+            .opaque = s,
+        };
+        memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs", 4);
+        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
+    }
+}
+
+static void xilinx_devcfg_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    XilinxDevcfg *s = XILINX_DEVCFG(obj);
+
+    s->timer_bh = qemu_bh_new(xilinx_devcfg_dma_go, s);
+    s->timer = ptimer_init(s->timer_bh);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init(&s->iomem, "devcfg", R_MAX*4);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void xilinx_devcfg_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = xilinx_devcfg_reset;
+    dc->vmsd = &vmstate_xilinx_devcfg;
+    dc->realize = xilinx_devcfg_realize;
+}
+
+static const TypeInfo xilinx_devcfg_info = {
+    .name           = TYPE_XILINX_DEVCFG,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(XilinxDevcfg),
+    .instance_init  = xilinx_devcfg_init,
+    .class_init     = xilinx_devcfg_class_init,
+};
+
+static void xilinx_devcfg_register_types(void)
+{
+    type_register_static(&xilinx_devcfg_info);
+}
+
+type_init(xilinx_devcfg_register_types)
-- 
1.7.0.4

[Qemu-devel] [PATCH v2 5/5] xilinx_zynq: added devcfg to machine model

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
Changed since v1:
Added manual parenting of devcfg node (evil but needed for early access
to canonical path by devcfgs realize fn).

 hw/arm/xilinx_zynq.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 6f36286..fb3a089 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -220,6 +220,14 @@ static void zynq_init(QEMUMachineInitArgs *args)
         sysbus_connect_irq(busdev, n + 1, pic[dma_irqs[n] - IRQ_OFFSET]);
     }
 
+    dev = qdev_create(NULL, "xlnx.ps7-dev-cfg");
+    object_property_add_child(qdev_get_machine(), "xilinx-devcfg", OBJECT(dev),
+                              NULL);
+    qdev_init_nofail(dev);
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_connect_irq(busdev, 0, pic[40-IRQ_OFFSET]);
+    sysbus_mmio_map(busdev, 0, 0xF8007000);
+
     zynq_binfo.ram_size = ram_size;
     zynq_binfo.kernel_filename = kernel_filename;
     zynq_binfo.kernel_cmdline = kernel_cmdline;
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 1/5] bitops: Add ONES macro

[Peter Maydell]

On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Little macro that just gives you N ones (justified to LSB).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  include/qemu/bitops.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index affcc96..da47fc8 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -273,4 +273,6 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>      return (value & ~mask) | ((fieldval << start) & mask);
>  }
>
> +#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1)

You can avoid the ?: here (assuming you're happy to say that
ONES(0) is a silly thing to ask for):

#define ONES(num) (~0ULL >> (64 - (num)))

Needs a documentation comment, anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] bitops: Add ONES macro

[Peter Crosthwaite]

Hi Peter,

On Fri, Apr 5, 2013 at 6:53 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Little macro that just gives you N ones (justified to LSB).
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  include/qemu/bitops.h |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index affcc96..da47fc8 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>> @@ -273,4 +273,6 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>>      return (value & ~mask) | ((fieldval << start) & mask);
>>  }
>>
>> +#define ONES(num) ((num) == 64 ? ~0ull : (1ull << (num)) - 1)
>
> You can avoid the ?: here (assuming you're happy to say that
> ONES(0) is a silly thing to ask for):
>

Not that silly. ONES(0) should just stay true to the contract and
return 0. Otherwise prospective clients may have to guard against 0
cases in loops which is ugly:

for (i = 0; i < N; i++) {
    ....
    mask = ONES(i); /* rather than ones = i ? ones(i) : 0 */
    ....
}

But  I thought about this more, and perhaps just ditch the ? anyway,
and rely on the shift by 64 ending up as 0. Then the -1 should return
return 64 (all) ones anyway.

> #define ONES(num) (~0ULL >> (64 - (num)))
>
> Needs a documentation comment, anyway.
>

OK

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2 1/5] bitops: Add ONES macro

[Peter Maydell]

On 5 April 2013 10:11, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> But  I thought about this more, and perhaps just ditch the ? anyway,
> and rely on the shift by 64 ending up as 0.

That's undefined behaviour, please don't.

-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API

[Peter Maydell]

On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> This API provides some encapsulation of registers and factors our some
> common functionality to common code. Bits of device state (usually MMIO
> registers), often have all sorts of access restrictions and semantics
> associated with them. This API allow you to define what those
> restrictions are on a bit-by-bit basis.
> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
> +{
> +    uint64_t old_val, new_val, test;
> +    const RegisterAccessInfo *ac;
> +    const RegisterAccessError *rae;
> +
> +    assert(reg);
> +
> +    ac = reg->access;
> +    if (!ac || !ac->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
> +        return;
> +    }
> +
> +    uint32_t no_w0_mask = ac->ro | ac->w1c | ac->nw0 | ~we;
> +    uint32_t no_w1_mask = ac->ro | ac->w1c | ac->nw1 | ~we;
> +
> +    if (reg->debug) {
> +        fprintf(stderr, "%s:%s: write of value %#" PRIx64 "\n", reg->prefix,
> +                ac->name, val);
> +    }
> +
> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
> +        for (rae = ac->ge1; rae && rae->mask; rae++) {
> +            test = val & rae->mask;
> +            if (test) {
> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
> +                                   "invalid", rae->reason);
> +            }
> +        }
> +        for (rae = ac->ge0; rae && rae->mask; rae++) {
> +            test = val & rae->mask;
> +            if (test) {
> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
> +                                   "invalid", rae->reason);
> +            }
> +        }
> +    }
> +
> +    if (qemu_loglevel_mask(LOG_UNIMP)) {
> +        for (rae = ac->ui1; rae && rae->mask; rae++) {
> +            test = val & rae->mask;
> +            if (test) {
> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
> +                                   "unimplmented", rae->reason);
> +            }
> +        }
> +        for (rae = ac->ui0; rae && rae->mask; rae++) {
> +            test = val & rae->mask;
> +            if (test) {
> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
> +                                   "unimplemented", rae->reason);
> +            }
> +        }
> +    }
> +
> +    assert(reg->data);
> +    old_val = register_read_val(reg);
> +
> +    new_val = val & ~(no_w1_mask & val);
> +    new_val |= no_w1_mask & old_val & val;
> +    new_val |= no_w0_mask & old_val & ~val;
> +    new_val &= ~(val & ac->w1c);
> +
> +    if (ac->pre_write) {
> +        new_val = ac->pre_write(reg, new_val);
> +    }
> +    register_write_val(reg, new_val);
> +    if (ac->post_write) {
> +        ac->post_write(reg, new_val);
> +    }
> +}

Wow, this feels pretty heavyweight for "write a register"...

-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API

[Peter Crosthwaite]

On Fri, Apr 5, 2013 at 7:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> This API provides some encapsulation of registers and factors our some
>> common functionality to common code. Bits of device state (usually MMIO
>> registers), often have all sorts of access restrictions and semantics
>> associated with them. This API allow you to define what those
>> restrictions are on a bit-by-bit basis.
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>> +{
>> +    uint64_t old_val, new_val, test;
>> +    const RegisterAccessInfo *ac;
>> +    const RegisterAccessError *rae;
>> +
>> +    assert(reg);
>> +
>> +    ac = reg->access;
>> +    if (!ac || !ac->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
>> +        return;
>> +    }
>> +
>> +    uint32_t no_w0_mask = ac->ro | ac->w1c | ac->nw0 | ~we;
>> +    uint32_t no_w1_mask = ac->ro | ac->w1c | ac->nw1 | ~we;
>> +
>> +    if (reg->debug) {
>> +        fprintf(stderr, "%s:%s: write of value %#" PRIx64 "\n", reg->prefix,
>> +                ac->name, val);
>> +    }
>> +
>> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
>> +        for (rae = ac->ge1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>> +        for (rae = ac->ge0; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>> +    }
>> +
>> +    if (qemu_loglevel_mask(LOG_UNIMP)) {
>> +        for (rae = ac->ui1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "unimplmented", rae->reason);
>> +            }
>> +        }
>> +        for (rae = ac->ui0; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "unimplemented", rae->reason);
>> +            }
>> +        }
>> +    }
>> +
>> +    assert(reg->data);
>> +    old_val = register_read_val(reg);
>> +
>> +    new_val = val & ~(no_w1_mask & val);
>> +    new_val |= no_w1_mask & old_val & val;
>> +    new_val |= no_w0_mask & old_val & ~val;
>> +    new_val &= ~(val & ac->w1c);
>> +
>> +    if (ac->pre_write) {
>> +        new_val = ac->pre_write(reg, new_val);
>> +    }
>> +    register_write_val(reg, new_val);
>> +    if (ac->post_write) {
>> +        ac->post_write(reg, new_val);
>> +    }
>> +}
>
> Wow, this feels pretty heavyweight for "write a register"...
>

Its a pritty fast path through if all the optionals turned off. If I
was going to lighten it up, Id get rid of the reg_size and the byte
loop first although that would mandate uint64_t as the data type for
the backing store.

But the intended primary usage of the API is for device control and
status registers, where you are generally not interested in
performance, unless you are doing some form of PIO. In that case, you
can opt out on a per register basis and make yourself a fast path for
the critical registers (r/w fifo heads access regs etc). This is
designed for developer and debugging convenience, where you have a
large number or registers with a heteorgenous mix of accesses and side
effects but at the same time the registers are infrequent access. This
is true of most device registers.

Another solution is some sort of "lite" mode. A single check in the
definition that disables a lot of this and cuts to the chase (the
actual write).

> -- PMM
>

Re: [Qemu-devel] [PATCH v2 3/5] register: Add Memory API glue

[Paolo Bonzini]

Il 05/04/2013 10:43, Peter Crosthwaite ha scritto:
> Add memory io handlers that glue the register API to the memory API.
> Just translation functions at this stage. Although it does allow for
> devices to be created without all-in-one mmio r/w handlers.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  include/exec/register.h |   13 +++++++++++++

Please put it in include/hw/.

Putting these five in exec/ was a mistake:

#include "exec/address-spaces.h"
#include "exec/hwaddr.h"
#include "exec/ioport.h"
#include "exec/iorange.h"
#include "exec/memory.h"

Most files in hw/ should not need include/exec/.

>  register.c              |   43 +++++++++++++++++++++++++++++++++++++++++++

If my reorganization goes in before, please make it hw/core/register.c.

The next step in the reorganization would be to move memory.c to hw/core
too.

Paolo

Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API

[Peter Crosthwaite]

Hi Peter,

On Fri, Apr 5, 2013 at 7:49 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Fri, Apr 5, 2013 at 7:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>> This API provides some encapsulation of registers and factors our some
>>> common functionality to common code. Bits of device state (usually MMIO
>>> registers), often have all sorts of access restrictions and semantics
>>> associated with them. This API allow you to define what those
>>> restrictions are on a bit-by-bit basis.
>>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>>> +{
>>> +    uint64_t old_val, new_val, test;
>>> +    const RegisterAccessInfo *ac;
>>> +    const RegisterAccessError *rae;
>>> +
>>> +    assert(reg);
>>> +
>>> +    ac = reg->access;
>>> +    if (!ac || !ac->name) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>>> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
>>> +        return;
>>> +    }
>>> +
>>> +    uint32_t no_w0_mask = ac->ro | ac->w1c | ac->nw0 | ~we;
>>> +    uint32_t no_w1_mask = ac->ro | ac->w1c | ac->nw1 | ~we;
>>> +
>>> +    if (reg->debug) {
>>> +        fprintf(stderr, "%s:%s: write of value %#" PRIx64 "\n", reg->prefix,
>>> +                ac->name, val);
>>> +    }
>>> +
>>> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
>>> +        for (rae = ac->ge1; rae && rae->mask; rae++) {
>>> +            test = val & rae->mask;
>>> +            if (test) {
>>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>>> +                                   "invalid", rae->reason);
>>> +            }
>>> +        }
>>> +        for (rae = ac->ge0; rae && rae->mask; rae++) {
>>> +            test = val & rae->mask;
>>> +            if (test) {
>>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>>> +                                   "invalid", rae->reason);
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if (qemu_loglevel_mask(LOG_UNIMP)) {
>>> +        for (rae = ac->ui1; rae && rae->mask; rae++) {
>>> +            test = val & rae->mask;
>>> +            if (test) {
>>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>>> +                                   "unimplmented", rae->reason);
>>> +            }
>>> +        }
>>> +        for (rae = ac->ui0; rae && rae->mask; rae++) {
>>> +            test = val & rae->mask;
>>> +            if (test) {
>>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>>> +                                   "unimplemented", rae->reason);
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    assert(reg->data);
>>> +    old_val = register_read_val(reg);
>>> +
>>> +    new_val = val & ~(no_w1_mask & val);
>>> +    new_val |= no_w1_mask & old_val & val;
>>> +    new_val |= no_w0_mask & old_val & ~val;
>>> +    new_val &= ~(val & ac->w1c);
>>> +
>>> +    if (ac->pre_write) {
>>> +        new_val = ac->pre_write(reg, new_val);
>>> +    }
>>> +    register_write_val(reg, new_val);
>>> +    if (ac->post_write) {
>>> +        ac->post_write(reg, new_val);
>>> +    }
>>> +}
>>
>> Wow, this feels pretty heavyweight for "write a register"...
>>

I have been thinking about this more, and in the interest of getting
something merged, I'm happy to work on this. I could use some specific
feedback however, considering there was generally positive feedback on
v1 so im guessing the objections come from the v2 added features
(being the byte loop, ui, we and the hooks). Here are my current
"lightening" proposals:

- Add a "ne" (no/native endianess) version to the memory api handlers
which just passes the args straight through to register API, no byte
reversal logic needed.
- Remove nw0 and nw1, They are reasonably rare and can be handled with
the odd pre_write hook for those cases.
- Add a "lite" flag that is (automatically) computed once that takes a
fast path through the read/write handlers. A reg is lite if it
requires no read-modify-write (no ro, nw, ge, ui, wtc). Only the
actual write and post_write handlers are executed.
- Remove the byte loops. Just switch on register size and cast to
uint_XXt. Remove support for non power-of-two size registers and
non-host endianess (needed for potential PCI conversion) but should
make the code faster.
- Make data pointer optional. No data means register is read as reset
and no write occurs (post write hook only). This make wo redundant
(anyone wanting to do per-bit wo semantic uses pre-write hook).

If you care about performance of a particular reg, just add a
post_write hook, set it up as native endianess, with a NULL data
register, no access checks. Then its a very fast path from the memory
API through to your handler.

Regards,
Peter

>
> Its a pritty fast path through if all the optionals turned off. If I
> was going to lighten it up, Id get rid of the reg_size and the byte
> loop first although that would mandate uint64_t as the data type for
> the backing store.
>
> But the intended primary usage of the API is for device control and
> status registers, where you are generally not interested in
> performance, unless you are doing some form of PIO. In that case, you
> can opt out on a per register basis and make yourself a fast path for
> the critical registers (r/w fifo heads access regs etc). This is
> designed for developer and debugging convenience, where you have a
> large number or registers with a heteorgenous mix of accesses and side
> effects but at the same time the registers are infrequent access. This
> is true of most device registers.
>
> Another solution is some sort of "lite" mode. A single check in the
> definition that disables a lot of this and cuts to the chase (the
> actual write).
>
>> -- PMM
>>

Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API

[Gerd Hoffmann]

  Hi,

> - Add a "lite" flag that is (automatically) computed once that takes a
> fast path through the read/write handlers. A reg is lite if it
> requires no read-modify-write (no ro, nw, ge, ui, wtc). Only the
> actual write and post_write handlers are executed.

Sounds good to me.

> - Remove the byte loops. Just switch on register size and cast to
> uint_XXt. Remove support for non power-of-two size registers and
> non-host endianess (needed for potential PCI conversion) but should
> make the code faster.

This too.

Also: you might want to pass both old and new value to the post-write
hook so the handler can easily figure what has actually changed.

cheers,
  Gerd