[Qemu-devel] [PATCH v2 0/8] Add virtio-mmio and use it in vexpress

[Qemu-devel] [PATCH v2 0/8] Add virtio-mmio and use it in vexpress

[Peter Maydell]

This patch series adds an implementation of the virtio-mmio
transport, and uses it in the vexpress-a9 and vexpress-a15
board models.

The basic idea is that the board instantiates some transports,
the user can create backends which automatically plug into them
(via -device virtio-blk-backend and the like), and we tell the
guest kernel about them using a new arm/boot hook that lets
the board modify the user's device tree blob.

Changes v1->v2:
 * move the 'is guest provided queue size too large?' check
   from virtio-mmio into the core virtio 'set_num' function
 * rebase (including adding owner parameter to memory region
   init call in virtio-mmio.c)


Peter Maydell (8):
  device_tree: Add qemu_devtree_setprop_sized_cells() utility functions
  arm/boot: Use qemu_devtree_setprop_sized_cells()
  virtio: Add support for guest setting of queue size
  virtio: Support transports which can specify the vring alignment
  virtio: Implement MMIO based virtio transport
  arm/boot: Allow boards to modify the FDT blob
  vexpress: Make VEDBoardInfo extend arm_boot_info
  vexpress: Add virtio-mmio transports

 device_tree.c                  |   31 +++
 hw/arm/boot.c                  |   33 ++--
 hw/arm/vexpress.c              |  128 ++++++++++--
 hw/virtio/Makefile.objs        |    1 +
 hw/virtio/virtio-mmio.c        |  421 ++++++++++++++++++++++++++++++++++++++++
 hw/virtio/virtio.c             |   40 +++-
 include/hw/arm/arm.h           |    4 +
 include/hw/virtio/virtio-bus.h |    6 +
 include/hw/virtio/virtio.h     |    2 +
 include/sysemu/device_tree.h   |   62 ++++++
 10 files changed, 689 insertions(+), 39 deletions(-)
 create mode 100644 hw/virtio/virtio-mmio.c

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Maydell]

We already have a qemu_devtree_setprop_cells() which sets a dtb
property to an array of cells whose values are specified by varargs.
However for the fairly common case of setting a property to a list
of addresses or of address,size pairs the number of cells used by
each element in the list depends on the parent's #address-cells
and #size-cells properties. To make this easier we provide an analogous
qemu_devtree_setprop_sized_cells() macro which allows the number
of cells used by each element to be specified. This is implemented
using an underlying qemu_devtree_setprop_sized_cells_from_array()
function which takes the values and sizes as an array; this may
also be directly useful for cases where the cell contents are
constructed programmatically.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 device_tree.c                |   31 +++++++++++++++++++++
 include/sysemu/device_tree.h |   62 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 10cf3d0..308205c 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -308,3 +308,34 @@ void qemu_devtree_dumpdtb(void *fdt, int size)
         exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1);
     }
 }
+
+int qemu_devtree_setprop_sized_cells_from_array(void *fdt,
+                                                const char *node_path,
+                                                const char *property,
+                                                int numvalues,
+                                                uint64_t *values)
+{
+    uint32_t *propcells;
+    uint64_t value;
+    int cellnum, vnum, ncells;
+    uint32_t hival;
+
+    propcells = g_new0(uint32_t, numvalues * 2);
+
+    cellnum = 0;
+    for (vnum = 0; vnum < numvalues; vnum++) {
+        ncells = values[vnum * 2];
+        assert(ncells == 1 || ncells == 2);
+        value = values[vnum * 2 + 1];
+        hival = cpu_to_be32(value >> 32);
+        if (ncells > 1) {
+            propcells[cellnum++] = hival;
+        } else if (hival != 0) {
+            return vnum + 1;
+        }
+        propcells[cellnum++] = cpu_to_be32(value);
+    }
+
+    return qemu_devtree_setprop(fdt, node_path, property, propcells,
+                                cellnum * sizeof(uint32_t));
+}
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index f0b3f35..8189ebc 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -51,4 +51,66 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
 
 void qemu_devtree_dumpdtb(void *fdt, int size);
 
+/**
+ * qemu_devtree_setprop_sized_cells_from_array:
+ * @fdt: device tree blob
+ * @node_path: node to set property on
+ * @property: property to set
+ * @numvalues: number of values
+ * @values: array of number-of-cells, value pairs
+ *
+ * Set the specified property on the specified node in the device tree
+ * to be an array of cells. The values of the cells are specified via
+ * the values list, which alternates between "number of cells used by
+ * this value" and "value".
+ * number-of-cells must be either 1 or 2 (other values will assert()).
+ *
+ * This function is useful because device tree nodes often have cell arrays
+ * which are either lists of addresses or lists of address,size tuples, but
+ * the number of cells used for each element vary depending on the
+ * #address-cells and #size-cells properties of their parent node.
+ * If you know all your cell elements are one cell wide you can use the
+ * simpler qemu_devtree_setprop_cells(). If you're not setting up the
+ * array programmatically, qemu_devtree_setprop_sized_cells may be more
+ * convenient.
+ *
+ * Return value: 0 on success, <0 if setting the property failed,
+ * n (for n>0) if value n wouldn't fit in the required number of cells.
+ * (This slightly odd convention is for the benefit of callers who might
+ * wish to print different error messages depending on which value
+ * was too large to fit, since values might be user-specified.)
+ */
+int qemu_devtree_setprop_sized_cells_from_array(void *fdt,
+                                                const char *node_path,
+                                                const char *property,
+                                                int numvalues,
+                                                uint64_t *values);
+
+/**
+ * qemu_devtree_setprop_sized_cells:
+ * @fdt: device tree blob
+ * @node_path: node to set property on
+ * @property: property to set
+ * @...: list of number-of-cells, value pairs
+ *
+ * Set the specified property on the specified node in the device tree
+ * to be an array of cells. The values of the cells are specified via
+ * the variable arguments, which alternates between "number of cells
+ * used by this value" and "value".
+ *
+ * This is a convenience wrapper for the function
+ * qemu_devtree_setprop_sized_cells_from_array().
+ *
+ * Return value: 0 on success, <0 if setting the property failed,
+ * n (for n>0) if value n wouldn't fit in the required number of cells.
+ */
+#define qemu_devtree_setprop_sized_cells(fdt, node_path, property, ...)       \
+    ({                                                                        \
+        uint64_t qdt_tmp[] = { __VA_ARGS__ };                                 \
+        qemu_devtree_setprop_sized_cells_from_array(fdt, node_path,           \
+                                                    property,                 \
+                                                    ARRAY_SIZE(qdt_tmp) / 2,  \
+                                                    qdt_tmp);                 \
+    })
+
 #endif /* __DEVICE_TREE_H__ */
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 2/8] arm/boot: Use qemu_devtree_setprop_sized_cells()

[Peter Maydell]

Replace the opencoded assembly of the reg property array for the
/memory node with a call to qemu_devtree_setprop_sized_cells().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c |   28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index a2e4032..1166632 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -227,12 +227,10 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 
 static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 {
-    uint32_t *mem_reg_property;
-    uint32_t mem_reg_propsize;
     void *fdt = NULL;
     char *filename;
     int size, rc;
-    uint32_t acells, scells, hival;
+    uint32_t acells, scells;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
     if (!filename) {
@@ -255,30 +253,18 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
         goto fail;
     }
 
-    mem_reg_propsize = acells + scells;
-    mem_reg_property = g_new0(uint32_t, mem_reg_propsize);
-    mem_reg_property[acells - 1] = cpu_to_be32(binfo->loader_start);
-    hival = cpu_to_be32(binfo->loader_start >> 32);
-    if (acells > 1) {
-        mem_reg_property[acells - 2] = hival;
-    } else if (hival != 0) {
+    rc = qemu_devtree_setprop_sized_cells(fdt, "/memory", "reg",
+                                          acells, binfo->loader_start,
+                                          scells, binfo->ram_size);
+    if (rc == 1) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM start address > 4GB\n");
         goto fail;
-    }
-    mem_reg_property[acells + scells - 1] = cpu_to_be32(binfo->ram_size);
-    hival = cpu_to_be32(binfo->ram_size >> 32);
-    if (scells > 1) {
-        mem_reg_property[acells + scells - 2] = hival;
-    } else if (hival != 0) {
+    } else if (rc == 2) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM size > 4GB\n");
         goto fail;
-    }
-
-    rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
-                              mem_reg_propsize * sizeof(uint32_t));
-    if (rc < 0) {
+    } else if (rc != 0) {
         fprintf(stderr, "couldn't set /memory/reg\n");
         goto fail;
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Peter Maydell]

The MMIO virtio transport spec allows the guest to tell the host how
large the queue size is. Add virtio_queue_set_num() function which
implements this in the QEMU common virtio support code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio.c         |    8 ++++++++
 include/hw/virtio/virtio.h |    1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8176c14..01b05f3 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -667,6 +667,14 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
     return vdev->vq[n].pa;
 }
 
+void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
+{
+    if (num <= VIRTQUEUE_MAX_SIZE) {
+        vdev->vq[n].vring.num = num;
+        virtqueue_init(&vdev->vq[n]);
+    }
+}
+
 int virtio_queue_get_num(VirtIODevice *vdev, int n)
 {
     return vdev->vq[n].vring.num;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a6c5c53..95c4772 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -198,6 +198,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
+void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 4/8] virtio: Support transports which can specify the vring alignment

[Peter Maydell]

Support virtio transports which can specify the vring alignment
(ie where the guest communicates this to the host) by providing
a new virtio_queue_set_align() function. (The default alignment
remains as before.)

Transports which wish to make use of this must set the
has_variable_vring_alignment field in their VirtioBusClass
struct to true; they can then change the alignment via
virtio_queue_set_align().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio.c             |   32 +++++++++++++++++++++++++++++---
 include/hw/virtio/virtio-bus.h |    6 ++++++
 include/hw/virtio/virtio.h     |    1 +
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 01b05f3..09f62c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,8 +19,11 @@
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
 
-/* The alignment to use between consumer and producer parts of vring.
- * x86 pagesize again. */
+/*
+ * The alignment to use between consumer and producer parts of vring.
+ * x86 pagesize again. This is the default, used by transports like PCI
+ * which don't provide a means for the guest to tell the host the alignment.
+ */
 #define VIRTIO_PCI_VRING_ALIGN         4096
 
 typedef struct VRingDesc
@@ -54,6 +57,7 @@ typedef struct VRingUsed
 typedef struct VRing
 {
     unsigned int num;
+    unsigned int align;
     hwaddr desc;
     hwaddr avail;
     hwaddr used;
@@ -93,7 +97,7 @@ static void virtqueue_init(VirtQueue *vq)
     vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc);
     vq->vring.used = vring_align(vq->vring.avail +
                                  offsetof(VRingAvail, ring[vq->vring.num]),
-                                 VIRTIO_PCI_VRING_ALIGN);
+                                 vq->vring.align);
 }
 
 static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
@@ -687,6 +691,21 @@ int virtio_queue_get_id(VirtQueue *vq)
     return vq - &vdev->vq[0];
 }
 
+void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    /* Check that the transport told us it was going to do this
+     * (so a buggy transport will immediately assert rather than
+     * silently failing to migrate this state)
+     */
+    assert(k->has_variable_vring_alignment);
+
+    vdev->vq[n].vring.align = align;
+    virtqueue_init(&vdev->vq[n]);
+}
+
 void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc) {
@@ -727,6 +746,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
         abort();
 
     vdev->vq[i].vring.num = queue_size;
+    vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
 
     return &vdev->vq[i];
@@ -833,6 +853,9 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
             break;
 
         qemu_put_be32(f, vdev->vq[i].vring.num);
+        if (k->has_variable_vring_alignment) {
+            qemu_put_be32(f, vdev->vq[i].vring.align);
+        }
         qemu_put_be64(f, vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
         if (k->save_queue) {
@@ -889,6 +912,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 
     for (i = 0; i < num; i++) {
         vdev->vq[i].vring.num = qemu_get_be32(f);
+        if (k->has_variable_vring_alignment) {
+            vdev->vq[i].vring.align = qemu_get_be32(f);
+        }
         vdev->vq[i].pa = qemu_get_be64(f);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
         vdev->vq[i].signalled_used_valid = false;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 9ed60f9..9217f85 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -62,6 +62,12 @@ typedef struct VirtioBusClass {
      * This is called by virtio-bus just before the device is unplugged.
      */
     void (*device_unplug)(DeviceState *d);
+    /*
+     * Does the transport have variable vring alignment?
+     * (ie can it ever call virtio_queue_set_align()?)
+     * Note that changing this will break migration for this transport.
+     */
+    bool has_variable_vring_alignment;
 } VirtioBusClass;
 
 struct VirtioBusState {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 95c4772..6c2cee6 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -200,6 +200,7 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
+void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 5/8] virtio: Implement MMIO based virtio transport

[Peter Maydell]

Add support for the generic MMIO based virtio transport.

This patch includes some fixes for bugs spotted by
Ying-Shiuan Pan <yspan@itri.org.tw>.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
[Fred changes: updated to new virtio-bus mechanisms]
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
[PMM changes:
 * fixed trivial makefile conflict
 * removed unused int_enable
 * host_features doesn't need migrating
 * reset guest accessible state in the reset function
 * minor style fixes like extra blank lines
 * RAZ/WI if there's no backend
 * made transport size 0x200, in line with kvmtool
 * set has_variable_vring_alignment
]
---
 hw/virtio/Makefile.objs |    1 +
 hw/virtio/virtio-mmio.c |  421 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 422 insertions(+)
 create mode 100644 hw/virtio/virtio-mmio.c

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index cbe6d51..1ba53d9 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -1,6 +1,7 @@
 common-obj-y += virtio-rng.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-y += virtio-bus.o
+common-obj-y += virtio-mmio.o
 common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
 
 obj-y += virtio.o virtio-balloon.o 
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
new file mode 100644
index 0000000..54d6679
--- /dev/null
+++ b/hw/virtio/virtio-mmio.c
@@ -0,0 +1,421 @@
+/*
+ * Virtio MMIO bindings
+ *
+ * Copyright (c) 2011 Linaro Limited
+ *
+ * Author:
+ *  Peter Maydell <peter.maydell@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/virtio/virtio.h"
+#include "qemu/host-utils.h"
+#include "hw/virtio/virtio-bus.h"
+
+/* #define DEBUG_VIRTIO_MMIO */
+
+#ifdef DEBUG_VIRTIO_MMIO
+
+#define DPRINTF(fmt, ...) \
+do { printf("virtio_mmio: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+/* QOM macros */
+/* virtio-mmio-bus */
+#define TYPE_VIRTIO_MMIO_BUS "virtio-mmio-bus"
+#define VIRTIO_MMIO_BUS(obj) \
+        OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_MMIO_BUS)
+#define VIRTIO_MMIO_BUS_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioBusClass, (obj), TYPE_VIRTIO_MMIO_BUS)
+#define VIRTIO_MMIO_BUS_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioBusClass, (klass), TYPE_VIRTIO_MMIO_BUS)
+
+/* virtio-mmio */
+#define TYPE_VIRTIO_MMIO "virtio-mmio"
+#define VIRTIO_MMIO(obj) \
+        OBJECT_CHECK(VirtIOMMIOProxy, (obj), TYPE_VIRTIO_MMIO)
+
+/* Memory mapped register offsets */
+#define VIRTIO_MMIO_MAGIC 0x0
+#define VIRTIO_MMIO_VERSION 0x4
+#define VIRTIO_MMIO_DEVICEID 0x8
+#define VIRTIO_MMIO_VENDORID 0xc
+#define VIRTIO_MMIO_HOSTFEATURES 0x10
+#define VIRTIO_MMIO_HOSTFEATURESSEL 0x14
+#define VIRTIO_MMIO_GUESTFEATURES 0x20
+#define VIRTIO_MMIO_GUESTFEATURESSEL 0x24
+#define VIRTIO_MMIO_GUESTPAGESIZE 0x28
+#define VIRTIO_MMIO_QUEUESEL 0x30
+#define VIRTIO_MMIO_QUEUENUMMAX 0x34
+#define VIRTIO_MMIO_QUEUENUM 0x38
+#define VIRTIO_MMIO_QUEUEALIGN 0x3c
+#define VIRTIO_MMIO_QUEUEPFN 0x40
+#define VIRTIO_MMIO_QUEUENOTIFY 0x50
+#define VIRTIO_MMIO_INTERRUPTSTATUS 0x60
+#define VIRTIO_MMIO_INTERRUPTACK 0x64
+#define VIRTIO_MMIO_STATUS 0x70
+/* Device specific config space starts here */
+#define VIRTIO_MMIO_CONFIG 0x100
+
+#define VIRT_MAGIC 0x74726976 /* 'virt' */
+#define VIRT_VERSION 1
+#define VIRT_VENDOR 0x554D4551 /* 'QEMU' */
+
+typedef struct {
+    /* Generic */
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+    qemu_irq irq;
+    uint32_t host_features;
+    /* Guest accessible state needing migration and reset */
+    uint32_t host_features_sel;
+    uint32_t guest_features_sel;
+    uint32_t guest_page_shift;
+    /* virtio-bus */
+    VirtioBusState bus;
+} VirtIOMMIOProxy;
+
+static void virtio_mmio_bus_new(VirtioBusState *bus, VirtIOMMIOProxy *dev);
+
+static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
+{
+    VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
+    VirtIODevice *vdev = proxy->bus.vdev;
+
+    DPRINTF("virtio_mmio_read offset 0x%x\n", (int)offset);
+
+    if (!vdev) {
+        /* If no backend is present, we treat most registers as
+         * read-as-zero, except for the magic number, version and
+         * vendor ID. This is not strictly sanctioned by the virtio
+         * spec, but it allows us to provide transports with no backend
+         * plugged in which don't confuse Linux's virtio code: the
+         * probe won't complain about the bad magic number, but the
+         * device ID of zero means no backend will claim it.
+         */
+        switch (offset) {
+        case VIRTIO_MMIO_MAGIC:
+            return VIRT_MAGIC;
+        case VIRTIO_MMIO_VERSION:
+            return VIRT_VERSION;
+        case VIRTIO_MMIO_VENDORID:
+            return VIRT_VENDOR;
+        default:
+            return 0;
+        }
+    }
+
+    if (offset >= VIRTIO_MMIO_CONFIG) {
+        offset -= VIRTIO_MMIO_CONFIG;
+        switch (size) {
+        case 1:
+            return virtio_config_readb(vdev, offset);
+        case 2:
+            return virtio_config_readw(vdev, offset);
+        case 4:
+            return virtio_config_readl(vdev, offset);
+        default:
+            abort();
+        }
+    }
+    if (size != 4) {
+        DPRINTF("wrong size access to register!\n");
+        return 0;
+    }
+    switch (offset) {
+    case VIRTIO_MMIO_MAGIC:
+        return VIRT_MAGIC;
+    case VIRTIO_MMIO_VERSION:
+        return VIRT_VERSION;
+    case VIRTIO_MMIO_DEVICEID:
+        return vdev->device_id;
+    case VIRTIO_MMIO_VENDORID:
+        return VIRT_VENDOR;
+    case VIRTIO_MMIO_HOSTFEATURES:
+        if (proxy->host_features_sel) {
+            return 0;
+        }
+        return proxy->host_features;
+    case VIRTIO_MMIO_QUEUENUMMAX:
+        return VIRTQUEUE_MAX_SIZE;
+    case VIRTIO_MMIO_QUEUEPFN:
+        return virtio_queue_get_addr(vdev, vdev->queue_sel)
+            >> proxy->guest_page_shift;
+    case VIRTIO_MMIO_INTERRUPTSTATUS:
+        return vdev->isr;
+    case VIRTIO_MMIO_STATUS:
+        return vdev->status;
+    case VIRTIO_MMIO_HOSTFEATURESSEL:
+    case VIRTIO_MMIO_GUESTFEATURES:
+    case VIRTIO_MMIO_GUESTFEATURESSEL:
+    case VIRTIO_MMIO_GUESTPAGESIZE:
+    case VIRTIO_MMIO_QUEUESEL:
+    case VIRTIO_MMIO_QUEUENUM:
+    case VIRTIO_MMIO_QUEUEALIGN:
+    case VIRTIO_MMIO_QUEUENOTIFY:
+    case VIRTIO_MMIO_INTERRUPTACK:
+        DPRINTF("read of write-only register\n");
+        return 0;
+    default:
+        DPRINTF("bad register offset\n");
+        return 0;
+    }
+    return 0;
+}
+
+static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
+                              unsigned size)
+{
+    VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
+    VirtIODevice *vdev = proxy->bus.vdev;
+
+    DPRINTF("virtio_mmio_write offset 0x%x value 0x%" PRIx64 "\n",
+            (int)offset, value);
+
+    if (!vdev) {
+        /* If no backend is present, we just make all registers
+         * write-ignored. This allows us to provide transports with
+         * no backend plugged in.
+         */
+        return;
+    }
+
+    if (offset >= VIRTIO_MMIO_CONFIG) {
+        offset -= VIRTIO_MMIO_CONFIG;
+        switch (size) {
+        case 1:
+            virtio_config_writeb(vdev, offset, value);
+            break;
+        case 2:
+            virtio_config_writew(vdev, offset, value);
+            break;
+        case 4:
+            virtio_config_writel(vdev, offset, value);
+            break;
+        default:
+            abort();
+        }
+        return;
+    }
+    if (size != 4) {
+        DPRINTF("wrong size access to register!\n");
+        return;
+    }
+    switch (offset) {
+    case VIRTIO_MMIO_HOSTFEATURESSEL:
+        proxy->host_features_sel = value;
+        break;
+    case VIRTIO_MMIO_GUESTFEATURES:
+        if (!proxy->guest_features_sel) {
+            virtio_set_features(vdev, value);
+        }
+        break;
+    case VIRTIO_MMIO_GUESTFEATURESSEL:
+        proxy->guest_features_sel = value;
+        break;
+    case VIRTIO_MMIO_GUESTPAGESIZE:
+        proxy->guest_page_shift = ctz32(value);
+        if (proxy->guest_page_shift > 31) {
+            proxy->guest_page_shift = 0;
+        }
+        DPRINTF("guest page size %" PRIx64 " shift %d\n", value,
+                proxy->guest_page_shift);
+        break;
+    case VIRTIO_MMIO_QUEUESEL:
+        if (value < VIRTIO_PCI_QUEUE_MAX) {
+            vdev->queue_sel = value;
+        }
+        break;
+    case VIRTIO_MMIO_QUEUENUM:
+        DPRINTF("mmio_queue write %d max %d\n", (int)value, VIRTQUEUE_MAX_SIZE);
+        virtio_queue_set_num(vdev, vdev->queue_sel, value);
+        break;
+    case VIRTIO_MMIO_QUEUEALIGN:
+        virtio_queue_set_align(vdev, vdev->queue_sel, value);
+        break;
+    case VIRTIO_MMIO_QUEUEPFN:
+        if (value == 0) {
+            virtio_reset(vdev);
+        } else {
+            virtio_queue_set_addr(vdev, vdev->queue_sel,
+                                  value << proxy->guest_page_shift);
+        }
+        break;
+    case VIRTIO_MMIO_QUEUENOTIFY:
+        if (value < VIRTIO_PCI_QUEUE_MAX) {
+            virtio_queue_notify(vdev, value);
+        }
+        break;
+    case VIRTIO_MMIO_INTERRUPTACK:
+        vdev->isr &= ~value;
+        virtio_update_irq(vdev);
+        break;
+    case VIRTIO_MMIO_STATUS:
+        virtio_set_status(vdev, value & 0xff);
+        if (vdev->status == 0) {
+            virtio_reset(vdev);
+        }
+        break;
+    case VIRTIO_MMIO_MAGIC:
+    case VIRTIO_MMIO_VERSION:
+    case VIRTIO_MMIO_DEVICEID:
+    case VIRTIO_MMIO_VENDORID:
+    case VIRTIO_MMIO_HOSTFEATURES:
+    case VIRTIO_MMIO_QUEUENUMMAX:
+    case VIRTIO_MMIO_INTERRUPTSTATUS:
+        DPRINTF("write to readonly register\n");
+        break;
+
+    default:
+        DPRINTF("bad register offset\n");
+    }
+}
+
+static const MemoryRegionOps virtio_mem_ops = {
+    .read = virtio_mmio_read,
+    .write = virtio_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+    int level;
+
+    if (!proxy->bus.vdev) {
+        return;
+    }
+    level = (proxy->bus.vdev->isr != 0);
+    DPRINTF("virtio_mmio setting IRQ %d\n", level);
+    qemu_set_irq(proxy->irq, level);
+}
+
+static unsigned int virtio_mmio_get_features(DeviceState *opaque)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+
+    return proxy->host_features;
+}
+
+static int virtio_mmio_load_config(DeviceState *opaque, QEMUFile *f)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+
+    proxy->host_features_sel = qemu_get_be32(f);
+    proxy->guest_features_sel = qemu_get_be32(f);
+    proxy->guest_page_shift = qemu_get_be32(f);
+    return 0;
+}
+
+static void virtio_mmio_save_config(DeviceState *opaque, QEMUFile *f)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+
+    qemu_put_be32(f, proxy->host_features_sel);
+    qemu_put_be32(f, proxy->guest_features_sel);
+    qemu_put_be32(f, proxy->guest_page_shift);
+}
+
+static void virtio_mmio_reset(DeviceState *d)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
+
+    virtio_bus_reset(&proxy->bus);
+    proxy->host_features_sel = 0;
+    proxy->guest_features_sel = 0;
+    proxy->guest_page_shift = 0;
+}
+
+/* virtio-mmio device */
+
+/* This is called by virtio-bus just after the device is plugged. */
+static void virtio_mmio_device_plugged(DeviceState *opaque)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+
+    proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+    proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
+                                                        proxy->host_features);
+}
+
+static void virtio_mmio_realizefn(DeviceState *d, Error **errp)
+{
+    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(d);
+
+    virtio_mmio_bus_new(&proxy->bus, proxy);
+    sysbus_init_irq(sbd, &proxy->irq);
+    memory_region_init_io(&proxy->iomem, OBJECT(d), &virtio_mem_ops, proxy,
+                          TYPE_VIRTIO_MMIO, 0x200);
+    sysbus_init_mmio(sbd, &proxy->iomem);
+}
+
+static void virtio_mmio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = virtio_mmio_realizefn;
+    dc->reset = virtio_mmio_reset;
+}
+
+static const TypeInfo virtio_mmio_info = {
+    .name          = TYPE_VIRTIO_MMIO,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(VirtIOMMIOProxy),
+    .class_init    = virtio_mmio_class_init,
+};
+
+/* virtio-mmio-bus. */
+
+static void virtio_mmio_bus_new(VirtioBusState *bus, VirtIOMMIOProxy *dev)
+{
+    DeviceState *qdev = DEVICE(dev);
+    BusState *qbus;
+
+    qbus_create_inplace((BusState *)bus, TYPE_VIRTIO_MMIO_BUS, qdev, NULL);
+    qbus = BUS(bus);
+    qbus->allow_hotplug = 0;
+}
+
+static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *bus_class = BUS_CLASS(klass);
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+
+    k->notify = virtio_mmio_update_irq;
+    k->save_config = virtio_mmio_save_config;
+    k->load_config = virtio_mmio_load_config;
+    k->get_features = virtio_mmio_get_features;
+    k->device_plugged = virtio_mmio_device_plugged;
+    k->has_variable_vring_alignment = true;
+    bus_class->max_dev = 1;
+}
+
+static const TypeInfo virtio_mmio_bus_info = {
+    .name          = TYPE_VIRTIO_MMIO_BUS,
+    .parent        = TYPE_VIRTIO_BUS,
+    .instance_size = sizeof(VirtioBusState),
+    .class_init    = virtio_mmio_bus_class_init,
+};
+
+static void virtio_mmio_register_types(void)
+{
+    type_register_static(&virtio_mmio_bus_info);
+    type_register_static(&virtio_mmio_info);
+}
+
+type_init(virtio_mmio_register_types)
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 6/8] arm/boot: Allow boards to modify the FDT blob

[Peter Maydell]

Add a callback hook in arm_boot_info to allow board models to
modify the device tree blob if they need to. (The major expected
use case is to add virtio-mmio nodes for virtio-mmio transports
that exist in QEMU but not in the hardware.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c        |    5 +++++
 include/hw/arm/arm.h |    4 ++++
 2 files changed, 9 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1166632..72ae562 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -293,6 +293,11 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
             goto fail;
         }
     }
+
+    if (binfo->modify_dtb) {
+        binfo->modify_dtb(binfo, fdt);
+    }
+
     qemu_devtree_dumpdtb(fdt, size);
 
     cpu_physical_memory_write(addr, fdt, size);
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 7b2b02d..bae87c6 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -55,6 +55,10 @@ struct arm_boot_info {
                                  const struct arm_boot_info *info);
     void (*secondary_cpu_reset_hook)(ARMCPU *cpu,
                                      const struct arm_boot_info *info);
+    /* if a board needs to be able to modify a device tree provided by
+     * the user it should implement this hook.
+     */
+    void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
     /* Used internally by arm_boot.c */
     int is_linux;
     hwaddr initrd_start;
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 7/8] vexpress: Make VEDBoardInfo extend arm_boot_info

[Peter Maydell]

Make the VEDBoardInfo struct extend arm_boot_info; this will
allow us to get at the VEDBoardInfo information inside callbacks
from arm/boot code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/vexpress.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index fd18b60..d1c28aa 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -36,8 +36,6 @@
 #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
 #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
 
-static struct arm_boot_info vexpress_binfo;
-
 /* Address maps for peripherals:
  * the Versatile Express motherboard has two possible maps,
  * the "legacy" one (used for A9) and the "Cortex-A Series"
@@ -150,6 +148,7 @@ typedef void DBoardInitFn(const VEDBoardInfo *daughterboard,
                           qemu_irq *pic);
 
 struct VEDBoardInfo {
+    struct arm_boot_info bootinfo;
     const hwaddr *motherboard_map;
     hwaddr loader_start;
     const hwaddr gic_cpu_if_addr;
@@ -269,7 +268,7 @@ static const uint32_t a9_clocks[] = {
     66670000, /* Test chip reference clock: 66.67MHz */
 };
 
-static const VEDBoardInfo a9_daughterboard = {
+static VEDBoardInfo a9_daughterboard = {
     .motherboard_map = motherboard_legacy_map,
     .loader_start = 0x60000000,
     .gic_cpu_if_addr = 0x1e000100,
@@ -381,7 +380,7 @@ static const uint32_t a15_clocks[] = {
     40000000, /* OSCCLK8: 40MHz : DDR2 PLL reference */
 };
 
-static const VEDBoardInfo a15_daughterboard = {
+static VEDBoardInfo a15_daughterboard = {
     .motherboard_map = motherboard_aseries_map,
     .loader_start = 0x80000000,
     .gic_cpu_if_addr = 0x2c002000,
@@ -393,7 +392,7 @@ static const VEDBoardInfo a15_daughterboard = {
     .init = a15_daughterboard_init,
 };
 
-static void vexpress_common_init(const VEDBoardInfo *daughterboard,
+static void vexpress_common_init(VEDBoardInfo *daughterboard,
                                  QEMUMachineInitArgs *args)
 {
     DeviceState *dev, *sysctl, *pl041;
@@ -509,17 +508,17 @@ static void vexpress_common_init(const VEDBoardInfo *daughterboard,
 
     /* VE_DAPROM: not modelled */
 
-    vexpress_binfo.ram_size = args->ram_size;
-    vexpress_binfo.kernel_filename = args->kernel_filename;
-    vexpress_binfo.kernel_cmdline = args->kernel_cmdline;
-    vexpress_binfo.initrd_filename = args->initrd_filename;
-    vexpress_binfo.nb_cpus = smp_cpus;
-    vexpress_binfo.board_id = VEXPRESS_BOARD_ID;
-    vexpress_binfo.loader_start = daughterboard->loader_start;
-    vexpress_binfo.smp_loader_start = map[VE_SRAM];
-    vexpress_binfo.smp_bootreg_addr = map[VE_SYSREGS] + 0x30;
-    vexpress_binfo.gic_cpu_if_addr = daughterboard->gic_cpu_if_addr;
-    arm_load_kernel(ARM_CPU(first_cpu), &vexpress_binfo);
+    daughterboard->bootinfo.ram_size = args->ram_size;
+    daughterboard->bootinfo.kernel_filename = args->kernel_filename;
+    daughterboard->bootinfo.kernel_cmdline = args->kernel_cmdline;
+    daughterboard->bootinfo.initrd_filename = args->initrd_filename;
+    daughterboard->bootinfo.nb_cpus = smp_cpus;
+    daughterboard->bootinfo.board_id = VEXPRESS_BOARD_ID;
+    daughterboard->bootinfo.loader_start = daughterboard->loader_start;
+    daughterboard->bootinfo.smp_loader_start = map[VE_SRAM];
+    daughterboard->bootinfo.smp_bootreg_addr = map[VE_SYSREGS] + 0x30;
+    daughterboard->bootinfo.gic_cpu_if_addr = daughterboard->gic_cpu_if_addr;
+    arm_load_kernel(ARM_CPU(first_cpu), &daughterboard->bootinfo);
 }
 
 static void vexpress_a9_init(QEMUMachineInitArgs *args)
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 8/8] vexpress: Add virtio-mmio transports

[Peter Maydell]

Add some virtio-mmio transports to the vexpress board model,
together with a modify_dtb hook which adds them to the device
tree so that the kernel will probe for them. We put them
in a reserved area of the address map.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/vexpress.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index d1c28aa..dcc86b9 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -31,11 +31,18 @@
 #include "exec/address-spaces.h"
 #include "sysemu/blockdev.h"
 #include "hw/block/flash.h"
+#include "sysemu/device_tree.h"
+#include <libfdt.h>
 
 #define VEXPRESS_BOARD_ID 0x8e0
 #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
 #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
 
+/* Number of virtio transports to create (0..8; limited by
+ * number of available IRQ lines).
+ */
+#define NUM_VIRTIO_TRANSPORTS 4
+
 /* Address maps for peripherals:
  * the Versatile Express motherboard has two possible maps,
  * the "legacy" one (used for A9) and the "Cortex-A Series"
@@ -70,6 +77,7 @@ enum {
     VE_ETHERNET,
     VE_USB,
     VE_DAPROM,
+    VE_VIRTIO,
 };
 
 static hwaddr motherboard_legacy_map[] = {
@@ -88,6 +96,7 @@ static hwaddr motherboard_legacy_map[] = {
     [VE_WDT] = 0x1000f000,
     [VE_TIMER01] = 0x10011000,
     [VE_TIMER23] = 0x10012000,
+    [VE_VIRTIO] = 0x10013000,
     [VE_SERIALDVI] = 0x10016000,
     [VE_RTC] = 0x10017000,
     [VE_COMPACTFLASH] = 0x1001a000,
@@ -132,6 +141,7 @@ static hwaddr motherboard_aseries_map[] = {
     [VE_WDT] = 0x1c0f0000,
     [VE_TIMER01] = 0x1c110000,
     [VE_TIMER23] = 0x1c120000,
+    [VE_VIRTIO] = 0x1c130000,
     [VE_SERIALDVI] = 0x1c160000,
     [VE_RTC] = 0x1c170000,
     [VE_COMPACTFLASH] = 0x1c1a0000,
@@ -392,6 +402,85 @@ static VEDBoardInfo a15_daughterboard = {
     .init = a15_daughterboard_init,
 };
 
+static int add_virtio_mmio_node(void *fdt, uint32_t acells, uint32_t scells,
+                                hwaddr addr, hwaddr size, uint32_t intc,
+                                int irq)
+{
+    /* Add a virtio_mmio node to the device tree blob:
+     *   virtio_mmio@ADDRESS {
+     *       compatible = "virtio,mmio";
+     *       reg = <ADDRESS, SIZE>;
+     *       interrupt-parent = <&intc>;
+     *       interrupts = <0, irq, 1>;
+     *   }
+     * (Note that the format of the interrupts property is dependent on the
+     * interrupt controller that interrupt-parent points to; these are for
+     * the ARM GIC and indicate an SPI interrupt, rising-edge-triggered.)
+     */
+    int rc;
+    char *nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, addr);
+
+    rc = qemu_devtree_add_subnode(fdt, nodename);
+    rc |= qemu_devtree_setprop_string(fdt, nodename,
+                                      "compatible", "virtio,mmio");
+    rc |= qemu_devtree_setprop_sized_cells(fdt, nodename, "reg",
+                                           acells, addr, scells, size);
+    qemu_devtree_setprop_cells(fdt, nodename, "interrupt-parent", intc);
+    qemu_devtree_setprop_cells(fdt, nodename, "interrupts", 0, irq, 1);
+    g_free(nodename);
+    if (rc) {
+        return -1;
+    }
+    return 0;
+}
+
+static uint32_t find_int_controller(void *fdt)
+{
+    /* Find the FDT node corresponding to the interrupt controller
+     * for virtio-mmio devices. We do this by scanning the fdt for
+     * a node with the right compatibility, since we know there is
+     * only one GIC on a vexpress board.
+     * We return the phandle of the node, or 0 if none was found.
+     */
+    const char *compat = "arm,cortex-a9-gic";
+    int offset;
+
+    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
+    if (offset >= 0) {
+        return fdt_get_phandle(fdt, offset);
+    }
+    return 0;
+}
+
+static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
+{
+    uint32_t acells, scells, intc;
+    const VEDBoardInfo *daughterboard = (const VEDBoardInfo *)info;
+
+    acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
+    scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
+    intc = find_int_controller(fdt);
+    if (!intc) {
+        /* Not fatal, we just won't provide virtio. This will
+         * happen with older device tree blobs.
+         */
+        fprintf(stderr, "QEMU: warning: couldn't find interrupt controller in "
+                "dtb; will not include virtio-mmio devices in the dtb.\n");
+    } else {
+        int i;
+        const hwaddr *map = daughterboard->motherboard_map;
+
+        /* We iterate backwards here because adding nodes
+         * to the dtb puts them in last-first.
+         */
+        for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
+            add_virtio_mmio_node(fdt, acells, scells,
+                                 map[VE_VIRTIO] + 0x200 * i,
+                                 0x200, intc, 40 + i);
+        }
+    }
+}
+
 static void vexpress_common_init(VEDBoardInfo *daughterboard,
                                  QEMUMachineInitArgs *args)
 {
@@ -508,6 +597,15 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
 
     /* VE_DAPROM: not modelled */
 
+    /* Create mmio transports, so the user can create virtio backends
+     * (which will be automatically plugged in to the transports). If
+     * no backend is created the transport will just sit harmlessly idle.
+     */
+    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
+        sysbus_create_simple("virtio-mmio", map[VE_VIRTIO] + 0x200 * i,
+                             pic[40 + i]);
+    }
+
     daughterboard->bootinfo.ram_size = args->ram_size;
     daughterboard->bootinfo.kernel_filename = args->kernel_filename;
     daughterboard->bootinfo.kernel_cmdline = args->kernel_cmdline;
@@ -518,6 +616,7 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
     daughterboard->bootinfo.smp_loader_start = map[VE_SRAM];
     daughterboard->bootinfo.smp_bootreg_addr = map[VE_SYSREGS] + 0x30;
     daughterboard->bootinfo.gic_cpu_if_addr = daughterboard->gic_cpu_if_addr;
+    daughterboard->bootinfo.modify_dtb = vexpress_modify_dtb;
     arm_load_kernel(ARM_CPU(first_cpu), &daughterboard->bootinfo);
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 7/8] vexpress: Make VEDBoardInfo extend arm_boot_info

[Peter Crosthwaite]

Hi Peter,

On Sat, Jul 13, 2013 at 6:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Make the VEDBoardInfo struct extend arm_boot_info; this will
> allow us to get at the VEDBoardInfo information inside callbacks
> from arm/boot code.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/vexpress.c |   31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index fd18b60..d1c28aa 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -36,8 +36,6 @@
>  #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
>  #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
>
> -static struct arm_boot_info vexpress_binfo;
> -
>  /* Address maps for peripherals:
>   * the Versatile Express motherboard has two possible maps,
>   * the "legacy" one (used for A9) and the "Cortex-A Series"
> @@ -150,6 +148,7 @@ typedef void DBoardInitFn(const VEDBoardInfo *daughterboard,
>                            qemu_irq *pic);
>
>  struct VEDBoardInfo {
> +    struct arm_boot_info bootinfo;
>      const hwaddr *motherboard_map;
>      hwaddr loader_start;
>      const hwaddr gic_cpu_if_addr;
> @@ -269,7 +268,7 @@ static const uint32_t a9_clocks[] = {
>      66670000, /* Test chip reference clock: 66.67MHz */
>  };
>
> -static const VEDBoardInfo a9_daughterboard = {
> +static VEDBoardInfo a9_daughterboard = {

Wouldn't it be cleaner to just add opaque data to arm_boot_info?, then
you don't have to promote this previously const struct to global state.

Regards,
Peter

Re: [Qemu-devel] [PATCH v2 7/8] vexpress: Make VEDBoardInfo extend arm_boot_info

[Peter Maydell]

On 14 July 2013 12:36, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Hi Peter,
>
> On Sat, Jul 13, 2013 at 6:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Make the VEDBoardInfo struct extend arm_boot_info; this will
>> allow us to get at the VEDBoardInfo information inside callbacks
>> from arm/boot code.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/vexpress.c |   31 +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>> index fd18b60..d1c28aa 100644
>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -36,8 +36,6 @@
>>  #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
>>  #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
>>
>> -static struct arm_boot_info vexpress_binfo;
>> -
>>  /* Address maps for peripherals:
>>   * the Versatile Express motherboard has two possible maps,
>>   * the "legacy" one (used for A9) and the "Cortex-A Series"
>> @@ -150,6 +148,7 @@ typedef void DBoardInitFn(const VEDBoardInfo *daughterboard,
>>                            qemu_irq *pic);
>>
>>  struct VEDBoardInfo {
>> +    struct arm_boot_info bootinfo;
>>      const hwaddr *motherboard_map;
>>      hwaddr loader_start;
>>      const hwaddr gic_cpu_if_addr;
>> @@ -269,7 +268,7 @@ static const uint32_t a9_clocks[] = {
>>      66670000, /* Test chip reference clock: 66.67MHz */
>>  };
>>
>> -static const VEDBoardInfo a9_daughterboard = {
>> +static VEDBoardInfo a9_daughterboard = {
>
> Wouldn't it be cleaner to just add opaque data to arm_boot_info?, then
> you don't have to promote this previously const struct to global state.

I thought about that but decided I didn't really want to throw an
opaque into the boot_info structure when the callbacks already
get a pointer to state.

-- PMM

Re: [Qemu-devel] [PATCH v2 8/8] vexpress: Add virtio-mmio transports

[Peter Crosthwaite]

Hi,

On Sat, Jul 13, 2013 at 6:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add some virtio-mmio transports to the vexpress board model,
> together with a modify_dtb hook which adds them to the device
> tree so that the kernel will probe for them. We put them
> in a reserved area of the address map.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/vexpress.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index d1c28aa..dcc86b9 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -31,11 +31,18 @@
>  #include "exec/address-spaces.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/block/flash.h"
> +#include "sysemu/device_tree.h"
> +#include <libfdt.h>
>
>  #define VEXPRESS_BOARD_ID 0x8e0
>  #define VEXPRESS_FLASH_SIZE (64 * 1024 * 1024)
>  #define VEXPRESS_FLASH_SECT_SIZE (256 * 1024)
>
> +/* Number of virtio transports to create (0..8; limited by
> + * number of available IRQ lines).
> + */
> +#define NUM_VIRTIO_TRANSPORTS 4
> +
>  /* Address maps for peripherals:
>   * the Versatile Express motherboard has two possible maps,
>   * the "legacy" one (used for A9) and the "Cortex-A Series"
> @@ -70,6 +77,7 @@ enum {
>      VE_ETHERNET,
>      VE_USB,
>      VE_DAPROM,
> +    VE_VIRTIO,
>  };
>
>  static hwaddr motherboard_legacy_map[] = {
> @@ -88,6 +96,7 @@ static hwaddr motherboard_legacy_map[] = {
>      [VE_WDT] = 0x1000f000,
>      [VE_TIMER01] = 0x10011000,
>      [VE_TIMER23] = 0x10012000,
> +    [VE_VIRTIO] = 0x10013000,
>      [VE_SERIALDVI] = 0x10016000,
>      [VE_RTC] = 0x10017000,
>      [VE_COMPACTFLASH] = 0x1001a000,
> @@ -132,6 +141,7 @@ static hwaddr motherboard_aseries_map[] = {
>      [VE_WDT] = 0x1c0f0000,
>      [VE_TIMER01] = 0x1c110000,
>      [VE_TIMER23] = 0x1c120000,
> +    [VE_VIRTIO] = 0x1c130000,
>      [VE_SERIALDVI] = 0x1c160000,
>      [VE_RTC] = 0x1c170000,
>      [VE_COMPACTFLASH] = 0x1c1a0000,
> @@ -392,6 +402,85 @@ static VEDBoardInfo a15_daughterboard = {
>      .init = a15_daughterboard_init,
>  };
>
> +static int add_virtio_mmio_node(void *fdt, uint32_t acells, uint32_t scells,
> +                                hwaddr addr, hwaddr size, uint32_t intc,
> +                                int irq)

This seems pretty general to me and maybe belongs in the device tree API. Some
of the PPC machines could make good use of this. From e500:

snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
    qemu_devtree_add_subnode(fdt, mpic);
    qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
    qemu_devtree_setprop_string(fdt, mpic, "compatible", "fsl,mpic");
    qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
                               0x40000);
    qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
    qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);

The nasty intc specific stuff is a problem, but perhaps at least the
boiler plate
creation+reg+compatibile can be factored out for all to use?

Regards,
Peter

Re: [Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Crosthwaite]

Hi Peter,

On Sat, Jul 13, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> We already have a qemu_devtree_setprop_cells() which sets a dtb
> property to an array of cells whose values are specified by varargs.
> However for the fairly common case of setting a property to a list
> of addresses or of address,size pairs the number of cells used by
> each element in the list depends on the parent's #address-cells
> and #size-cells properties. To make this easier we provide an analogous
> qemu_devtree_setprop_sized_cells() macro which allows the number
> of cells used by each element to be specified. This is implemented
> using an underlying qemu_devtree_setprop_sized_cells_from_array()
> function which takes the values and sizes as an array; this may
> also be directly useful for cases where the cell contents are
> constructed programmatically.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  device_tree.c                |   31 +++++++++++++++++++++
>  include/sysemu/device_tree.h |   62 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index 10cf3d0..308205c 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -308,3 +308,34 @@ void qemu_devtree_dumpdtb(void *fdt, int size)
>          exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1);
>      }
>  }
> +
> +int qemu_devtree_setprop_sized_cells_from_array(void *fdt,
> +                                                const char *node_path,
> +                                                const char *property,
> +                                                int numvalues,
> +                                                uint64_t *values)
> +{
> +    uint32_t *propcells;
> +    uint64_t value;
> +    int cellnum, vnum, ncells;
> +    uint32_t hival;
> +
> +    propcells = g_new0(uint32_t, numvalues * 2);
> +
> +    cellnum = 0;
> +    for (vnum = 0; vnum < numvalues; vnum++) {
> +        ncells = values[vnum * 2];
> +        assert(ncells == 1 || ncells == 2);
> +        value = values[vnum * 2 + 1];
> +        hival = cpu_to_be32(value >> 32);
> +        if (ncells > 1) {
> +            propcells[cellnum++] = hival;
> +        } else if (hival != 0) {
> +            return vnum + 1;
> +        }
> +        propcells[cellnum++] = cpu_to_be32(value);
> +    }
> +
> +    return qemu_devtree_setprop(fdt, node_path, property, propcells,
> +                                cellnum * sizeof(uint32_t));
> +}
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f0b3f35..8189ebc 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -51,4 +51,66 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
>
>  void qemu_devtree_dumpdtb(void *fdt, int size);
>
> +/**
> + * qemu_devtree_setprop_sized_cells_from_array:
> + * @fdt: device tree blob
> + * @node_path: node to set property on
> + * @property: property to set
> + * @numvalues: number of values
> + * @values: array of number-of-cells, value pairs
> + *
> + * Set the specified property on the specified node in the device tree
> + * to be an array of cells. The values of the cells are specified via
> + * the values list, which alternates between "number of cells used by
> + * this value" and "value".
> + * number-of-cells must be either 1 or 2 (other values will assert()).
> + *
> + * This function is useful because device tree nodes often have cell arrays
> + * which are either lists of addresses or lists of address,size tuples, but
> + * the number of cells used for each element vary depending on the
> + * #address-cells and #size-cells properties of their parent node.
> + * If you know all your cell elements are one cell wide you can use the
> + * simpler qemu_devtree_setprop_cells(). If you're not setting up the
> + * array programmatically, qemu_devtree_setprop_sized_cells may be more
> + * convenient.
> + *
> + * Return value: 0 on success, <0 if setting the property failed,
> + * n (for n>0) if value n wouldn't fit in the required number of cells.
> + * (This slightly odd convention is for the benefit of callers who might
> + * wish to print different error messages depending on which value
> + * was too large to fit, since values might be user-specified.)

I have patches on list converting the other "setprop" APIs to Error ** based
reporting (as per your suggestion). Once that change happens we will have
two separate error schemes for the one fn.

For sanity checking user specified values (e.g. command line args) cant it
just be done explicitly as required? It seems to me that this will be the
exception not the rule, as for DTB generation from machine model a fail
here will be a fatal anyway (better handled by Error **).

if (acells < 2 && addr >= (1ull << 32)) {
   fprintf("your address is too big for this dtb");
}

LOC looks to be similar and this API gets a little cleaner.

Regards,
Peter

Re: [Qemu-devel] [PATCH v2 8/8] vexpress: Add virtio-mmio transports

[Peter Maydell]

On 15 July 2013 01:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sat, Jul 13, 2013 at 6:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +static int add_virtio_mmio_node(void *fdt, uint32_t acells, uint32_t scells,
>> +                                hwaddr addr, hwaddr size, uint32_t intc,
>> +                                int irq)
>
> This seems pretty general to me and maybe belongs in the device tree API. Some
> of the PPC machines could make good use of this. From e500:
>
> snprintf(mpic, sizeof(mpic), "%s/pic@%llx", soc, MPC8544_MPIC_REGS_OFFSET);
>     qemu_devtree_add_subnode(fdt, mpic);
>     qemu_devtree_setprop_string(fdt, mpic, "device_type", "open-pic");
>     qemu_devtree_setprop_string(fdt, mpic, "compatible", "fsl,mpic");
>     qemu_devtree_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
>                                0x40000);
>     qemu_devtree_setprop_cell(fdt, mpic, "#address-cells", 0);
>     qemu_devtree_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>
> The nasty intc specific stuff is a problem, but perhaps at least the
> boiler plate
> creation+reg+compatibile can be factored out for all to use?

Doesn't really seem worth doing to me -- all we're doing is
setting some string properties, and the reg property is tricky
because it's only the right thing in some cases (ie where you
know you're not putting the node inside some other container
that messes with ranges). The setprop_sized_cells is the part
I felt really merited being factored out.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Maydell]

On 15 July 2013 01:30, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> I have patches on list converting the other "setprop" APIs to Error ** based
> reporting (as per your suggestion). Once that change happens we will have
> two separate error schemes for the one fn.
>
> For sanity checking user specified values (e.g. command line args) cant it
> just be done explicitly as required? It seems to me that this will be the
> exception not the rule, as for DTB generation from machine model a fail
> here will be a fatal anyway (better handled by Error **).
>
> if (acells < 2 && addr >= (1ull << 32)) {
>    fprintf("your address is too big for this dtb");
> }
>
> LOC looks to be similar and this API gets a little cleaner.

It's trying to allow the arm/boot code to continue to distinguish
the two error cases it does currently:
 * RAM start address doesn't fit [a board model error]
 * RAM size doesn't fit [a user error for which a specific
   error message is more friendly and helpful]

It's a fatal error in both cases, but the text is different.

I guess I could put a special purpose "if scells is 1 and
ram_size > 4GB then fail early" check in boot.c...

I wanted to catch "user is using a DTB with only 32 bit
addresses but tried a big fat RAM size", because some earlier
versions of vexpress DTBs did only use 32 bit cells.

-- PMM

Re: [Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Crosthwaite]

Hi Peter,

On Mon, Jul 15, 2013 at 7:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 July 2013 01:30, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> I have patches on list converting the other "setprop" APIs to Error ** based
>> reporting (as per your suggestion). Once that change happens we will have
>> two separate error schemes for the one fn.
>>
>> For sanity checking user specified values (e.g. command line args) cant it
>> just be done explicitly as required? It seems to me that this will be the
>> exception not the rule, as for DTB generation from machine model a fail
>> here will be a fatal anyway (better handled by Error **).
>>
>> if (acells < 2 && addr >= (1ull << 32)) {
>>    fprintf("your address is too big for this dtb");
>> }
>>
>> LOC looks to be similar and this API gets a little cleaner.
>
> It's trying to allow the arm/boot code to continue to distinguish
> the two error cases it does currently:
>  * RAM start address doesn't fit [a board model error]
>  * RAM size doesn't fit [a user error for which a specific
>    error message is more friendly and helpful]
>
> It's a fatal error in both cases, but the text is different.
>
> I guess I could put a special purpose "if scells is 1 and
> ram_size > 4GB then fail early" check in boot.c...
>

That was what I thinking with the above snippet. It goes with the
caller in this case arm/boot.c. You still need to switch on the return
code in the caller for your multiple error messages so I dont think
you save too much save over just sanity checking args beforehand in
boot.c

Regards,
Peter

> I wanted to catch "user is using a DTB with only 32 bit
> addresses but tried a big fat RAM size", because some earlier
> versions of vexpress DTBs did only use 32 bit cells.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Maydell]

On 15 July 2013 11:24, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jul 15, 2013 at 7:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> It's trying to allow the arm/boot code to continue to distinguish
>> the two error cases it does currently:
>>  * RAM start address doesn't fit [a board model error]
>>  * RAM size doesn't fit [a user error for which a specific
>>    error message is more friendly and helpful]
>>
>> It's a fatal error in both cases, but the text is different.
>>
>> I guess I could put a special purpose "if scells is 1 and
>> ram_size > 4GB then fail early" check in boot.c...

> That was what I thinking with the above snippet. It goes with the
> caller in this case arm/boot.c. You still need to switch on the return
> code in the caller for your multiple error messages so I dont think
> you save too much save over just sanity checking args beforehand in
> boot.c

Yeah, you've convinced me -- it's not worth complicating
the heck out of the API for the generic function; I'll
put an explicit check in boot.c

-- PMM

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Michael S. Tsirkin]

On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote:
> The MMIO virtio transport spec allows the guest to tell the host how
> large the queue size is. Add virtio_queue_set_num() function which
> implements this in the QEMU common virtio support code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Probably needs to go back to default value on reset?
Need to migrate?
Is the default value a max legal value? If yes probably a good
idea to enforce this.


> ---
>  hw/virtio/virtio.c         |    8 ++++++++
>  include/hw/virtio/virtio.h |    1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8176c14..01b05f3 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -667,6 +667,14 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>      return vdev->vq[n].pa;
>  }
>  
> +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
> +{
> +    if (num <= VIRTQUEUE_MAX_SIZE) {
> +        vdev->vq[n].vring.num = num;
> +        virtqueue_init(&vdev->vq[n]);
> +    }
> +}
> +
>  int virtio_queue_get_num(VirtIODevice *vdev, int n)
>  {
>      return vdev->vq[n].vring.num;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a6c5c53..95c4772 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -198,6 +198,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data);
>  void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data);
>  void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
>  hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
> +void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
>  int virtio_queue_get_num(VirtIODevice *vdev, int n);
>  void virtio_queue_notify(VirtIODevice *vdev, int n);
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
> -- 
> 1.7.9.5
> 

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Peter Maydell]

On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote:
>> The MMIO virtio transport spec allows the guest to tell the host how
>> large the queue size is. Add virtio_queue_set_num() function which
>> implements this in the QEMU common virtio support code.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Probably needs to go back to default value on reset?

Tricky, since the default value is "whatever was passed to
virtio_add_queue()" and we don't save that anywhere.

For virtio-mmio it is a guest bug to fail to write to the
QueueNum register, so the current behaviour is not out of
specification (and not harmful either AFAICT).

I guess we could add a vring.defaultnum, which would be
set by virtio_add_queue/virtio_del_queue, and have reset
copy defaultnum into num. No migration needed for defaultnum
because it's always the same for a particular qemu config.

> Need to migrate?

It's already migrated (though I'm not entirely sure why).

> Is the default value a max legal value? If yes probably a good
> idea to enforce this.

virtio_add_queue() already enforces this -- it will abort()
if you try to set up a queue with a default size greater
than VIRTQUEUE_MAX_SIZE.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Michael S. Tsirkin]

On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote:
> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote:
> >> The MMIO virtio transport spec allows the guest to tell the host how
> >> large the queue size is. Add virtio_queue_set_num() function which
> >> implements this in the QEMU common virtio support code.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Probably needs to go back to default value on reset?
> 
> Tricky, since the default value is "whatever was passed to
> virtio_add_queue()" and we don't save that anywhere.
>
> For virtio-mmio it is a guest bug to fail to write to the
> QueueNum register, so the current behaviour is not out of
> specification (and not harmful either AFAICT).

Best not to leak info across reboots.
Also if guest sets num = 0 it will cause all kind of
harm, no?

> I guess we could add a vring.defaultnum, which would be
> set by virtio_add_queue/virtio_del_queue, and have reset
> copy defaultnum into num. No migration needed for defaultnum
> because it's always the same for a particular qemu config.

Sounds good.

> > Need to migrate?
> 
> It's already migrated (though I'm not entirely sure why).
> 
> > Is the default value a max legal value? If yes probably a good
> > idea to enforce this.
> 
> virtio_add_queue() already enforces this -- it will abort()
> if you try to set up a queue with a default size greater
> than VIRTQUEUE_MAX_SIZE.

No I mean does default value have some meaning?
Does host supply a hint of a max value
besides VIRTQUEUE_MAX_SIZE?

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Peter Maydell]

On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote:
>> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Jul 12, 2013 at 09:36:57PM +0100, Peter Maydell wrote:
>> >> The MMIO virtio transport spec allows the guest to tell the host how
>> >> large the queue size is. Add virtio_queue_set_num() function which
>> >> implements this in the QEMU common virtio support code.
>> >>
>> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> >
>> > Probably needs to go back to default value on reset?
>>
>> Tricky, since the default value is "whatever was passed to
>> virtio_add_queue()" and we don't save that anywhere.
>>
>> For virtio-mmio it is a guest bug to fail to write to the
>> QueueNum register, so the current behaviour is not out of
>> specification (and not harmful either AFAICT).
>
> Best not to leak info across reboots.
> Also if guest sets num = 0 it will cause all kind of
> harm, no?

Yes, I had the thought on the way into work that we shouldn't
allow the guest to switch between 0 and non-zero.

>> I guess we could add a vring.defaultnum, which would be
>> set by virtio_add_queue/virtio_del_queue, and have reset
>> copy defaultnum into num. No migration needed for defaultnum
>> because it's always the same for a particular qemu config.
>
> Sounds good.
>
>> > Need to migrate?
>>
>> It's already migrated (though I'm not entirely sure why).
>>
>> > Is the default value a max legal value? If yes probably a good
>> > idea to enforce this.
>>
>> virtio_add_queue() already enforces this -- it will abort()
>> if you try to set up a queue with a default size greater
>> than VIRTQUEUE_MAX_SIZE.
>
> No I mean does default value have some meaning?

It's not clear to me -- virtio_add_queue() doesn't document
the semantics of its queue_size parameter.

> Does host supply a hint of a max value
> besides VIRTQUEUE_MAX_SIZE?

The spec says there's a QueueNumMax register which returns
the maximum for the selected queue. The current implementation
of virtio-mmio always returns VIRTQUEUE_MAX_SIZE. We could
return the default size as the QueueNumMax instead but that
would seem to be limiting the guest unnecessarily.

-- PMM

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Peter Maydell]

On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote:
>> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > Probably needs to go back to default value on reset?
>>
>> Tricky, since the default value is "whatever was passed to
>> virtio_add_queue()" and we don't save that anywhere.
>>
>> For virtio-mmio it is a guest bug to fail to write to the
>> QueueNum register, so the current behaviour is not out of
>> specification (and not harmful either AFAICT).
>
> Best not to leak info across reboots.
> Also if guest sets num = 0 it will cause all kind of
> harm, no?
>
>> I guess we could add a vring.defaultnum, which would be
>> set by virtio_add_queue/virtio_del_queue, and have reset
>> copy defaultnum into num. No migration needed for defaultnum
>> because it's always the same for a particular qemu config.

So I had a look at implementing this, and I noticed that
we already have some odd behaviour on reset. Specifically,
virtio backends like net can create virtio queues based on
guest behaviour (ie setting feature bits). But on reset,
these queues aren't deleted, so a post-reset QEMU will look
different from a from-scratch restarted QEMU...

This in turn makes 'save defaultnum and have reset copy it
into num' awkward, because defaultnum now needs to be
migrated (otherwise it would do the wrong thing on a reset
after a VM migration).

-- PMM

Re: [Qemu-devel] [PATCH v2 3/8] virtio: Add support for guest setting of queue size

[Michael S. Tsirkin]

On Thu, Jul 25, 2013 at 01:30:15PM +0100, Peter Maydell wrote:
> On 25 July 2013 10:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Jul 25, 2013 at 09:50:21AM +0100, Peter Maydell wrote:
> >> On 25 July 2013 06:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > Probably needs to go back to default value on reset?
> >>
> >> Tricky, since the default value is "whatever was passed to
> >> virtio_add_queue()" and we don't save that anywhere.
> >>
> >> For virtio-mmio it is a guest bug to fail to write to the
> >> QueueNum register, so the current behaviour is not out of
> >> specification (and not harmful either AFAICT).
> >
> > Best not to leak info across reboots.
> > Also if guest sets num = 0 it will cause all kind of
> > harm, no?
> >
> >> I guess we could add a vring.defaultnum, which would be
> >> set by virtio_add_queue/virtio_del_queue, and have reset
> >> copy defaultnum into num. No migration needed for defaultnum
> >> because it's always the same for a particular qemu config.
> 
> So I had a look at implementing this, and I noticed that
> we already have some odd behaviour on reset. Specifically,
> virtio backends like net can create virtio queues based on
> guest behaviour (ie setting feature bits). But on reset,
> these queues aren't deleted, so a post-reset QEMU will look
> different from a from-scratch restarted QEMU...

That's a bug. Thanks for the report. :)

> This in turn makes 'save defaultnum and have reset copy it
> into num' awkward, because defaultnum now needs to be
> migrated (otherwise it would do the wrong thing on a reset
> after a VM migration).
> 
> -- PMM

Looks like we'll have to fix the bug first :(