[Qemu-devel] [RFC PATCH v1 0/4] Data Driven device registers & Zynq DEVCFG

[Qemu-devel] [RFC PATCH v1 0/4] 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 an extension of qemu_log to query what features are and are not enabled.
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.


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

Peter Crosthwaite (2):
  qemu-log: Allow checking of the current mask
  bitops: Add UInt32StateInfo and helper functions

 hw/arm/Makefile.objs  |    2 +-
 hw/xilinx_devcfg.c    |  443 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/xilinx_zynq.c      |    6 +
 include/qemu/bitops.h |   59 +++++++
 include/qemu/log.h    |    2 +
 qemu-log.c            |    5 +
 util/bitops.c         |   71 ++++++++
 7 files changed, 587 insertions(+), 1 deletions(-)
 create mode 100644 hw/xilinx_devcfg.c

[Qemu-devel] [RFC PATCH v1 1/4] qemu-log: Allow checking of the current mask

[Peter Crosthwaite]

Useful for heavy users of qemu_log_mask that want to avoid executing expensive
logic that sets up a qemu_log_mask when that mask is disabled. E.G.

if (qemu_log_get_mask() && LOG_GUEST_ERROR) {
	/* do my expensive logging data query */
}
qemu_log_mask(LOG_GUEST_ERROR, ...)

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

 include/qemu/log.h |    2 ++
 qemu-log.c         |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 6b0db02..68188f0 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -154,6 +154,8 @@ static inline void qemu_set_log(int log_flags)
 #endif
 }
 
+int qemu_get_log_flags(void);
+
 void qemu_set_log_filename(const char *filename);
 int qemu_str_to_log_mask(const char *str);
 
diff --git a/qemu-log.c b/qemu-log.c
index 797f2af..2ffee1d 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -82,6 +82,11 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
     }
 }
 
+int qemu_get_log_flags(void)
+{
+    return qemu_loglevel;
+}
+
 void qemu_set_log_filename(const char *filename)
 {
     g_free(logfilename);
-- 
1.7.0.4

[Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Crosthwaite]

This struct and functions provide some encapsulation of the uint32_t type to
make it more friendly for use as guest accessible device state. Bits of device
state (usually MMIO registers), often have all sorts of access restrictions
and semantics associated with them. This struct allow you to define what whose
restrictions are on a bit-by-bit basis.

Helper functions are then used to access the uint32_t which observe the
semantics defined by the UInt32StateInfo 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 sticky (nw0 and nw1)
Reset values can be defined (reset)
Bits can be marked to throw guest errors when written certain values (ge0, ge1)
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)

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>
---

 include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
 util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 0 deletions(-)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index affcc96..8ad0290 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
     return (value & ~mask) | ((fieldval << start) & mask);
 }
 
+/**
+ * A descriptor for a Uint32 that is part of guest accessible device state
+ * @ro: whether or not the bit is read-only state comming out of reset
+ * @w1c: bits with the common write 1 to clear semantic.
+ * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
+ * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
+ * @reset: reset value.
+ * @ge1: Bits that when written 1 indicate a guest error
+ * @ge0: Bits that when written 0 indicate a guest error
+ * @cor: Bits that are clear on read
+ * @width: width of the uint32t. Only the @width least significant bits are
+ * valid. All others are silent Read-as-reset/WI.
+ */
+
+typedef struct UInt32StateInfo {
+    const char *name;
+    uint32_t ro;
+    uint32_t w1c;
+    uint32_t nw0;
+    uint32_t nw1;
+    uint32_t reset;
+    uint32_t ge1;
+    uint32_t ge0;
+    uint32_t cor;
+    int width;
+} UInt32StateInfo;
+
+/**
+ * reset an array of u32s
+ * @array: array of u32s to reset
+ * @info: corresponding array of UInt32StateInfos to get reset values from
+ * @num: number of values to reset
+ */
+
+void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
+
+/**
+ * write a value to a uint32_t subject to its restrictions
+ * @state: Pointer to location to be written
+ * @info: Definition of variable
+ * @val: value to write
+ * @prefix: Debug and error message prefix
+ * @debug: Turn on noisy debug printfery
+ */
+
+void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
+                  const char *prefix, bool debug);
+
+/**
+ * write a value from a uint32_t subject to its restrictions
+ * @state: Pointer to location to be read
+ * @info: Definition of variable
+ * @prefix: Debug and error message prefix
+ * @debug: Turn on noisy debug printfery
+ */
+
+uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
+                     const char *prefix, bool debug);
+
 #endif
diff --git a/util/bitops.c b/util/bitops.c
index e72237a..51db476 100644
--- a/util/bitops.c
+++ b/util/bitops.c
@@ -11,6 +11,8 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include "qemu-common.h"
+#include "qemu/log.h"
 #include "qemu/bitops.h"
 
 #define BITOP_WORD(nr)		((nr) / BITS_PER_LONG)
@@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
     /* Not found */
     return size;
 }
+void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
+{
+    int i = 0;
+
+    for (i = 0; i < num; ++i) {
+        state[i] = info[i].reset;
+    }
+}
+
+void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
+                  const char *prefix, bool debug)
+{
+    int i;
+    uint32_t new_val;
+    int width = info->width ? info->width : 32;
+
+    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
+                        ~((1ull << width) - 1);
+    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
+                        ~((1ull << width) - 1);
+
+    if (!info->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
+                      "(written value: %#08x)\n", prefix, val);
+        return;
+    }
+
+    if (debug) {
+        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
+                val);
+    }
+
+    /*FIXME: skip over if no LOG_GUEST_ERROR */
+    for (i = 0; i <= 1; ++i) {
+        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
+        if (test) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
+                          " to %d\n", prefix, info->name, test, i);
+        }
+    }
+
+    new_val = val & ~(no_w1_mask & val);
+    new_val |= no_w1_mask & *state & val;
+    new_val |= no_w0_mask & *state & ~val;
+    new_val &= ~(val & info->w1c);
+    *state = new_val;
+}
+
+uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
+                     const char *prefix, bool debug)
+{
+    uint32_t ret = *state;
+
+    /* clear on read */
+    ret &= ~info->cor;
+    *state = ret;
+
+    if (!info->name) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
+                      "(read value: %#08x)\n", prefix, ret);
+        return ret;
+    }
+
+    if (debug) {
+        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
+    }
+
+    return ret;
+}
-- 
1.7.0.4

[Qemu-devel] [RFC PATCH v1 3/4] xilinx_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. Lock mechanisms supported.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---

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

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 4c10985..d461fd0 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,6 +1,6 @@
 obj-y = integratorcp.o versatilepb.o arm_pic.o
 obj-y += arm_boot.o
-obj-y += xilinx_zynq.o zynq_slcr.o
+obj-y += xilinx_zynq.o 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..c54baef
--- /dev/null
+++ b/hw/xilinx_devcfg.c
@@ -0,0 +1,443 @@
+/*
+ * 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"
+
+#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 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
+
+static const UInt32StateInfo 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 | USER_MODE,
+        .ge0 = 0x3 << 13,
+    },
+    [R_LOCK] = { .name = "LOCK", .width = 5, .nw0 = ~0 },
+    [R_CFG] = { .name = "CFG",
+        .width = 12,
+        .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
+        .ge1 = 0x7,
+        .ge0 = 0x8,
+        .ro = 0x00f,
+    },
+    [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,
+    },
+    [R_INT_MASK] = { .name = "INT_MASK", .reset = ~0, .ro = R_INT_STS_RSVD },
+    [R_STATUS] = { .name = "STATUS",
+        .reset = DMA_CMD_Q_E | PSS_GTS_USR_B | PSS_CFG_RESET_B,
+        .ro = ~0,
+        .ge1 = 0x1,
+    },
+    [R_DMA_SRC_ADDR] = { .name = "DMA_SRC_ADDR" },
+    [R_DMA_DST_ADDR] = { .name = "DMA_DST_ADDR" },
+    [R_DMA_SRC_LEN] = { .name = "DMA_SRC_LEN", .width = 27 },
+    [R_DMA_DST_LEN] = { .name = "DMA_DST_LEN", .width = 27 },
+    [R_ROM_SHADOW] = { .name = "ROM_SHADOW", .ge1 = ~0  },
+    [R_SW_ID] = { .name = "SW_ID" },
+    [R_UNLOCK] = { .name = "UNLOCK" },
+    [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 = 0x0F003003,
+        .ge0 = 1 << 23,
+    },
+    [R_MAX] = {}
+};
+
+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;
+    bool lock;
+
+    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];
+} 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_BOOL(lock, XilinxDevcfg),
+        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);
+
+    uint32_array_reset(s->regs, xilinx_devcfg_regs_info, R_MAX);
+}
+
+#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 xilinx_devcfg_write(void *opaque, hwaddr addr, uint64_t value,
+                                unsigned size)
+{
+    XilinxDevcfg *s = opaque;
+    int i;
+    uint32_t aes_en;
+    const char *prefix = "";
+
+    /* FIXME: use tracing to avoid these gymnastics */
+    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags() & LOG_GUEST_ERROR) {
+        prefix = g_strdup_printf("%s:Addr %#08x",
+                                 object_get_canonical_path(OBJECT(s)),
+                                 (unsigned)addr);
+    }
+    addr >>= 2;
+    assert(addr < R_MAX);
+
+    if (s->lock && addr != R_UNLOCK) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s:write to non-lock register while"
+                      " locked\n", prefix);
+        return;
+    }
+
+    uint32_write(&s->regs[addr], &xilinx_devcfg_regs_info[addr], value, prefix,
+                 XILINX_DEVCFG_ERR_DEBUG);
+
+    /*side effects */
+    switch (addr) {
+    case R_CTRL:
+        for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
+            if (s->regs[R_LOCK] & 1 << i) {
+                value &= ~lock_ctrl_map[i];
+                value |= lock_ctrl_map[i] & s->regs[R_CTRL];
+            }
+        }
+        aes_en = extract32(value, 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",
+                          prefix);
+        }
+        break;
+
+    case R_DMA_DST_LEN:
+        /* 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);
+        break;
+
+    case R_UNLOCK:
+        if (value == R_UNLOCK_MAGIC) {
+            s->lock = 0;
+            DB_PRINT("successful unlock\n");
+        } else if (s->lock) {/* bad unlock attempt */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s:failed unlock\n", prefix);
+            s->regs[R_CTRL] &= ~PCAP_PR;
+            s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
+        }
+        break;
+    }
+    xilinx_devcfg_update_ixr(s);
+
+    if (*prefix) {
+        g_free((void *)prefix);
+    }
+}
+
+static uint64_t xilinx_devcfg_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XilinxDevcfg *s = opaque;
+    uint32_t ret  = s->regs[addr];
+    const char *prefix = "";
+
+    /* FIXME: use tracing to avoid these gymnastics */
+    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags()) {
+        prefix = g_strdup_printf("%s:Addr %#08x",
+                                 object_get_canonical_path(OBJECT(s)),
+                                 (unsigned)addr);
+    }
+
+    addr >>= 2;
+    assert(addr < R_MAX);
+    ret = uint32_read(&s->regs[addr], &xilinx_devcfg_regs_info[addr], prefix,
+                      XILINX_DEVCFG_ERR_DEBUG);
+    if (*prefix) {
+        g_free((void *)prefix);
+    }
+    return ret;
+}
+
+static const MemoryRegionOps devcfg_ops = {
+    .read = xilinx_devcfg_read,
+    .write = xilinx_devcfg_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+
+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_io(&s->iomem, &devcfg_ops, s, "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;
+}
+
+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] [RFC PATCH v1 4/4] 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>
---

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

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index 2f67d90..16d8dc8 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -196,6 +196,12 @@ static void zynq_init(QEMUMachineInitArgs *args)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]);
 
+    dev = qdev_create(NULL, "xlnx.ps7-dev-cfg");
+    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] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Blue Swirl]

On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This struct and functions provide some encapsulation of the uint32_t type to
> make it more friendly for use as guest accessible device state. Bits of device
> state (usually MMIO registers), often have all sorts of access restrictions
> and semantics associated with them. This struct allow you to define what whose
> restrictions are on a bit-by-bit basis.

I like the approach, it could simplify devices and make them more self
documenting. Maybe devices could be also generated directly from HW
synthesis tool outputs.

How to couple this with Pins, memory API, VMState and reset handling
needs some thought.

There's some overlap also with PCI subsystem, it already implements
readonly bits.

>
> Helper functions are then used to access the uint32_t which observe the
> semantics defined by the UInt32StateInfo struct.

We also need uint8_t, uint16_t and uint64_t versions for some devices.
Perhaps it would be better to implement a uint64_t device which can be
used with shorter widths or even stronger connection with memory API.

>
> 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 sticky (nw0 and nw1)
> Reset values can be defined (reset)
> Bits can be marked to throw guest errors when written certain values (ge0, ge1)

Other bits could be marked as unimplemented (LOG_UNIMP).

> Bits can be marked clear on read (cor)
> Regsiters can be truncated in width (width)

s/Regsiters/Registers/

>
> 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.

For maximum flexibility, a callback could be specified but then we
overlap memory API.

Once we have Pin available, it could be useful to couple a register
bit directly with a Pin. Currently we could use qemu_irq. This would
mean that the dynamic state would need to be more complex than just
uint32_t. Also Pin could implement some of this functionality.

>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 0 deletions(-)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index affcc96..8ad0290 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h

I think this is not the right place since this is very much HW
specific, please introduce a new file.

> @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>      return (value & ~mask) | ((fieldval << start) & mask);
>  }
>
> +/**
> + * A descriptor for a Uint32 that is part of guest accessible device state
> + * @ro: whether or not the bit is read-only state comming out of reset

s/comming/coming/

> + * @w1c: bits with the common write 1 to clear semantic.
> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)

s/cant/can't/, also below

> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
> + * @reset: reset value.
> + * @ge1: Bits that when written 1 indicate a guest error
> + * @ge0: Bits that when written 0 indicate a guest error
> + * @cor: Bits that are clear on read
> + * @width: width of the uint32t. Only the @width least significant bits are
> + * valid. All others are silent Read-as-reset/WI.
> + */
> +
> +typedef struct UInt32StateInfo {
> +    const char *name;
> +    uint32_t ro;
> +    uint32_t w1c;
> +    uint32_t nw0;
> +    uint32_t nw1;
> +    uint32_t reset;
> +    uint32_t ge1;
> +    uint32_t ge0;
> +    uint32_t cor;
> +    int width;
> +} UInt32StateInfo;
> +
> +/**
> + * reset an array of u32s
> + * @array: array of u32s to reset
> + * @info: corresponding array of UInt32StateInfos to get reset values from
> + * @num: number of values to reset
> + */
> +
> +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
> +
> +/**
> + * write a value to a uint32_t subject to its restrictions
> + * @state: Pointer to location to be written
> + * @info: Definition of variable
> + * @val: value to write
> + * @prefix: Debug and error message prefix
> + * @debug: Turn on noisy debug printfery
> + */
> +
> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> +                  const char *prefix, bool debug);

Prefix could be part of the structure. I'd also combine state and
info, and avoid passing debug flag directly (it could be in the
dynamic structure or a pointer). Then it should be easy to be
compatible with memory API.

> +
> +/**
> + * write a value from a uint32_t subject to its restrictions

read

> + * @state: Pointer to location to be read
> + * @info: Definition of variable
> + * @prefix: Debug and error message prefix
> + * @debug: Turn on noisy debug printfery
> + */
> +
> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> +                     const char *prefix, bool debug);
> +
>  #endif
> diff --git a/util/bitops.c b/util/bitops.c
> index e72237a..51db476 100644
> --- a/util/bitops.c
> +++ b/util/bitops.c
> @@ -11,6 +11,8 @@
>   * 2 of the License, or (at your option) any later version.
>   */
>
> +#include "qemu-common.h"
> +#include "qemu/log.h"
>  #include "qemu/bitops.h"
>
>  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
> @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
>      /* Not found */
>      return size;
>  }
> +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
> +{
> +    int i = 0;
> +
> +    for (i = 0; i < num; ++i) {
> +        state[i] = info[i].reset;
> +    }
> +}

Perhaps this could be automatically registered.

> +
> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> +                  const char *prefix, bool debug)
> +{
> +    int i;
> +    uint32_t new_val;
> +    int width = info->width ? info->width : 32;
> +
> +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
> +                        ~((1ull << width) - 1);
> +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
> +                        ~((1ull << width) - 1);
> +
> +    if (!info->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> +                      "(written value: %#08x)\n", prefix, val);
> +        return;
> +    }
> +
> +    if (debug) {
> +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
> +                val);
> +    }
> +
> +    /*FIXME: skip over if no LOG_GUEST_ERROR */
> +    for (i = 0; i <= 1; ++i) {
> +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
> +        if (test) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
> +                          " to %d\n", prefix, info->name, test, i);
> +        }
> +    }
> +
> +    new_val = val & ~(no_w1_mask & val);
> +    new_val |= no_w1_mask & *state & val;
> +    new_val |= no_w0_mask & *state & ~val;
> +    new_val &= ~(val & info->w1c);
> +    *state = new_val;
> +}
> +
> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> +                     const char *prefix, bool debug)
> +{
> +    uint32_t ret = *state;
> +
> +    /* clear on read */
> +    ret &= ~info->cor;
> +    *state = ret;
> +
> +    if (!info->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
> +                      "(read value: %#08x)\n", prefix, ret);
> +        return ret;
> +    }
> +
> +    if (debug) {
> +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
> +    }
> +
> +    return ret;
> +}
> --
> 1.7.0.4
>
>

Re: [Qemu-devel] [RFC PATCH v1 1/4] qemu-log: Allow checking of the current mask

[Peter Maydell]

On 3 March 2013 06:13, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Useful for heavy users of qemu_log_mask that want to avoid executing expensive
> logic that sets up a qemu_log_mask when that mask is disabled. E.G.
>
> if (qemu_log_get_mask() && LOG_GUEST_ERROR) {
>         /* do my expensive logging data query */
> }
> qemu_log_mask(LOG_GUEST_ERROR, ...)

Why can't you use the existing
  if (qemu_loglevel_mask(LOG_GUEST_ERROR) {
      /* expensive thing */
  }
?

-- PMM

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Crosthwaite]

Hi Blue,

Thanks for the review. Comments in-line below. Are you on the IRC much
and what timezone? I'd like to ask a few questions about how you see
this fitting with the memory API, that would probably go much faster
in live discussion. I've commented on the issue below with my current
thoughts.

On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> This struct and functions provide some encapsulation of the uint32_t type to
>> make it more friendly for use as guest accessible device state. Bits of device
>> state (usually MMIO registers), often have all sorts of access restrictions
>> and semantics associated with them. This struct allow you to define what whose
>> restrictions are on a bit-by-bit basis.
>
> I like the approach, it could simplify devices and make them more self
> documenting. Maybe devices could be also generated directly from HW
> synthesis tool outputs.
>

Nice idea. Or at least a template device that reduces the coding to a
few /* YOUR CODE HERE */ is another possibility.

> How to couple this with Pins, memory API,

I was actually quite deliberate in making this de-coupled from the
memory API, The idea is any device that has guest accessible state can
use this, not just SysBus/PCI/MMIO. Even though the 90% use case is
making arrays of these definitions for memory mapped register files,
its usable for any software accessible device state. The ARM
co-processor interface is a good example of this. A random SPI or I2C
device may also have some registers that are accessed indirectly over
their respective transport layers. It'd be nice to be able to use this
API independent of the memory API in those cases. That said if you can
see any way to modify the API to make it play nicer with the Memory
API I'd definitely take it onboard.

> VMState and reset handling
> needs some thought.
>

If we add VMState and reset then we are only one step away from these
registers being a QOM object (TYPE_DEVICE) in their own right. Might
not be a bad thing but does strike me as heavy-weight. It is a good
question where do we draw the line between the responsibilities of the
device and the API. Here im am shooting for the grey area between full
QOM device for every register and the status quo (no API at all).
Regarding VMState, the current implementation the devices dynamic
state is still limited to a uint32_t array so a single
VMSTATE_UINT32_ARRAY will do the job.

> There's some overlap also with PCI subsystem, it already implements
> readonly bits.
>
>>
>> Helper functions are then used to access the uint32_t which observe the
>> semantics defined by the UInt32StateInfo struct.
>
> We also need uint8_t, uint16_t and uint64_t versions for some devices.
> Perhaps it would be better to implement a uint64_t device which can be
> used with shorter widths or even stronger connection with memory API.
>

Yes. Going lowest common denominator with uint64_t does however cost
more memory and VMS storage which could impact devices with large
register spaces. This wouldn't impact any of my devices in any way
though. The performance hit is probably worth it for the simplicity.
V2 will be 64 bit. I will drop the "64" from the function name to keep
it generic and we can assert that no one tries to create a register
with .width > 64.

>>
>> 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 sticky (nw0 and nw1)
>> Reset values can be defined (reset)
>> Bits can be marked to throw guest errors when written certain values (ge0, ge1)
>
> Other bits could be marked as unimplemented (LOG_UNIMP).
>

Agreed. Will add to V2. Another Idea id like to play with is adding
per-bit explanations for the LOG_UNIMP and LOG_GUEST_ERROR. Just
arrays of const char * in the definitions that cat onto the qemu_log
msgs telling you why that was a guest error or exactly what feature is
unimplemented.

>> Bits can be marked clear on read (cor)
>> Regsiters can be truncated in width (width)
>
> s/Regsiters/Registers/
>

Will fix.

>>
>> 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.
>
> For maximum flexibility, a callback could be specified but then we
> overlap memory API.
>

I think this is a good idea, but continuing on the theme of what this
API is trying to achieve I think there should be capability for
per-bit function definitions. On the topic I think Gerd has played
with the idea of per-register callbacks for some device models in the
past.

> Once we have Pin available, it could be useful to couple a register
> bit directly with a Pin. Currently we could use qemu_irq.

Ill wait on Pin to land and stabilise for this one and continue to use
code driven GPIO setters.

> This would
> mean that the dynamic state would need to be more complex than just
> uint32_t. Also Pin could implement some of this functionality.
>
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>>  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 130 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index affcc96..8ad0290 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>
> I think this is not the right place since this is very much HW
> specific, please introduce a new file.
>
>> @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>>      return (value & ~mask) | ((fieldval << start) & mask);
>>  }
>>
>> +/**
>> + * A descriptor for a Uint32 that is part of guest accessible device state
>> + * @ro: whether or not the bit is read-only state comming out of reset
>
> s/comming/coming/
>

Will fix

>> + * @w1c: bits with the common write 1 to clear semantic.
>> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
>
> s/cant/can't/, also below
>

Will fix

>> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
>> + * @reset: reset value.
>> + * @ge1: Bits that when written 1 indicate a guest error
>> + * @ge0: Bits that when written 0 indicate a guest error
>> + * @cor: Bits that are clear on read
>> + * @width: width of the uint32t. Only the @width least significant bits are
>> + * valid. All others are silent Read-as-reset/WI.
>> + */
>> +
>> +typedef struct UInt32StateInfo {
>> +    const char *name;
>> +    uint32_t ro;
>> +    uint32_t w1c;
>> +    uint32_t nw0;
>> +    uint32_t nw1;
>> +    uint32_t reset;
>> +    uint32_t ge1;
>> +    uint32_t ge0;
>> +    uint32_t cor;
>> +    int width;
>> +} UInt32StateInfo;
>> +
>> +/**
>> + * reset an array of u32s
>> + * @array: array of u32s to reset
>> + * @info: corresponding array of UInt32StateInfos to get reset values from
>> + * @num: number of values to reset
>> + */
>> +
>> +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
>> +
>> +/**
>> + * write a value to a uint32_t subject to its restrictions
>> + * @state: Pointer to location to be written
>> + * @info: Definition of variable
>> + * @val: value to write
>> + * @prefix: Debug and error message prefix
>> + * @debug: Turn on noisy debug printfery
>> + */
>> +
>> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>> +                  const char *prefix, bool debug);
>
> Prefix could be part of the structure.

Prefix is an awkward one as its only common to a single device. E.G.
My first UART controller could have the prefix "UART1" while the
second has "UART2". Currently the property info here are created as
static const. To fold prefix into info each device would have to clone
a template info and then populate prefix at object initialize time. I
am currently just using the object canonical path strcatted with the
register offset for this as that gives a unique per-device prefix
rather than per-device-type.

> I'd also combine state and
> info,

Similar problem. info would need to be per-device and no longer
shared. Not such a bad thing though.

> and avoid passing debug flag directly (it could be in the
> dynamic structure or a pointer).

Debug as used in its current form could actually live in the static
structure. I'm just thinking if there are any nice ways to set that up
such that you don't have to add ".debug = FOO_DEBUG" to every table
entry. Although if we end up de-constifying the table to fold in the
prefix and state (as discussed above) then a helper function can just
iterate over the table and sort it out in one go.

Any by now you've convinced me :) Ill add some per-instance state in
V2 to store this stuff. The const table example I've given in P3 will
be unchanged. It will reduce this function prototype to two args (info
and val). There will a few new this and that helpers that object init
fns can call to set this up in a handful of lines.

> Then it should be easy to be
> compatible with memory API.
>
>> +
>> +/**
>> + * write a value from a uint32_t subject to its restrictions
>
> read
>

Will fix.

Regards,
Peter

>> + * @state: Pointer to location to be read
>> + * @info: Definition of variable
>> + * @prefix: Debug and error message prefix
>> + * @debug: Turn on noisy debug printfery
>> + */
>> +
>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>> +                     const char *prefix, bool debug);
>> +
>>  #endif
>> diff --git a/util/bitops.c b/util/bitops.c
>> index e72237a..51db476 100644
>> --- a/util/bitops.c
>> +++ b/util/bitops.c
>> @@ -11,6 +11,8 @@
>>   * 2 of the License, or (at your option) any later version.
>>   */
>>
>> +#include "qemu-common.h"
>> +#include "qemu/log.h"
>>  #include "qemu/bitops.h"
>>
>>  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
>> @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
>>      /* Not found */
>>      return size;
>>  }
>> +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
>> +{
>> +    int i = 0;
>> +
>> +    for (i = 0; i < num; ++i) {
>> +        state[i] = info[i].reset;
>> +    }
>> +}
>
> Perhaps this could be automatically registered.
>
>> +
>> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>> +                  const char *prefix, bool debug)
>> +{
>> +    int i;
>> +    uint32_t new_val;
>> +    int width = info->width ? info->width : 32;
>> +
>> +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
>> +                        ~((1ull << width) - 1);
>> +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
>> +                        ~((1ull << width) - 1);
>> +
>> +    if (!info->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>> +                      "(written value: %#08x)\n", prefix, val);
>> +        return;
>> +    }
>> +
>> +    if (debug) {
>> +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
>> +                val);
>> +    }
>> +
>> +    /*FIXME: skip over if no LOG_GUEST_ERROR */
>> +    for (i = 0; i <= 1; ++i) {
>> +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
>> +        if (test) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
>> +                          " to %d\n", prefix, info->name, test, i);
>> +        }
>> +    }
>> +
>> +    new_val = val & ~(no_w1_mask & val);
>> +    new_val |= no_w1_mask & *state & val;
>> +    new_val |= no_w0_mask & *state & ~val;
>> +    new_val &= ~(val & info->w1c);
>> +    *state = new_val;
>> +}
>> +
>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>> +                     const char *prefix, bool debug)
>> +{
>> +    uint32_t ret = *state;
>> +
>> +    /* clear on read */
>> +    ret &= ~info->cor;
>> +    *state = ret;
>> +
>> +    if (!info->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
>> +                      "(read value: %#08x)\n", prefix, ret);
>> +        return ret;
>> +    }
>> +
>> +    if (debug) {
>> +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
>> +    }
>> +
>> +    return ret;
>> +}
>> --
>> 1.7.0.4
>>
>>
>

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Gerd Hoffmann]

> +/**
> + * A descriptor for a Uint32 that is part of guest accessible device state
> + * @ro: whether or not the bit is read-only state comming out of reset
> + * @w1c: bits with the common write 1 to clear semantic.

> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)

Why these?

reset=0 + ro=1 equals nw1=1
reset=1 + ro=1 equals nw0=1

> + * @width: width of the uint32t. Only the @width least significant bits are
> + * valid. All others are silent Read-as-reset/WI.

That's kida redundant with "ro" too.  I'd do it the other way around
btw:  Specify the writable bits instead of the read-only ones.  width=8
easily be written as wmask=0xff then.

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Crosthwaite]

Hi Gerd,

On Mon, Mar 4, 2013 at 4:55 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> +/**
>> + * A descriptor for a Uint32 that is part of guest accessible device state
>> + * @ro: whether or not the bit is read-only state comming out of reset
>> + * @w1c: bits with the common write 1 to clear semantic.
>
>> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
>> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
>
> Why these?
>
> reset=0 + ro=1 equals nw1=1
> reset=1 + ro=1 equals nw0=1
>

Some bits in my devices can only be written one-way they are either
"stuck on" once they are written, or only the hardware is allowed to
write the other way. Setting both nw0 and nw1 is equivalent to ro. In
the devcfg example, bits in the LOCK bits are sticky (the idea is once
the firmware locks them the booted system cant unlock them for
security and we want to test the firmware as a guest).

>> + * @width: width of the uint32t. Only the @width least significant bits are
>> + * valid. All others are silent Read-as-reset/WI.
>
> That's kida redundant with "ro" too.  I'd do it the other way around
> btw:  Specify the writable bits instead of the read-only ones.  width=8
> easily be written as wmask=0xff then.

In V2 ill be doing some work to make this more flexible regarding
widths to address Blues comments. Ill incorporate this as well, and I
think I can make it work getting rid of the width altogether.

Regards,
Peter

>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Gerd Hoffmann]

  Hi,

>> For maximum flexibility, a callback could be specified but then we
>> overlap memory API.
> 
> I think this is a good idea, but continuing on the theme of what this
> API is trying to achieve I think there should be capability for
> per-bit function definitions. On the topic I think Gerd has played
> with the idea of per-register callbacks for some device models in the
> past.

hw/intel-hda.c is it.

Predates memory api though and thus doesn't integrated with it, although
it makes sense of course.  Could be as simple as a generic MemoryRegionOps.

I'm not sure it is that useful to have per-bit callbacks.  For "normal"
devices you rarely need that, and for gpio arrays probably want more
than just callbacks per bit (qemu_irqs for example).

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Gerd Hoffmann]

  Hi,

>>> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
>>> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
>>
>> Why these?
>>
>> reset=0 + ro=1 equals nw1=1
>> reset=1 + ro=1 equals nw0=1
>>
> 
> Some bits in my devices can only be written one-way they are either
> "stuck on" once they are written, or only the hardware is allowed to
> write the other way.

Ah, ok.  Makes sense.  Can you make the description a bit more verbose
to clarify this?

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Michael S. Tsirkin]

On Sun, Mar 03, 2013 at 09:01:11AM +0000, Blue Swirl wrote:
> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
> > This struct and functions provide some encapsulation of the uint32_t type to
> > make it more friendly for use as guest accessible device state. Bits of device
> > state (usually MMIO registers), often have all sorts of access restrictions
> > and semantics associated with them. This struct allow you to define what whose
> > restrictions are on a bit-by-bit basis.
> 
> I like the approach, it could simplify devices and make them more self
> documenting. Maybe devices could be also generated directly from HW
> synthesis tool outputs.
> 
> How to couple this with Pins, memory API, VMState and reset handling
> needs some thought.
> 
> There's some overlap also with PCI subsystem, it already implements
> readonly bits.

One other interesting feature that pci has is migration
sanity checks: a bit can be marked as immutable
which means that its value must be consistent on
migration source and destination.
This is often the case for read-only bits - but not always,
as bits could be set externally.

Further, pci also supports a bit based allocator, so
if you need a range of bits for a capability you
can allocate them cleanly.

> >
> > Helper functions are then used to access the uint32_t which observe the
> > semantics defined by the UInt32StateInfo struct.
> 
> We also need uint8_t, uint16_t and uint64_t versions for some devices.
> Perhaps it would be better to implement a uint64_t device which can be
> used with shorter widths or even stronger connection with memory API.

Why not uint8_t for everyone?

> >
> > 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 sticky (nw0 and nw1)
> > Reset values can be defined (reset)
> > Bits can be marked to throw guest errors when written certain values (ge0, ge1)
> 
> Other bits could be marked as unimplemented (LOG_UNIMP).
> 
> > Bits can be marked clear on read (cor)
> > Regsiters can be truncated in width (width)
> 
> s/Regsiters/Registers/
> 
> >
> > 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.
> 
> For maximum flexibility, a callback could be specified but then we
> overlap memory API.
> 
> Once we have Pin available, it could be useful to couple a register
> bit directly with a Pin. Currently we could use qemu_irq. This would
> mean that the dynamic state would need to be more complex than just
> uint32_t. Also Pin could implement some of this functionality.
> 
> >
> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> >
> >  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
> >  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 130 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index affcc96..8ad0290 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> 
> I think this is not the right place since this is very much HW
> specific, please introduce a new file.
> 
> > @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
> >      return (value & ~mask) | ((fieldval << start) & mask);
> >  }
> >
> > +/**
> > + * A descriptor for a Uint32 that is part of guest accessible device state
> > + * @ro: whether or not the bit is read-only state comming out of reset
> 
> s/comming/coming/
> 
> > + * @w1c: bits with the common write 1 to clear semantic.
> > + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
> 
> s/cant/can't/, also below
> 
> > + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
> > + * @reset: reset value.
> > + * @ge1: Bits that when written 1 indicate a guest error
> > + * @ge0: Bits that when written 0 indicate a guest error
> > + * @cor: Bits that are clear on read
> > + * @width: width of the uint32t. Only the @width least significant bits are
> > + * valid. All others are silent Read-as-reset/WI.
> > + */
> > +
> > +typedef struct UInt32StateInfo {
> > +    const char *name;
> > +    uint32_t ro;
> > +    uint32_t w1c;
> > +    uint32_t nw0;
> > +    uint32_t nw1;
> > +    uint32_t reset;
> > +    uint32_t ge1;
> > +    uint32_t ge0;
> > +    uint32_t cor;
> > +    int width;
> > +} UInt32StateInfo;
> > +
> > +/**
> > + * reset an array of u32s
> > + * @array: array of u32s to reset
> > + * @info: corresponding array of UInt32StateInfos to get reset values from
> > + * @num: number of values to reset
> > + */
> > +
> > +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
> > +
> > +/**
> > + * write a value to a uint32_t subject to its restrictions
> > + * @state: Pointer to location to be written
> > + * @info: Definition of variable
> > + * @val: value to write
> > + * @prefix: Debug and error message prefix
> > + * @debug: Turn on noisy debug printfery
> > + */
> > +
> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> > +                  const char *prefix, bool debug);
> 
> Prefix could be part of the structure. I'd also combine state and
> info, and avoid passing debug flag directly (it could be in the
> dynamic structure or a pointer). Then it should be easy to be
> compatible with memory API.
> 
> > +
> > +/**
> > + * write a value from a uint32_t subject to its restrictions
> 
> read
> 
> > + * @state: Pointer to location to be read
> > + * @info: Definition of variable
> > + * @prefix: Debug and error message prefix
> > + * @debug: Turn on noisy debug printfery
> > + */
> > +
> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> > +                     const char *prefix, bool debug);
> > +
> >  #endif
> > diff --git a/util/bitops.c b/util/bitops.c
> > index e72237a..51db476 100644
> > --- a/util/bitops.c
> > +++ b/util/bitops.c
> > @@ -11,6 +11,8 @@
> >   * 2 of the License, or (at your option) any later version.
> >   */
> >
> > +#include "qemu-common.h"
> > +#include "qemu/log.h"
> >  #include "qemu/bitops.h"
> >
> >  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
> > @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
> >      /* Not found */
> >      return size;
> >  }
> > +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
> > +{
> > +    int i = 0;
> > +
> > +    for (i = 0; i < num; ++i) {
> > +        state[i] = info[i].reset;
> > +    }
> > +}
> 
> Perhaps this could be automatically registered.
> 
> > +
> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> > +                  const char *prefix, bool debug)
> > +{
> > +    int i;
> > +    uint32_t new_val;
> > +    int width = info->width ? info->width : 32;
> > +
> > +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
> > +                        ~((1ull << width) - 1);
> > +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
> > +                        ~((1ull << width) - 1);
> > +
> > +    if (!info->name) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> > +                      "(written value: %#08x)\n", prefix, val);
> > +        return;
> > +    }
> > +
> > +    if (debug) {
> > +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
> > +                val);
> > +    }
> > +
> > +    /*FIXME: skip over if no LOG_GUEST_ERROR */
> > +    for (i = 0; i <= 1; ++i) {
> > +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
> > +        if (test) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
> > +                          " to %d\n", prefix, info->name, test, i);
> > +        }
> > +    }
> > +
> > +    new_val = val & ~(no_w1_mask & val);
> > +    new_val |= no_w1_mask & *state & val;
> > +    new_val |= no_w0_mask & *state & ~val;
> > +    new_val &= ~(val & info->w1c);
> > +    *state = new_val;
> > +}
> > +
> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> > +                     const char *prefix, bool debug)
> > +{
> > +    uint32_t ret = *state;
> > +
> > +    /* clear on read */
> > +    ret &= ~info->cor;
> > +    *state = ret;
> > +
> > +    if (!info->name) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
> > +                      "(read value: %#08x)\n", prefix, ret);
> > +        return ret;
> > +    }
> > +
> > +    if (debug) {
> > +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
> > +    }
> > +
> > +    return ret;
> > +}
> > --
> > 1.7.0.4
> >
> >

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Blue Swirl]

On Mon, Mar 4, 2013 at 1:37 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Blue,
>
> Thanks for the review. Comments in-line below. Are you on the IRC much
> and what timezone? I'd like to ask a few questions about how you see
> this fitting with the memory API, that would probably go much faster
> in live discussion. I've commented on the issue below with my current
> thoughts.
>
> On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> This struct and functions provide some encapsulation of the uint32_t type to
>>> make it more friendly for use as guest accessible device state. Bits of device
>>> state (usually MMIO registers), often have all sorts of access restrictions
>>> and semantics associated with them. This struct allow you to define what whose
>>> restrictions are on a bit-by-bit basis.
>>
>> I like the approach, it could simplify devices and make them more self
>> documenting. Maybe devices could be also generated directly from HW
>> synthesis tool outputs.
>>
>
> Nice idea. Or at least a template device that reduces the coding to a
> few /* YOUR CODE HERE */ is another possibility.
>
>> How to couple this with Pins, memory API,
>
> I was actually quite deliberate in making this de-coupled from the
> memory API, The idea is any device that has guest accessible state can
> use this, not just SysBus/PCI/MMIO. Even though the 90% use case is
> making arrays of these definitions for memory mapped register files,
> its usable for any software accessible device state. The ARM
> co-processor interface is a good example of this. A random SPI or I2C
> device may also have some registers that are accessed indirectly over
> their respective transport layers. It'd be nice to be able to use this
> API independent of the memory API in those cases. That said if you can
> see any way to modify the API to make it play nicer with the Memory
> API I'd definitely take it onboard.

I think Avi mentioned that that memory API should be operating at
single register level one day and your model looks like we could use
that as one step. Perhaps the best of all worlds would be to keep
things compatible with memory API so that it can be used if needed.

>
>> VMState and reset handling
>> needs some thought.
>>
>
> If we add VMState and reset then we are only one step away from these
> registers being a QOM object (TYPE_DEVICE) in their own right. Might
> not be a bad thing but does strike me as heavy-weight. It is a good
> question where do we draw the line between the responsibilities of the
> device and the API. Here im am shooting for the grey area between full
> QOM device for every register and the status quo (no API at all).
> Regarding VMState, the current implementation the devices dynamic
> state is still limited to a uint32_t array so a single
> VMSTATE_UINT32_ARRAY will do the job.

Yes, QOM and VMState probably model bigger entities than single
register entries. Specifying everything in one place would be nice
though.

>
>> There's some overlap also with PCI subsystem, it already implements
>> readonly bits.
>>
>>>
>>> Helper functions are then used to access the uint32_t which observe the
>>> semantics defined by the UInt32StateInfo struct.
>>
>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
>> Perhaps it would be better to implement a uint64_t device which can be
>> used with shorter widths or even stronger connection with memory API.
>>
>
> Yes. Going lowest common denominator with uint64_t does however cost
> more memory and VMS storage which could impact devices with large
> register spaces. This wouldn't impact any of my devices in any way
> though. The performance hit is probably worth it for the simplicity.
> V2 will be 64 bit. I will drop the "64" from the function name to keep
> it generic and we can assert that no one tries to create a register
> with .width > 64.
>
>>>
>>> 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 sticky (nw0 and nw1)
>>> Reset values can be defined (reset)
>>> Bits can be marked to throw guest errors when written certain values (ge0, ge1)
>>
>> Other bits could be marked as unimplemented (LOG_UNIMP).
>>
>
> Agreed. Will add to V2. Another Idea id like to play with is adding
> per-bit explanations for the LOG_UNIMP and LOG_GUEST_ERROR. Just
> arrays of const char * in the definitions that cat onto the qemu_log
> msgs telling you why that was a guest error or exactly what feature is
> unimplemented.
>
>>> Bits can be marked clear on read (cor)
>>> Regsiters can be truncated in width (width)
>>
>> s/Regsiters/Registers/
>>
>
> Will fix.
>
>>>
>>> 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.
>>
>> For maximum flexibility, a callback could be specified but then we
>> overlap memory API.
>>
>
> I think this is a good idea, but continuing on the theme of what this
> API is trying to achieve I think there should be capability for
> per-bit function definitions. On the topic I think Gerd has played
> with the idea of per-register callbacks for some device models in the
> past.
>
>> Once we have Pin available, it could be useful to couple a register
>> bit directly with a Pin. Currently we could use qemu_irq.
>
> Ill wait on Pin to land and stabilise for this one and continue to use
> code driven GPIO setters.
>
>> This would
>> mean that the dynamic state would need to be more complex than just
>> uint32_t. Also Pin could implement some of this functionality.
>>
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>>  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>>>  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 130 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>>> index affcc96..8ad0290 100644
>>> --- a/include/qemu/bitops.h
>>> +++ b/include/qemu/bitops.h
>>
>> I think this is not the right place since this is very much HW
>> specific, please introduce a new file.
>>
>>> @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>>>      return (value & ~mask) | ((fieldval << start) & mask);
>>>  }
>>>
>>> +/**
>>> + * A descriptor for a Uint32 that is part of guest accessible device state
>>> + * @ro: whether or not the bit is read-only state comming out of reset
>>
>> s/comming/coming/
>>
>
> Will fix
>
>>> + * @w1c: bits with the common write 1 to clear semantic.
>>> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
>>
>> s/cant/can't/, also below
>>
>
> Will fix
>
>>> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
>>> + * @reset: reset value.
>>> + * @ge1: Bits that when written 1 indicate a guest error
>>> + * @ge0: Bits that when written 0 indicate a guest error
>>> + * @cor: Bits that are clear on read
>>> + * @width: width of the uint32t. Only the @width least significant bits are
>>> + * valid. All others are silent Read-as-reset/WI.
>>> + */
>>> +
>>> +typedef struct UInt32StateInfo {
>>> +    const char *name;
>>> +    uint32_t ro;
>>> +    uint32_t w1c;
>>> +    uint32_t nw0;
>>> +    uint32_t nw1;
>>> +    uint32_t reset;
>>> +    uint32_t ge1;
>>> +    uint32_t ge0;
>>> +    uint32_t cor;
>>> +    int width;
>>> +} UInt32StateInfo;
>>> +
>>> +/**
>>> + * reset an array of u32s
>>> + * @array: array of u32s to reset
>>> + * @info: corresponding array of UInt32StateInfos to get reset values from
>>> + * @num: number of values to reset
>>> + */
>>> +
>>> +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
>>> +
>>> +/**
>>> + * write a value to a uint32_t subject to its restrictions
>>> + * @state: Pointer to location to be written
>>> + * @info: Definition of variable
>>> + * @val: value to write
>>> + * @prefix: Debug and error message prefix
>>> + * @debug: Turn on noisy debug printfery
>>> + */
>>> +
>>> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>>> +                  const char *prefix, bool debug);
>>
>> Prefix could be part of the structure.
>
> Prefix is an awkward one as its only common to a single device. E.G.
> My first UART controller could have the prefix "UART1" while the
> second has "UART2". Currently the property info here are created as
> static const. To fold prefix into info each device would have to clone
> a template info and then populate prefix at object initialize time. I
> am currently just using the object canonical path strcatted with the
> register offset for this as that gives a unique per-device prefix
> rather than per-device-type.

I think this is asking that the prefix should be handled at device
level and register objects should be always part of a parent device
object so that register methods can always query parent device name.

>
>> I'd also combine state and
>> info,
>
> Similar problem. info would need to be per-device and no longer
> shared. Not such a bad thing though.
>
>> and avoid passing debug flag directly (it could be in the
>> dynamic structure or a pointer).
>
> Debug as used in its current form could actually live in the static
> structure. I'm just thinking if there are any nice ways to set that up
> such that you don't have to add ".debug = FOO_DEBUG" to every table
> entry. Although if we end up de-constifying the table to fold in the
> prefix and state (as discussed above) then a helper function can just
> iterate over the table and sort it out in one go.

The dynamic structure could have a pointer to the static const struct.

>
> Any by now you've convinced me :) Ill add some per-instance state in
> V2 to store this stuff. The const table example I've given in P3 will
> be unchanged. It will reduce this function prototype to two args (info
> and val). There will a few new this and that helpers that object init
> fns can call to set this up in a handful of lines.
>
>> Then it should be easy to be
>> compatible with memory API.
>>
>>> +
>>> +/**
>>> + * write a value from a uint32_t subject to its restrictions
>>
>> read
>>
>
> Will fix.
>
> Regards,
> Peter
>
>>> + * @state: Pointer to location to be read
>>> + * @info: Definition of variable
>>> + * @prefix: Debug and error message prefix
>>> + * @debug: Turn on noisy debug printfery
>>> + */
>>> +
>>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>>> +                     const char *prefix, bool debug);
>>> +
>>>  #endif
>>> diff --git a/util/bitops.c b/util/bitops.c
>>> index e72237a..51db476 100644
>>> --- a/util/bitops.c
>>> +++ b/util/bitops.c
>>> @@ -11,6 +11,8 @@
>>>   * 2 of the License, or (at your option) any later version.
>>>   */
>>>
>>> +#include "qemu-common.h"
>>> +#include "qemu/log.h"
>>>  #include "qemu/bitops.h"
>>>
>>>  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
>>> @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
>>>      /* Not found */
>>>      return size;
>>>  }
>>> +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
>>> +{
>>> +    int i = 0;
>>> +
>>> +    for (i = 0; i < num; ++i) {
>>> +        state[i] = info[i].reset;
>>> +    }
>>> +}
>>
>> Perhaps this could be automatically registered.
>>
>>> +
>>> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>>> +                  const char *prefix, bool debug)
>>> +{
>>> +    int i;
>>> +    uint32_t new_val;
>>> +    int width = info->width ? info->width : 32;
>>> +
>>> +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
>>> +                        ~((1ull << width) - 1);
>>> +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
>>> +                        ~((1ull << width) - 1);
>>> +
>>> +    if (!info->name) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>>> +                      "(written value: %#08x)\n", prefix, val);
>>> +        return;
>>> +    }
>>> +
>>> +    if (debug) {
>>> +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
>>> +                val);
>>> +    }
>>> +
>>> +    /*FIXME: skip over if no LOG_GUEST_ERROR */
>>> +    for (i = 0; i <= 1; ++i) {
>>> +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
>>> +        if (test) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
>>> +                          " to %d\n", prefix, info->name, test, i);
>>> +        }
>>> +    }
>>> +
>>> +    new_val = val & ~(no_w1_mask & val);
>>> +    new_val |= no_w1_mask & *state & val;
>>> +    new_val |= no_w0_mask & *state & ~val;
>>> +    new_val &= ~(val & info->w1c);
>>> +    *state = new_val;
>>> +}
>>> +
>>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>>> +                     const char *prefix, bool debug)
>>> +{
>>> +    uint32_t ret = *state;
>>> +
>>> +    /* clear on read */
>>> +    ret &= ~info->cor;
>>> +    *state = ret;
>>> +
>>> +    if (!info->name) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
>>> +                      "(read value: %#08x)\n", prefix, ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (debug) {
>>> +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> --
>>> 1.7.0.4
>>>
>>>
>>

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Blue Swirl]

On Mon, Mar 4, 2013 at 9:44 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Mar 03, 2013 at 09:01:11AM +0000, Blue Swirl wrote:
>> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>> > This struct and functions provide some encapsulation of the uint32_t type to
>> > make it more friendly for use as guest accessible device state. Bits of device
>> > state (usually MMIO registers), often have all sorts of access restrictions
>> > and semantics associated with them. This struct allow you to define what whose
>> > restrictions are on a bit-by-bit basis.
>>
>> I like the approach, it could simplify devices and make them more self
>> documenting. Maybe devices could be also generated directly from HW
>> synthesis tool outputs.
>>
>> How to couple this with Pins, memory API, VMState and reset handling
>> needs some thought.
>>
>> There's some overlap also with PCI subsystem, it already implements
>> readonly bits.
>
> One other interesting feature that pci has is migration
> sanity checks: a bit can be marked as immutable
> which means that its value must be consistent on
> migration source and destination.
> This is often the case for read-only bits - but not always,
> as bits could be set externally.
>
> Further, pci also supports a bit based allocator, so
> if you need a range of bits for a capability you
> can allocate them cleanly.

Maybe it would be useful to make these features available for other devices.

>
>> >
>> > Helper functions are then used to access the uint32_t which observe the
>> > semantics defined by the UInt32StateInfo struct.
>>
>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
>> Perhaps it would be better to implement a uint64_t device which can be
>> used with shorter widths or even stronger connection with memory API.
>
> Why not uint8_t for everyone?

That would be simple, but then modeling for example 32 bit registers
gets clumsy. The register model could provide unions for accessing
different width entities though.

>
>> >
>> > 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 sticky (nw0 and nw1)
>> > Reset values can be defined (reset)
>> > Bits can be marked to throw guest errors when written certain values (ge0, ge1)
>>
>> Other bits could be marked as unimplemented (LOG_UNIMP).
>>
>> > Bits can be marked clear on read (cor)
>> > Regsiters can be truncated in width (width)
>>
>> s/Regsiters/Registers/
>>
>> >
>> > 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.
>>
>> For maximum flexibility, a callback could be specified but then we
>> overlap memory API.
>>
>> Once we have Pin available, it could be useful to couple a register
>> bit directly with a Pin. Currently we could use qemu_irq. This would
>> mean that the dynamic state would need to be more complex than just
>> uint32_t. Also Pin could implement some of this functionality.
>>
>> >
>> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> > ---
>> >
>> >  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>> >  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 130 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> > index affcc96..8ad0290 100644
>> > --- a/include/qemu/bitops.h
>> > +++ b/include/qemu/bitops.h
>>
>> I think this is not the right place since this is very much HW
>> specific, please introduce a new file.
>>
>> > @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>> >      return (value & ~mask) | ((fieldval << start) & mask);
>> >  }
>> >
>> > +/**
>> > + * A descriptor for a Uint32 that is part of guest accessible device state
>> > + * @ro: whether or not the bit is read-only state comming out of reset
>>
>> s/comming/coming/
>>
>> > + * @w1c: bits with the common write 1 to clear semantic.
>> > + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
>>
>> s/cant/can't/, also below
>>
>> > + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
>> > + * @reset: reset value.
>> > + * @ge1: Bits that when written 1 indicate a guest error
>> > + * @ge0: Bits that when written 0 indicate a guest error
>> > + * @cor: Bits that are clear on read
>> > + * @width: width of the uint32t. Only the @width least significant bits are
>> > + * valid. All others are silent Read-as-reset/WI.
>> > + */
>> > +
>> > +typedef struct UInt32StateInfo {
>> > +    const char *name;
>> > +    uint32_t ro;
>> > +    uint32_t w1c;
>> > +    uint32_t nw0;
>> > +    uint32_t nw1;
>> > +    uint32_t reset;
>> > +    uint32_t ge1;
>> > +    uint32_t ge0;
>> > +    uint32_t cor;
>> > +    int width;
>> > +} UInt32StateInfo;
>> > +
>> > +/**
>> > + * reset an array of u32s
>> > + * @array: array of u32s to reset
>> > + * @info: corresponding array of UInt32StateInfos to get reset values from
>> > + * @num: number of values to reset
>> > + */
>> > +
>> > +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
>> > +
>> > +/**
>> > + * write a value to a uint32_t subject to its restrictions
>> > + * @state: Pointer to location to be written
>> > + * @info: Definition of variable
>> > + * @val: value to write
>> > + * @prefix: Debug and error message prefix
>> > + * @debug: Turn on noisy debug printfery
>> > + */
>> > +
>> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>> > +                  const char *prefix, bool debug);
>>
>> Prefix could be part of the structure. I'd also combine state and
>> info, and avoid passing debug flag directly (it could be in the
>> dynamic structure or a pointer). Then it should be easy to be
>> compatible with memory API.
>>
>> > +
>> > +/**
>> > + * write a value from a uint32_t subject to its restrictions
>>
>> read
>>
>> > + * @state: Pointer to location to be read
>> > + * @info: Definition of variable
>> > + * @prefix: Debug and error message prefix
>> > + * @debug: Turn on noisy debug printfery
>> > + */
>> > +
>> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>> > +                     const char *prefix, bool debug);
>> > +
>> >  #endif
>> > diff --git a/util/bitops.c b/util/bitops.c
>> > index e72237a..51db476 100644
>> > --- a/util/bitops.c
>> > +++ b/util/bitops.c
>> > @@ -11,6 +11,8 @@
>> >   * 2 of the License, or (at your option) any later version.
>> >   */
>> >
>> > +#include "qemu-common.h"
>> > +#include "qemu/log.h"
>> >  #include "qemu/bitops.h"
>> >
>> >  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
>> > @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
>> >      /* Not found */
>> >      return size;
>> >  }
>> > +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
>> > +{
>> > +    int i = 0;
>> > +
>> > +    for (i = 0; i < num; ++i) {
>> > +        state[i] = info[i].reset;
>> > +    }
>> > +}
>>
>> Perhaps this could be automatically registered.
>>
>> > +
>> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
>> > +                  const char *prefix, bool debug)
>> > +{
>> > +    int i;
>> > +    uint32_t new_val;
>> > +    int width = info->width ? info->width : 32;
>> > +
>> > +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
>> > +                        ~((1ull << width) - 1);
>> > +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
>> > +                        ~((1ull << width) - 1);
>> > +
>> > +    if (!info->name) {
>> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
>> > +                      "(written value: %#08x)\n", prefix, val);
>> > +        return;
>> > +    }
>> > +
>> > +    if (debug) {
>> > +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
>> > +                val);
>> > +    }
>> > +
>> > +    /*FIXME: skip over if no LOG_GUEST_ERROR */
>> > +    for (i = 0; i <= 1; ++i) {
>> > +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
>> > +        if (test) {
>> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
>> > +                          " to %d\n", prefix, info->name, test, i);
>> > +        }
>> > +    }
>> > +
>> > +    new_val = val & ~(no_w1_mask & val);
>> > +    new_val |= no_w1_mask & *state & val;
>> > +    new_val |= no_w0_mask & *state & ~val;
>> > +    new_val &= ~(val & info->w1c);
>> > +    *state = new_val;
>> > +}
>> > +
>> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>> > +                     const char *prefix, bool debug)
>> > +{
>> > +    uint32_t ret = *state;
>> > +
>> > +    /* clear on read */
>> > +    ret &= ~info->cor;
>> > +    *state = ret;
>> > +
>> > +    if (!info->name) {
>> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
>> > +                      "(read value: %#08x)\n", prefix, ret);
>> > +        return ret;
>> > +    }
>> > +
>> > +    if (debug) {
>> > +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
>> > +    }
>> > +
>> > +    return ret;
>> > +}
>> > --
>> > 1.7.0.4
>> >
>> >

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Michael S. Tsirkin]

On Mon, Mar 04, 2013 at 08:52:33PM +0000, Blue Swirl wrote:
> On Mon, Mar 4, 2013 at 9:44 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Mar 03, 2013 at 09:01:11AM +0000, Blue Swirl wrote:
> >> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
> >> <peter.crosthwaite@xilinx.com> wrote:
> >> > This struct and functions provide some encapsulation of the uint32_t type to
> >> > make it more friendly for use as guest accessible device state. Bits of device
> >> > state (usually MMIO registers), often have all sorts of access restrictions
> >> > and semantics associated with them. This struct allow you to define what whose
> >> > restrictions are on a bit-by-bit basis.
> >>
> >> I like the approach, it could simplify devices and make them more self
> >> documenting. Maybe devices could be also generated directly from HW
> >> synthesis tool outputs.
> >>
> >> How to couple this with Pins, memory API, VMState and reset handling
> >> needs some thought.
> >>
> >> There's some overlap also with PCI subsystem, it already implements
> >> readonly bits.
> >
> > One other interesting feature that pci has is migration
> > sanity checks: a bit can be marked as immutable
> > which means that its value must be consistent on
> > migration source and destination.
> > This is often the case for read-only bits - but not always,
> > as bits could be set externally.
> >
> > Further, pci also supports a bit based allocator, so
> > if you need a range of bits for a capability you
> > can allocate them cleanly.
> 
> Maybe it would be useful to make these features available for other devices.
> 
> >
> >> >
> >> > Helper functions are then used to access the uint32_t which observe the
> >> > semantics defined by the UInt32StateInfo struct.
> >>
> >> We also need uint8_t, uint16_t and uint64_t versions for some devices.
> >> Perhaps it would be better to implement a uint64_t device which can be
> >> used with shorter widths or even stronger connection with memory API.
> >
> > Why not uint8_t for everyone?
> 
> That would be simple, but then modeling for example 32 bit registers
> gets clumsy.

The way we do this in pci is support wrappers for word/long accesses.
This is a nice way to do endian-ness conversion anyway, I guess.
If people are interested, it shouldn't be hard to generalize the pci code...

> The register model could provide unions for accessing
> different width entities though.

At least with PCI, guest can perform a long access and host word access
to the same register, so width is not a register property.

> >
> >> >
> >> > 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 sticky (nw0 and nw1)
> >> > Reset values can be defined (reset)
> >> > Bits can be marked to throw guest errors when written certain values (ge0, ge1)
> >>
> >> Other bits could be marked as unimplemented (LOG_UNIMP).
> >>
> >> > Bits can be marked clear on read (cor)
> >> > Regsiters can be truncated in width (width)
> >>
> >> s/Regsiters/Registers/
> >>
> >> >
> >> > 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.
> >>
> >> For maximum flexibility, a callback could be specified but then we
> >> overlap memory API.
> >>
> >> Once we have Pin available, it could be useful to couple a register
> >> bit directly with a Pin. Currently we could use qemu_irq. This would
> >> mean that the dynamic state would need to be more complex than just
> >> uint32_t. Also Pin could implement some of this functionality.
> >>
> >> >
> >> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> > ---
> >> >
> >> >  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
> >> >  util/bitops.c         |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 130 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> >> > index affcc96..8ad0290 100644
> >> > --- a/include/qemu/bitops.h
> >> > +++ b/include/qemu/bitops.h
> >>
> >> I think this is not the right place since this is very much HW
> >> specific, please introduce a new file.
> >>
> >> > @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
> >> >      return (value & ~mask) | ((fieldval << start) & mask);
> >> >  }
> >> >
> >> > +/**
> >> > + * A descriptor for a Uint32 that is part of guest accessible device state
> >> > + * @ro: whether or not the bit is read-only state comming out of reset
> >>
> >> s/comming/coming/
> >>
> >> > + * @w1c: bits with the common write 1 to clear semantic.
> >> > + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)
> >>
> >> s/cant/can't/, also below
> >>
> >> > + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
> >> > + * @reset: reset value.
> >> > + * @ge1: Bits that when written 1 indicate a guest error
> >> > + * @ge0: Bits that when written 0 indicate a guest error
> >> > + * @cor: Bits that are clear on read
> >> > + * @width: width of the uint32t. Only the @width least significant bits are
> >> > + * valid. All others are silent Read-as-reset/WI.
> >> > + */
> >> > +
> >> > +typedef struct UInt32StateInfo {
> >> > +    const char *name;
> >> > +    uint32_t ro;
> >> > +    uint32_t w1c;
> >> > +    uint32_t nw0;
> >> > +    uint32_t nw1;
> >> > +    uint32_t reset;
> >> > +    uint32_t ge1;
> >> > +    uint32_t ge0;
> >> > +    uint32_t cor;
> >> > +    int width;
> >> > +} UInt32StateInfo;
> >> > +
> >> > +/**
> >> > + * reset an array of u32s
> >> > + * @array: array of u32s to reset
> >> > + * @info: corresponding array of UInt32StateInfos to get reset values from
> >> > + * @num: number of values to reset
> >> > + */
> >> > +
> >> > +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int num);
> >> > +
> >> > +/**
> >> > + * write a value to a uint32_t subject to its restrictions
> >> > + * @state: Pointer to location to be written
> >> > + * @info: Definition of variable
> >> > + * @val: value to write
> >> > + * @prefix: Debug and error message prefix
> >> > + * @debug: Turn on noisy debug printfery
> >> > + */
> >> > +
> >> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> >> > +                  const char *prefix, bool debug);
> >>
> >> Prefix could be part of the structure. I'd also combine state and
> >> info, and avoid passing debug flag directly (it could be in the
> >> dynamic structure or a pointer). Then it should be easy to be
> >> compatible with memory API.
> >>
> >> > +
> >> > +/**
> >> > + * write a value from a uint32_t subject to its restrictions
> >>
> >> read
> >>
> >> > + * @state: Pointer to location to be read
> >> > + * @info: Definition of variable
> >> > + * @prefix: Debug and error message prefix
> >> > + * @debug: Turn on noisy debug printfery
> >> > + */
> >> > +
> >> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> >> > +                     const char *prefix, bool debug);
> >> > +
> >> >  #endif
> >> > diff --git a/util/bitops.c b/util/bitops.c
> >> > index e72237a..51db476 100644
> >> > --- a/util/bitops.c
> >> > +++ b/util/bitops.c
> >> > @@ -11,6 +11,8 @@
> >> >   * 2 of the License, or (at your option) any later version.
> >> >   */
> >> >
> >> > +#include "qemu-common.h"
> >> > +#include "qemu/log.h"
> >> >  #include "qemu/bitops.h"
> >> >
> >> >  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
> >> > @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
> >> >      /* Not found */
> >> >      return size;
> >> >  }
> >> > +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int num)
> >> > +{
> >> > +    int i = 0;
> >> > +
> >> > +    for (i = 0; i < num; ++i) {
> >> > +        state[i] = info[i].reset;
> >> > +    }
> >> > +}
> >>
> >> Perhaps this could be automatically registered.
> >>
> >> > +
> >> > +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> >> > +                  const char *prefix, bool debug)
> >> > +{
> >> > +    int i;
> >> > +    uint32_t new_val;
> >> > +    int width = info->width ? info->width : 32;
> >> > +
> >> > +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
> >> > +                        ~((1ull << width) - 1);
> >> > +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
> >> > +                        ~((1ull << width) - 1);
> >> > +
> >> > +    if (!info->name) {
> >> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> >> > +                      "(written value: %#08x)\n", prefix, val);
> >> > +        return;
> >> > +    }
> >> > +
> >> > +    if (debug) {
> >> > +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
> >> > +                val);
> >> > +    }
> >> > +
> >> > +    /*FIXME: skip over if no LOG_GUEST_ERROR */
> >> > +    for (i = 0; i <= 1; ++i) {
> >> > +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
> >> > +        if (test) {
> >> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be written"
> >> > +                          " to %d\n", prefix, info->name, test, i);
> >> > +        }
> >> > +    }
> >> > +
> >> > +    new_val = val & ~(no_w1_mask & val);
> >> > +    new_val |= no_w1_mask & *state & val;
> >> > +    new_val |= no_w0_mask & *state & ~val;
> >> > +    new_val &= ~(val & info->w1c);
> >> > +    *state = new_val;
> >> > +}
> >> > +
> >> > +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> >> > +                     const char *prefix, bool debug)
> >> > +{
> >> > +    uint32_t ret = *state;
> >> > +
> >> > +    /* clear on read */
> >> > +    ret &= ~info->cor;
> >> > +    *state = ret;
> >> > +
> >> > +    if (!info->name) {
> >> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state "
> >> > +                      "(read value: %#08x)\n", prefix, ret);
> >> > +        return ret;
> >> > +    }
> >> > +
> >> > +    if (debug) {
> >> > +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, ret);
> >> > +    }
> >> > +
> >> > +    return ret;
> >> > +}
> >> > --
> >> > 1.7.0.4
> >> >
> >> >

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Gerd Hoffmann]

  Hi,

>>>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
>>>> Perhaps it would be better to implement a uint64_t device which can be
>>>> used with shorter widths or even stronger connection with memory API.
>>>
>>> Why not uint8_t for everyone?
>>
>> That would be simple, but then modeling for example 32 bit registers
>> gets clumsy.
> 
> The way we do this in pci is support wrappers for word/long accesses.
> This is a nice way to do endian-ness conversion anyway, I guess.
> If people are interested, it shouldn't be hard to generalize the pci code...

> At least with PCI, guest can perform a long access and host word access
> to the same register, so width is not a register property.

Thanks, but I'm not interested.

Memory API handles this just fine for me, and there is zero reason to
care about how the guests accesses the registers (unless the hardware
you are emulating behaves strange enough that you have to care to get it
right).

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Michael S. Tsirkin]

On Tue, Mar 05, 2013 at 03:17:13PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
> >>>> Perhaps it would be better to implement a uint64_t device which can be
> >>>> used with shorter widths or even stronger connection with memory API.
> >>>
> >>> Why not uint8_t for everyone?
> >>
> >> That would be simple, but then modeling for example 32 bit registers
> >> gets clumsy.
> > 
> > The way we do this in pci is support wrappers for word/long accesses.
> > This is a nice way to do endian-ness conversion anyway, I guess.
> > If people are interested, it shouldn't be hard to generalize the pci code...
> 
> > At least with PCI, guest can perform a long access and host word access
> > to the same register, so width is not a register property.
> 
> Thanks, but I'm not interested.
> 
> Memory API handles this just fine for me, and there is zero reason to
> care about how the guests accesses the registers (unless the hardware
> you are emulating behaves strange enough that you have to care to get it
> right).
> 
> cheers,
>   Gerd

If the intended audience uses the memory API, then it's not needed.
pci config is not going through a memory API ATM though
I think it might be possible to make it do this by creating a separate
config address space per device.

-- 
MST

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Crosthwaite]

Hi All,

I have brought myself up to speed with this PCI stuff. First of all I
am assuming you are talking about the PCI Config space only? This
(hw/pci.h):

struct PCIDevice {
    DeviceState qdev;

    /* PCI config space */
    uint8_t *config;

    [snip]
    /* Used to implement RW1C(Write 1 to Clear) bytes */
    uint8_t *w1cmask;

    /* Used to allocate config space for capabilities. */
    uint8_t *used;

On Wed, Mar 6, 2013 at 12:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Mar 05, 2013 at 03:17:13PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>> >>>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
>> >>>> Perhaps it would be better to implement a uint64_t device which can be
>> >>>> used with shorter widths or even stronger connection with memory API.
>> >>>
>> >>> Why not uint8_t for everyone?
>> >>
>> >> That would be simple, but then modeling for example 32 bit registers
>> >> gets clumsy.
>> >
>> > The way we do this in pci is support wrappers for word/long accesses.
>> > This is a nice way to do endian-ness conversion anyway, I guess.
>> > If people are interested, it shouldn't be hard to generalize the pci code...

So the issue I have with the uint8_t based defintion + accessor is you
need to use code driven to set all the properties (w1cmask, used and
friends). eg (hw/pci/shpc.c):

    pci_set_byte(shpc->wmask + SHPC_CMD_CODE, 0xff);
    pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX);
    pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX);


I'm trying to break that, with table driven instantiation of this
information. Check patch 3 of this series for the guinea pig example
that does this, short except below:

+static const UInt32StateInfo xilinx_devcfg_regs_info[] = {
+    [R_CTRL] = { .name = "CTRL",
+        .reset = PCAP_PR | PCAP_MODE | 0x3 << 13,
+        .ro = 0x107f6000 | USER_MODE,
+        .ge0 = 0x3 << 13,
+    },
+    [R_LOCK] = { .name = "LOCK", .width = 5, .nw0 = ~0 },
+    [R_CFG] = { .name = "CFG",
+        .width = 12,
+        .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
+        .ge1 = 0x7,
+        .ge0 = 0x8,
+        .ro = 0x00f,
+    },

This philosophy also breaks the parallel array approach adopted by
PCI, so to share with PCI that will require some major earthworks PCI
side.

>>
>> > At least with PCI, guest can perform a long access and host word access
>> > to the same register, so width is not a register property.
>>

Guest access is the easiest part as you can use wrappers/accessors
that translate memory-API to uint8_t. But for hardware side access its
nice to be able to bang on the raw uint32_t[]. E.G. (from next patch):

+        DB_PRINT("dma operation finished\n");
+        s->regs[R_INT_STS] |= DMA_DONE_INT | DMA_P_DONE_INT;


This code raises the interrupt from the hardware side by setting a
register bit directly. It would be tedious (and an enourmous tree-wide
change pattern) to have the change this over to use a wrapping
accessor. My goal here is that only guest side access are regulated by
this API - hardware has free reign.

>> Thanks, but I'm not interested.
>>
>> Memory API handles this just fine for me, and there is zero reason to
>> care about how the guests accesses the registers (unless the hardware
>> you are emulating behaves strange enough that you have to care to get it
>> right).
>>

So in V2 I want to use something close to the memory API in interface,
so there is only trivial glue on the device level between the memory
API callback and this API.

>> cheers,
>>   Gerd
>
> If the intended audience uses the memory API, then it's not needed.

Not 100% accurate. My goal here it to control (or wrap) only guest
accesses, in the first instance via the Memory API, but other forms of
guest access are perfectly valid as well, and PCI config space, would
be a good example. If we are going to share code however, we will need
to make changes PCI side, as that uint8_t with compulsory accessors is
too verbose to be used in devices.

I have a solution that may work for everyone however. Leave the
storage format (uint32_t, uint8_t etc) up to the client. In PCI config
this is the uint8_t *config as it stands today. In my device its
uint32_t *regs. The client then casts the accessed area to uint8_t and
is responsible for defining the endianness policy via the accompanying
Uint32StateInfo struct. For PCI, this is PCIs endianness. For my
device this is just the endianness of the host machine, as thats how
the hardware side accesses (eg s->regs[R_INT_STS] |= DMA_DONE_INT;)
will operate.

Might be easier to describe as patches rather than talk though.

Regards,
Peter

> pci config is not going through a memory API ATM though
> I think it might be possible to make it do this by creating a separate
> config address space per device.
>
> --
> MST
>

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Peter Maydell]

On 7 March 2013 10:00, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Not 100% accurate. My goal here it to control (or wrap) only guest
> accesses, in the first instance via the Memory API, but other forms of
> guest access are perfectly valid as well, and PCI config space, would
> be a good example. If we are going to share code however, we will need
> to make changes PCI side, as that uint8_t with compulsory accessors is
> too verbose to be used in devices.

We should just use the memory API. If other bits of QEMU like PCI
don't care to update their interfaces to be memory API then they
don't get to use your new features. I don't see any point in defining
a new interface at a level below the memory API for their benefit.

-- PMM

Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Blue Swirl]

On Thu, Mar 7, 2013 at 2:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 March 2013 10:00, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Not 100% accurate. My goal here it to control (or wrap) only guest
>> accesses, in the first instance via the Memory API, but other forms of
>> guest access are perfectly valid as well, and PCI config space, would
>> be a good example. If we are going to share code however, we will need
>> to make changes PCI side, as that uint8_t with compulsory accessors is
>> too verbose to be used in devices.
>
> We should just use the memory API. If other bits of QEMU like PCI
> don't care to update their interfaces to be memory API then they
> don't get to use your new features. I don't see any point in defining
> a new interface at a level below the memory API for their benefit.

I'm also leaning towards memory API approach, it should be better in
the long term since all devices should use that sooner or later.

>
> -- PMM