[Qemu-devel] [PATCH v3 0/6] machvirt dynamic sysbus device instantiation

[Qemu-devel] [PATCH v3 0/6] machvirt dynamic sysbus device instantiation

[Eric Auger]

This patch series enables machvirt to dynamically instantiate sysbus
devices from command line (using -device option).

It applies on top of Alex Graf's series
- "Dynamic sysbus device allocation support"
  http://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00047.html

and transposes the work done for PPC e500 on machvirt.

The code related to dynamic sysbus device IRQ/mmio region binding,
originally located in PPC e500.c, is moved into
hw/misc/dyn_sysbus_binding new module

for ARM only, the code related to device tree node generation is
moved into hw/arm/dyn_sysbus_devtree module.

Both e500 and machvirt are adapted to use dyn_sysbus_binding helper
routines.

integrated branch can be found on
http://git.linaro.org/people/eric.auger/qemu.git,
branch vfio_integ_v6

A related discussion can be found at
https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg04065.html

For e500 the device tree generation is kept in the machine file,
according to Alex will.

For machvirt we use dyn_sysbus_devtree since the code becomes large
with the expected support of vfio devices.

Best Regards
Eric

v2 -> v3:
- patch now applies on top of Alex full patchset
- dyn_sysbus_devtree: add arm_prefix to emphasize the fact those
  functions are arm specific; arm_sysbus_device_create_devtree
  becomes static
- load_dtb renamed into arm_load_dtb
- add copyright in hw/arm/dyn_sysbus_devtree.c

v1 -> v2:
- device node generation no more in sysbus device but in
  dyn_sysbus_devtree
- VFIO region shrinked to 4MB and relocated in machvirt to avoid PCI
  shrink (dynamic vfio-mmio support might come latter)
- platform_bus_base removed from PlatformDevtreeData


Alexander Graf (1):
  PPC: e500: use dyn_sysbus_binding helper routines

Eric Auger (5):
  hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding
  hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node  
      generation
  hw/arm/boot: load_dtb becomes non static arm_load_dtb
  hw/arm/virt: new add_fdt_*_node functions
  hw/arm/virt: Support dynamically spawned sysbus devices

 hw/arm/Makefile.objs                 |   1 +
 hw/arm/boot.c                        |   4 +-
 hw/arm/dyn_sysbus_devtree.c          |  92 ++++++++++++++++++
 hw/arm/virt.c                        | 129 +++++++++++++++++++-----
 hw/misc/Makefile.objs                |   1 +
 hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++
 hw/ppc/e500.c                        | 183 +++--------------------------------
 hw/ppc/e500.h                        |   7 +-
 hw/ppc/e500plat.c                    |  13 ++-
 include/hw/arm/arm.h                 |   1 +
 include/hw/arm/dyn_sysbus_devtree.h  |  16 +++
 include/hw/misc/dyn_sysbus_binding.h |  24 +++++
 12 files changed, 426 insertions(+), 208 deletions(-)
 create mode 100644 hw/arm/dyn_sysbus_devtree.c
 create mode 100644 hw/misc/dyn_sysbus_binding.c
 create mode 100644 include/hw/arm/dyn_sysbus_devtree.h
 create mode 100644 include/hw/misc/dyn_sysbus_binding.h

-- 
1.8.3.2

[Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Eric Auger]

This new module implements routines which help in dynamic device
binding (mmio regions, irq). They are supposed to be used by machine
files that support dynamic sysbus instantiation.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- platform_devices renamed into dyn_sysbus_binding
- PlatformParams renamed into DynSysbusParams
- PlatformBusNotifier renamed into DynSysbusNotifier
- platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
  platform_bus_init become static
- PlatformBusInitData becomes private to the module
- page_shift becomes a member of DynSysbusParams

v1: Dynamic sysbus device allocation fully written by Alex Graf.
Those functions were initially in ppc e500 machine file. Now moved to a
separate module.
PPCE500Params is replaced by a generic struct named PlatformParams
---
 hw/misc/Makefile.objs                |   1 +
 hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++
 include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
 3 files changed, 188 insertions(+)
 create mode 100644 hw/misc/dyn_sysbus_binding.c
 create mode 100644 include/hw/misc/dyn_sysbus_binding.h

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 979e532..86f6243 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
+obj-y += dyn_sysbus_binding.o
diff --git a/hw/misc/dyn_sysbus_binding.c b/hw/misc/dyn_sysbus_binding.c
new file mode 100644
index 0000000..0f34f0b
--- /dev/null
+++ b/hw/misc/dyn_sysbus_binding.c
@@ -0,0 +1,163 @@
+#include "hw/misc/dyn_sysbus_binding.h"
+#include "qemu/error-report.h"
+
+typedef struct PlatformBusInitData {
+    unsigned long *used_irqs;
+    unsigned long *used_mem;
+    MemoryRegion *mem;
+    qemu_irq *irqs;
+    int device_count;
+    DynSysbusParams *params;
+} PlatformBusInitData;
+
+
+static int platform_bus_map_irq(DynSysbusParams *params,
+                                SysBusDevice *sbdev,
+                                int n, unsigned long *used_irqs,
+                                qemu_irq *platform_irqs)
+{
+    int max_irqs = params->platform_bus_num_irqs;
+    char *prop = g_strdup_printf("irq[%d]", n);
+    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
+
+    if (irqn == SYSBUS_DYNAMIC) {
+        /* Find the first available IRQ */
+        irqn = find_first_zero_bit(used_irqs, max_irqs);
+    }
+
+    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
+        hw_error("IRQ %d is already allocated or no free IRQ left", irqn);
+    }
+
+    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
+    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
+
+    g_free(prop);
+    return 0;
+}
+
+static int platform_bus_map_mmio(DynSysbusParams *params,
+                                 SysBusDevice *sbdev,
+                                 int n, unsigned long *used_mem,
+                                 MemoryRegion *pmem)
+{
+    MemoryRegion *device_mem = sbdev->mmio[n].memory;
+    uint64_t size = memory_region_size(device_mem);
+    uint64_t page_size = (1 << params->page_shift);
+    uint64_t page_mask = page_size - 1;
+    uint64_t size_pages = (size + page_mask) >> params->page_shift;
+    uint64_t max_size = params->platform_bus_size;
+    uint64_t max_pages = max_size >> params->page_shift;
+    char *prop = g_strdup_printf("mmio[%d]", n);
+    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
+    int page;
+    int i;
+
+    page = addr >> params->page_shift;
+    if (addr == SYSBUS_DYNAMIC) {
+        uint64_t size_pages_align;
+
+        /* Align the region to at least its own size granularity */
+        if (is_power_of_2(size_pages)) {
+            size_pages_align = size_pages;
+        } else {
+            size_pages_align = pow2floor(size_pages) << 1;
+        }
+
+        /* Find the first available region that fits */
+        page = bitmap_find_next_zero_area(used_mem, max_pages, 0, size_pages,
+                                          size_pages_align);
+
+        addr = (uint64_t)page << params->page_shift;
+    }
+
+    if (page >= max_pages || test_bit(page, used_mem) ||
+        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
+        hw_error("Memory [%"PRIx64":%"PRIx64" is already allocated or "
+                 "no slot left", addr, size);
+    }
+
+    for (i = page; i < (page + size_pages); i++) {
+        set_bit(i, used_mem);
+    }
+
+    memory_region_add_subregion(pmem, addr, device_mem);
+    sbdev->mmio[n].addr = addr;
+    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
+
+    g_free(prop);
+    return 0;
+}
+
+static int sysbus_device_check(Object *obj, void *opaque)
+{
+    PlatformBusInitData *init = opaque;
+    Object *dev;
+    SysBusDevice *sbdev;
+    int i;
+
+    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
+    sbdev = (SysBusDevice *)dev;
+
+    if (!sbdev) {
+        /* Container, traverse it for children */
+        return object_child_foreach(obj, sysbus_device_check, opaque);
+    }
+
+    /* Connect sysbus device to virtual platform bus */
+    for (i = 0; i < sbdev->num_irq; i++) {
+        if (!sbdev->irqp[i]) {
+            /* This IRQ is an incoming IRQ, we can't wire those here */
+            continue;
+        }
+        platform_bus_map_irq(init->params, sbdev, i,
+                             init->used_irqs, init->irqs);
+    }
+
+    for (i = 0; i < sbdev->num_mmio; i++) {
+        platform_bus_map_mmio(init->params, sbdev, i,
+                              init->used_mem, init->mem);
+    }
+
+    return 0;
+}
+
+static void platform_bus_init(DynSysbusParams *params,
+                       MemoryRegion *address_space_mem,
+                       qemu_irq *mpic)
+{
+    uint64_t max_size = params->platform_bus_size;
+    uint64_t max_pages = max_size >> params->page_shift;
+    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
+    DECLARE_BITMAP(used_mem, max_pages);
+    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
+    Object *container;
+    PlatformBusInitData init = {
+        .used_irqs = used_irqs,
+        .used_mem = used_mem,
+        .mem = platform_region,
+        .irqs = &mpic[params->platform_bus_first_irq],
+        .params = params,
+    };
+
+    memory_region_init(platform_region, NULL, "platform devices",
+                       params->platform_bus_size);
+
+    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
+    bitmap_clear(used_mem, 0, max_pages);
+
+    /* Loop through all sysbus devices that were spawened outside the machine */
+    container = container_get(qdev_get_machine(), "/peripheral");
+    sysbus_device_check(container, &init);
+    container = container_get(qdev_get_machine(), "/peripheral-anon");
+    sysbus_device_check(container, &init);
+
+    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
+                                platform_region);
+}
+
+void platform_bus_init_notify(Notifier *notifier, void *data)
+{
+    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
+    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
+}
diff --git a/include/hw/misc/dyn_sysbus_binding.h b/include/hw/misc/dyn_sysbus_binding.h
new file mode 100644
index 0000000..961c9c7
--- /dev/null
+++ b/include/hw/misc/dyn_sysbus_binding.h
@@ -0,0 +1,24 @@
+#ifndef HW_MISC_PLATFORM_DEVICES_H
+#define HW_MISC_PLATFORM_DEVICES_H
+
+#include "qemu-common.h"
+#include "hw/sysbus.h"
+
+typedef struct {
+    bool has_platform_bus;
+    hwaddr platform_bus_base;
+    hwaddr platform_bus_size;
+    int platform_bus_first_irq;
+    int platform_bus_num_irqs;
+    int page_shift;
+} DynSysbusParams;
+
+typedef struct DynSysbusNotifier {
+    Notifier notifier;
+    MemoryRegion *address_space_mem;
+    qemu_irq *mpic;
+    DynSysbusParams params;
+} DynSysbusNotifier;
+
+void platform_bus_init_notify(Notifier *notifier, void *data);
+#endif
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Eric Auger]

This module will be used by ARM machine files to generate
device tree nodes of dynamically instantiated sysbus devices (ie.
those instantiated with -device option).

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
- add arm_ prefix
- arm_sysbus_device_create_devtree becomes static

v1 -> v2:
- Code moved in an arch specific file to accomodate architecture
  dependent specificities.
- remove platform_bus_base from PlatformDevtreeData

v1: code originally written by Alex Graf in e500.c and reused for ARM
    [Eric Auger]
    code originally moved in hw/misc/platform_devices and device itself
---
 hw/arm/Makefile.objs                |  1 +
 hw/arm/dyn_sysbus_devtree.c         | 66 +++++++++++++++++++++++++++++++++++++
 include/hw/arm/dyn_sysbus_devtree.h | 16 +++++++++
 3 files changed, 83 insertions(+)
 create mode 100644 hw/arm/dyn_sysbus_devtree.c
 create mode 100644 include/hw/arm/dyn_sysbus_devtree.h

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 6088e53..bc5e014 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
 obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
 obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
 obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
+obj-y += dyn_sysbus_devtree.o
 
 obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
 obj-$(CONFIG_DIGIC) += digic.o
diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
new file mode 100644
index 0000000..6375024
--- /dev/null
+++ b/hw/arm/dyn_sysbus_devtree.c
@@ -0,0 +1,66 @@
+#include "hw/arm/dyn_sysbus_devtree.h"
+#include "qemu/error-report.h"
+#include "sysemu/device_tree.h"
+
+static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
+{
+    PlatformDevtreeData *data = opaque;
+    Object *dev;
+    SysBusDevice *sbdev;
+    bool matched = false;
+
+    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
+    sbdev = (SysBusDevice *)dev;
+
+    if (!sbdev) {
+        /* Container, traverse it for children */
+        return object_child_foreach(obj,
+                                    arm_sysbus_device_create_devtree, data);
+    }
+
+    if (!matched) {
+        error_report("Device %s is not supported by this machine yet.",
+                     qdev_fw_name(DEVICE(dev)));
+        exit(1);
+    }
+
+    return 0;
+}
+
+void arm_platform_bus_create_devtree(DynSysbusParams *params,
+                                 void *fdt, const char *intc)
+{
+    gchar *node = g_strdup_printf("/platform@%"PRIx64,
+                                  params->platform_bus_base);
+    const char platcomp[] = "qemu,platform\0simple-bus";
+    PlatformDevtreeData data;
+    Object *container;
+    uint64_t addr = params->platform_bus_base;
+    uint64_t size = params->platform_bus_size;
+    int irq_start = params->platform_bus_first_irq;
+
+    /* Create a /platform node that we can put all devices into */
+    qemu_fdt_add_subnode(fdt, node);
+    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
+
+    /* Our platform bus region is less than 32bit big, so 1 cell is enough for
+       address and size */
+    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
+    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
+    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, size);
+
+    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
+
+    /* Loop through all devices and create nodes for known ones */
+    data.fdt = fdt;
+    data.intc = intc;
+    data.irq_start = irq_start;
+    data.node = node;
+
+    container = container_get(qdev_get_machine(), "/peripheral");
+    arm_sysbus_device_create_devtree(container, &data);
+    container = container_get(qdev_get_machine(), "/peripheral-anon");
+    arm_sysbus_device_create_devtree(container, &data);
+
+    g_free(node);
+}
diff --git a/include/hw/arm/dyn_sysbus_devtree.h b/include/hw/arm/dyn_sysbus_devtree.h
new file mode 100644
index 0000000..b072365
--- /dev/null
+++ b/include/hw/arm/dyn_sysbus_devtree.h
@@ -0,0 +1,16 @@
+#ifndef HW_ARM_DYN_SYSBUS_DEVTREE_H
+#define HW_ARM_DYN_SYSBUS_DEVTREE_H
+
+#include "hw/misc/dyn_sysbus_binding.h"
+
+typedef struct PlatformDevtreeData {
+    void *fdt;
+    const char *intc;
+    int irq_start;
+    const char *node;
+} PlatformDevtreeData;
+
+void arm_platform_bus_create_devtree(DynSysbusParams *params,
+                                    void *fdt, const char *intc);
+
+#endif
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 3/6] PPC: e500: use dyn_sysbus_binding helper routines

[Eric Auger]

From: Alexander Graf <agraf@suse.de>

Now platform_bus_init_notify and functions it calls were moved
to dyn_sysbus_binding.c, remove those functions from e500.

PPCE500Params includes a DynSysbusParams struct which contains
the settings related to platform device instantiation.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:

- Modify e500.c to use platform_bus_init_notify now implemented in
dyn_sysbus_binding
- new DynSysbusParams that contains dynamic sysbus settings
- PPCE500Params includes a DynSysbusParams struct
---
 hw/ppc/e500.c     | 183 ++++--------------------------------------------------
 hw/ppc/e500.h     |   7 +--
 hw/ppc/e500plat.c |  13 ++--
 3 files changed, 22 insertions(+), 181 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index fe9497a..0741412 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -38,6 +38,7 @@
 #include "hw/pci-host/ppce500.h"
 #include "qemu/error-report.h"
 #include "hw/net/fsl_etsec/etsec.h"
+#include "hw/misc/dyn_sysbus_binding.h"
 
 #define EPAPR_MAGIC                (0x45504150)
 #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
@@ -196,13 +197,14 @@ static int sysbus_device_create_devtree(Object *obj, void *opaque)
 static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
                                         const char *mpic)
 {
-    gchar *node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
+    gchar *node = g_strdup_printf("/platform@%"PRIx64,
+                                  params->dyn_sysbus_params.platform_bus_base);
     const char platcomp[] = "qemu,platform\0simple-bus";
     PlatformDevtreeData data;
     Object *container;
-    uint64_t addr = params->platform_bus_base;
-    uint64_t size = params->platform_bus_size;
-    int irq_start = params->platform_bus_first_irq;
+    uint64_t addr = params->dyn_sysbus_params.platform_bus_base;
+    uint64_t size = params->dyn_sysbus_params.platform_bus_size;
+    int irq_start = params->dyn_sysbus_params.platform_bus_first_irq;
 
     /* Create a /platform node that we can put all devices into */
 
@@ -489,7 +491,7 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
     qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
 
-    if (params->has_platform_bus) {
+    if (params->dyn_sysbus_params.has_platform_bus) {
         platform_bus_create_devtree(params, fdt, mpic);
     }
 
@@ -732,169 +734,6 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     return mpic;
 }
 
-typedef struct PlatformBusNotifier {
-    Notifier notifier;
-    MemoryRegion *address_space_mem;
-    qemu_irq *mpic;
-    PPCE500Params params;
-} PlatformBusNotifier;
-
-typedef struct PlatformBusInitData {
-    unsigned long *used_irqs;
-    unsigned long *used_mem;
-    MemoryRegion *mem;
-    qemu_irq *irqs;
-    int device_count;
-    PPCE500Params *params;
-} PlatformBusInitData;
-
-static int platform_bus_map_irq(PPCE500Params *params, SysBusDevice *sbdev,
-                                int n, unsigned long *used_irqs,
-                                qemu_irq *platform_irqs)
-{
-    int max_irqs = params->platform_bus_num_irqs;
-    char *prop = g_strdup_printf("irq[%d]", n);
-    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
-
-    if (irqn == SYSBUS_DYNAMIC) {
-        /* Find the first available IRQ */
-        irqn = find_first_zero_bit(used_irqs, max_irqs);
-    }
-
-    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
-        hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
-    }
-
-    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
-    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
-
-    g_free(prop);
-    return 0;
-}
-
-static int platform_bus_map_mmio(PPCE500Params *params, SysBusDevice *sbdev,
-                                 int n, unsigned long *used_mem,
-                                 MemoryRegion *pmem)
-{
-    MemoryRegion *device_mem = sbdev->mmio[n].memory;
-    uint64_t size = memory_region_size(device_mem);
-    uint64_t page_size = (1 << E500_PLATFORM_BUS_PAGE_SHIFT);
-    uint64_t page_mask = page_size - 1;
-    uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_BUS_PAGE_SHIFT;
-    uint64_t max_size = params->platform_bus_size;
-    uint64_t max_pages = max_size >> E500_PLATFORM_BUS_PAGE_SHIFT;
-    char *prop = g_strdup_printf("mmio[%d]", n);
-    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
-    int page;
-    int i;
-
-    page = addr >> E500_PLATFORM_BUS_PAGE_SHIFT;
-    if (addr == SYSBUS_DYNAMIC) {
-        uint64_t size_pages_align;
-
-        /* Align the region to at least its own size granularity */
-        if (is_power_of_2(size_pages)) {
-            size_pages_align = size_pages;
-        } else {
-            size_pages_align = pow2floor(size_pages) << 1;
-        }
-
-        /* Find the first available region that fits */
-        page = bitmap_find_next_zero_area(used_mem, max_pages, 0, size_pages,
-                                          size_pages_align);
-
-        addr = (uint64_t)page << E500_PLATFORM_BUS_PAGE_SHIFT;
-    }
-
-    if (page >= max_pages || test_bit(page, used_mem) ||
-        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
-        hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
-                 "no slot left", addr, size);
-    }
-
-    for (i = page; i < (page + size_pages); i++) {
-        set_bit(i, used_mem);
-    }
-
-    memory_region_add_subregion(pmem, addr, device_mem);
-    sbdev->mmio[n].addr = addr;
-    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
-
-    g_free(prop);
-    return 0;
-}
-
-static int sysbus_device_check(Object *obj, void *opaque)
-{
-    PlatformBusInitData *init = opaque;
-    Object *dev;
-    SysBusDevice *sbdev;
-    int i;
-
-    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
-    sbdev = (SysBusDevice *)dev;
-
-    if (!sbdev) {
-        /* Container, traverse it for children */
-        return object_child_foreach(obj, sysbus_device_check, opaque);
-    }
-
-    /* Connect sysbus device to virtual platform bus */
-    for (i = 0; i < sbdev->num_irq; i++) {
-        if (!sbdev->irqp[i]) {
-            /* This IRQ is an incoming IRQ, we can't wire those here */
-            continue;
-        }
-        platform_bus_map_irq(init->params, sbdev, i, init->used_irqs, init->irqs);
-    }
-
-    for (i = 0; i < sbdev->num_mmio; i++) {
-        platform_bus_map_mmio(init->params, sbdev, i, init->used_mem, init->mem);
-    }
-
-    return 0;
-}
-
-static void platform_bus_init(PPCE500Params *params,
-                              MemoryRegion *address_space_mem,
-                              qemu_irq *mpic)
-{
-    uint64_t max_size = params->platform_bus_size;
-    uint64_t max_pages = max_size >> E500_PLATFORM_BUS_PAGE_SHIFT;
-    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
-    DECLARE_BITMAP(used_mem, max_pages);
-    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
-    Object *container;
-    PlatformBusInitData init = {
-        .used_irqs = used_irqs,
-        .used_mem = used_mem,
-        .mem = platform_region,
-        .irqs = &mpic[params->platform_bus_first_irq],
-        .params = params,
-    };
-
-    memory_region_init(platform_region, NULL, "platform devices",
-                       params->platform_bus_size);
-
-    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
-    bitmap_clear(used_mem, 0, max_pages);
-
-    /* Loop through all sysbus devices that were spawened outside the machine */
-    container = container_get(qdev_get_machine(), "/peripheral");
-    sysbus_device_check(container, &init);
-    container = container_get(qdev_get_machine(), "/peripheral-anon");
-    sysbus_device_check(container, &init);
-
-    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
-                                platform_region);
-}
-
-static void platform_bus_init_notify(Notifier *notifier, void *data)
-{
-    PlatformBusNotifier *pn = (PlatformBusNotifier *)notifier;
-    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
-}
-
 void ppce500_init(MachineState *machine, PPCE500Params *params)
 {
     MemoryRegion *address_space_mem = get_system_memory();
@@ -1047,13 +886,15 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     }
 
     /* Platform Devices */
-    if (params->has_platform_bus) {
-        PlatformBusNotifier *notifier = g_new(PlatformBusNotifier, 1);
+    if (params->dyn_sysbus_params.has_platform_bus) {
+        DynSysbusNotifier *notifier = g_new(DynSysbusNotifier, 1);
+        params->dyn_sysbus_params.page_shift =
+            E500_PLATFORM_BUS_PAGE_SHIFT;
 
         notifier->notifier.notify = platform_bus_init_notify;
         notifier->address_space_mem = address_space_mem;
         notifier->mpic = mpic;
-        notifier->params = *params;
+        notifier->params = params->dyn_sysbus_params;
         qemu_add_machine_init_done_notifier(&notifier->notifier);
     }
 
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index f1b2766..c191f9d 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -2,6 +2,7 @@
 #define PPCE500_H
 
 #include "hw/boards.h"
+#include "hw/misc/dyn_sysbus_binding.h"
 
 typedef struct PPCE500Params {
     int pci_first_slot;
@@ -11,11 +12,7 @@ typedef struct PPCE500Params {
     void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
 
     int mpic_version;
-    bool has_platform_bus;
-    hwaddr platform_bus_base;
-    hwaddr platform_bus_size;
-    int platform_bus_first_irq;
-    int platform_bus_num_irqs;
+    DynSysbusParams dyn_sysbus_params;
 } PPCE500Params;
 
 void ppce500_init(MachineState *machine, PPCE500Params *params);
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index befe1d1..4150a2c 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -35,11 +35,14 @@ static void e500plat_init(MachineState *machine)
         .pci_nr_slots = PCI_SLOT_MAX - 1,
         .fixup_devtree = e500plat_fixup_devtree,
         .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
-        .has_platform_bus = true,
-        .platform_bus_base = 0xf00000000ULL,
-        .platform_bus_size = (128ULL * 1024 * 1024),
-        .platform_bus_first_irq = 5,
-        .platform_bus_num_irqs = 10,
+        .dyn_sysbus_params = {
+            .has_platform_bus = true,
+            .platform_bus_base = 0xf00000000ULL,
+            .platform_bus_size = (128ULL * 1024 * 1024),
+            .platform_bus_first_irq = 5,
+            .platform_bus_num_irqs = 10,
+            .page_shift = 12 /* default */
+        }
     };
 
     /* Older KVM versions don't support EPR which breaks guests when we announce
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 4/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Eric Auger]

load_dtb is renamed into arm_load_dtb and becomes non static.
it will be used by machvirt for dynamic instantiation of
platform devices

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
load_dtb renamed into arm_load_dtb
---
 hw/arm/boot.c        | 4 ++--
 include/hw/arm/arm.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e32f2f4..79fe0f1 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -312,7 +312,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
     }
 }
 
-static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 {
     void *fdt = NULL;
     int size, rc;
@@ -569,7 +569,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
              */
             hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
                                              4096);
-            if (load_dtb(dtb_start, info)) {
+            if (arm_load_dtb(dtb_start, info)) {
                 exit(1);
             }
             fixupcontext[FIXUP_ARGPTR] = dtb_start;
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index cefc9e6..e60c4c0 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -68,6 +68,7 @@ struct arm_boot_info {
     hwaddr entry;
 };
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo);
 
 /* Multiplication factor to convert from system clock ticks to qemu timer
    ticks.  */
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 5/6] hw/arm/virt: new add_fdt_*_node functions

[Eric Auger]

Create new functions:
- add_fdt_uart_node
- add_fdt_rtc_node
- add_fdt_virtio_nodes

They will be used for dynamic sysbus instantiation.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
reword title to avoid content violation
---
 hw/arm/virt.c | 67 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d6fffc7..9085b88 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -351,18 +351,15 @@ static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
     fdt_add_gic_node(vbi);
 }
 
-static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void fdt_add_uart_node(const VirtBoardInfo *vbi)
 {
-    char *nodename;
     hwaddr base = vbi->memmap[VIRT_UART].base;
     hwaddr size = vbi->memmap[VIRT_UART].size;
     int irq = vbi->irqmap[VIRT_UART];
     const char compat[] = "arm,pl011\0arm,primecell";
     const char clocknames[] = "uartclk\0apb_pclk";
+    char *nodename = g_strdup_printf("/pl011@%" PRIx64, base);
 
-    sysbus_create_simple("pl011", base, pic[irq]);
-
-    nodename = g_strdup_printf("/pl011@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
     /* Note that we can't use setprop_string because of the embedded NUL */
     qemu_fdt_setprop(vbi->fdt, nodename, "compatible",
@@ -379,17 +376,23 @@ static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }
 
-static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    hwaddr base = vbi->memmap[VIRT_UART].base;
+    int irq = vbi->irqmap[VIRT_UART];
+
+    sysbus_create_simple("pl011", base, pic[irq]);
+    fdt_add_uart_node(vbi);
+}
+
+static void fdt_add_rtc_node(const VirtBoardInfo *vbi)
 {
-    char *nodename;
     hwaddr base = vbi->memmap[VIRT_RTC].base;
     hwaddr size = vbi->memmap[VIRT_RTC].size;
     int irq = vbi->irqmap[VIRT_RTC];
     const char compat[] = "arm,pl031\0arm,primecell";
+    char *nodename = g_strdup_printf("/pl031@%" PRIx64, base);
 
-    sysbus_create_simple("pl031", base, pic[irq]);
-
-    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
     qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
@@ -402,22 +405,20 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }
 
-static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
-    int i;
-    hwaddr size = vbi->memmap[VIRT_MMIO].size;
+    hwaddr base = vbi->memmap[VIRT_RTC].base;
+    int irq = vbi->irqmap[VIRT_RTC];
 
-    /* Note that we have to create the transports in forwards order
-     * so that command line devices are inserted lowest address first,
-     * and then add dtb nodes in reverse order so that they appear in
-     * the finished device tree lowest address first.
-     */
-    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
-        int irq = vbi->irqmap[VIRT_MMIO] + i;
-        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
+    sysbus_create_simple("pl031", base, pic[irq]);
 
-        sysbus_create_simple("virtio-mmio", base, pic[irq]);
-    }
+    fdt_add_rtc_node(vbi);
+}
+
+static void fdt_add_virtio_nodes(const VirtBoardInfo *vbi)
+{
+    int i;
+    hwaddr size = vbi->memmap[VIRT_MMIO].size;
 
     for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
         char *nodename;
@@ -437,6 +438,26 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
     }
 }
 
+static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    int i;
+    hwaddr size = vbi->memmap[VIRT_MMIO].size;
+
+    /* Note that we have to create the transports in forwards order
+     * so that command line devices are inserted lowest address first,
+     * and then add dtb nodes in reverse order so that they appear in
+     * the finished device tree lowest address first.
+     */
+    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
+        int irq = vbi->irqmap[VIRT_MMIO] + i;
+        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
+
+        sysbus_create_simple("virtio-mmio", base, pic[irq]);
+    }
+
+    fdt_add_virtio_nodes(vbi);
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 6/6] hw/arm/virt: Support dynamically spawned sysbus devices

[Eric Auger]

Allows sysbus devices to be instantiated from command line by
using -device option

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3
- renaming of arm_platform_bus_create_devtree and arm_load_dtb
- add copyright in hw/arm/dyn_sysbus_devtree.c

v1 -> v2:
- remove useless vfio-platform.h include file
- s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
- use dyn_sysbus_binding and dyn_sysbus_devtree
- dynamic sysbus platform buse size shrinked to 4MB and
  moved between RTC and MMIO

v1:

Inspired from what Alex Graf did in ppc e500
https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
---
 hw/arm/dyn_sysbus_devtree.c | 26 +++++++++++++++++++
 hw/arm/virt.c               | 62 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
index 6375024..61e5b5f 100644
--- a/hw/arm/dyn_sysbus_devtree.c
+++ b/hw/arm/dyn_sysbus_devtree.c
@@ -1,7 +1,30 @@
+/*
+ * ARM Platform Bus device tree generation helpers 
+ *
+ * Copyright (c) 2014 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/arm/dyn_sysbus_devtree.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
 
+/**
+ * arm_sysbus_device_create_devtree - create the node of devices
+ * attached to the platform bus
+ */
 static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
 {
     PlatformDevtreeData *data = opaque;
@@ -27,6 +50,9 @@ static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
     return 0;
 }
 
+/**
+ * arm_platform_bus_create_devtree - create the platform bus node
+ */
 void arm_platform_bus_create_devtree(DynSysbusParams *params,
                                  void *fdt, const char *intc)
 {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9085b88..16acf44 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -40,6 +40,8 @@
 #include "exec/address-spaces.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
+#include "hw/misc/dyn_sysbus_binding.h"
+#include "hw/arm/dyn_sysbus_devtree.h"
 
 #define NUM_VIRTIO_TRANSPORTS 32
 
@@ -57,6 +59,14 @@
 #define GIC_FDT_IRQ_PPI_CPU_START 8
 #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
 
+#define MACHVIRT_PLATFORM_BASE         0x9400000
+#define MACHVIRT_PLATFORM_SIZE         (4ULL * 1024 * 1024) /* 4 MB */
+#define MACHVIRT_PLATFORM_PAGE_SHIFT   12
+#define MACHVIRT_PLATFORM_SIZE_PAGES   (MACHVIRT_PLATFORM_SIZE >> \
+                                    MACHVIRT_PLATFORM_PAGE_SHIFT)
+#define MACHVIRT_PLATFORM_FIRST_IRQ    48
+#define MACHVIRT_PLATFORM_NUM_IRQS     20
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,
@@ -66,6 +76,7 @@ enum {
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
+    VIRT_PLATFORM,
 };
 
 typedef struct MemMapEntry {
@@ -105,16 +116,27 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
     [VIRT_UART] =       { 0x09000000, 0x00001000 },
     [VIRT_RTC] =        { 0x09010000, 0x00001000 },
+    [VIRT_PLATFORM] = {MACHVIRT_PLATFORM_BASE , MACHVIRT_PLATFORM_SIZE},
     [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     /* 0x10000000 .. 0x40000000 reserved for PCI */
-    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
+    [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
 };
 
 static const int a15irqmap[] = {
     [VIRT_UART] = 1,
     [VIRT_RTC] = 2,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
+    [VIRT_PLATFORM] = MACHVIRT_PLATFORM_FIRST_IRQ,
+};
+
+static DynSysbusParams dyn_sysbus_platform_params = {
+    .has_platform_bus = true,
+    .platform_bus_base = MACHVIRT_PLATFORM_BASE,
+    .platform_bus_size = MACHVIRT_PLATFORM_SIZE,
+    .platform_bus_first_irq = MACHVIRT_PLATFORM_FIRST_IRQ,
+    .platform_bus_num_irqs = MACHVIRT_PLATFORM_NUM_IRQS,
+    .page_shift = MACHVIRT_PLATFORM_PAGE_SHIFT,
 };
 
 static VirtBoardInfo machines[] = {
@@ -458,6 +480,18 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
     fdt_add_virtio_nodes(vbi);
 }
 
+static void machvirt_prep_device_tree(VirtBoardInfo *vbi)
+{
+    create_fdt(vbi);
+    fdt_add_timer_nodes(vbi);
+    fdt_add_cpu_nodes(vbi);
+    fdt_add_psci_node(vbi);
+    fdt_add_gic_node(vbi);
+    fdt_add_uart_node(vbi);
+    fdt_add_rtc_node(vbi);
+    fdt_add_virtio_nodes(vbi);
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
@@ -466,14 +500,28 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
     return board->fdt;
 }
 
+static void machvirt_reset_device_tree(void *opaque)
+{
+    VirtBoardInfo *board = (VirtBoardInfo *)opaque;
+    struct arm_boot_info *info = &board->bootinfo;
+    hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + info->initrd_size,
+                                     4096);
+    machvirt_prep_device_tree(board);
+    arm_platform_bus_create_devtree(&dyn_sysbus_platform_params,
+                                board->fdt, "/intc");
+
+    arm_load_dtb(dtb_start, info);
+}
+
 static void machvirt_init(MachineState *machine)
 {
-    qemu_irq pic[NUM_IRQS];
+    qemu_irq *pic = g_new(qemu_irq, NUM_IRQS);
     MemoryRegion *sysmem = get_system_memory();
     int n;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     const char *cpu_model = machine->cpu_model;
     VirtBoardInfo *vbi;
+    DynSysbusNotifier *notifier;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
@@ -547,6 +595,13 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
+    notifier = g_new(DynSysbusNotifier, 1);
+    notifier->notifier.notify = platform_bus_init_notify;
+    notifier->address_space_mem = sysmem;
+    notifier->mpic = pic;
+    notifier->params = dyn_sysbus_platform_params;
+    qemu_add_machine_init_done_notifier(&notifier->notifier);
+
     vbi->bootinfo.ram_size = machine->ram_size;
     vbi->bootinfo.kernel_filename = machine->kernel_filename;
     vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -556,6 +611,8 @@ static void machvirt_init(MachineState *machine)
     vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
     vbi->bootinfo.get_dtb = machvirt_dtb;
     arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
+
+    qemu_register_reset(machvirt_reset_device_tree, vbi);
 }
 
 static QEMUMachine machvirt_a15_machine = {
@@ -563,6 +620,7 @@ static QEMUMachine machvirt_a15_machine = {
     .desc = "ARM Virtual Machine",
     .init = machvirt_init,
     .max_cpus = 8,
+    .has_dynamic_sysbus = true,
 };
 
 static void machvirt_machine_init(void)
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 09/09/2014 09:54, Eric Auger ha scritto:
> This new module implements routines which help in dynamic device
> binding (mmio regions, irq). They are supposed to be used by machine
> files that support dynamic sysbus instantiation.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - platform_devices renamed into dyn_sysbus_binding

Please use "dynamic" instead of dyn.

> - PlatformParams renamed into DynSysbusParams
> - PlatformBusNotifier renamed into DynSysbusNotifier
> - platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
>   platform_bus_init become static
> - PlatformBusInitData becomes private to the module
> - page_shift becomes a member of DynSysbusParams
> 
> v1: Dynamic sysbus device allocation fully written by Alex Graf.
> Those functions were initially in ppc e500 machine file. Now moved to a
> separate module.
> PPCE500Params is replaced by a generic struct named PlatformParams
> ---
>  hw/misc/Makefile.objs                |   1 +
>  hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++

Please put these in hw/core/sysbus.c and include/hw/sysbus.h, and rename
everything called "Platform" or "platform_" to "Sysbus" or "sysbus_".

> +void platform_bus_init_notify(Notifier *notifier, void *data)
> +{
> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
> +}

Please provide a wrapper like sysbus_register_dynamic that takes the
params/address_space_mem/mpic as parameter.  platform_bus_init_notify
and DynSysbusNotifier can remain hidden within the .c file.

Also, why not move the address_space_mem and mpic into the parameters
struct?

Paolo

>  include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 hw/misc/dyn_sysbus_binding.c
>  create mode 100644 include/hw/misc/dyn_sysbus_binding.h
> 
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 979e532..86f6243 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>  
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
> +obj-y += dyn_sysbus_binding.o
> diff --git a/hw/misc/dyn_sysbus_binding.c b/hw/misc/dyn_sysbus_binding.c
> new file mode 100644
> index 0000000..0f34f0b
> --- /dev/null
> +++ b/hw/misc/dyn_sysbus_binding.c
> @@ -0,0 +1,163 @@
> +#include "hw/misc/dyn_sysbus_binding.h"
> +#include "qemu/error-report.h"
> +
> +typedef struct PlatformBusInitData {
> +    unsigned long *used_irqs;
> +    unsigned long *used_mem;
> +    MemoryRegion *mem;
> +    qemu_irq *irqs;
> +    int device_count;
> +    DynSysbusParams *params;
> +} PlatformBusInitData;
> +
> +
> +static int platform_bus_map_irq(DynSysbusParams *params,
> +                                SysBusDevice *sbdev,
> +                                int n, unsigned long *used_irqs,
> +                                qemu_irq *platform_irqs)
> +{
> +    int max_irqs = params->platform_bus_num_irqs;
> +    char *prop = g_strdup_printf("irq[%d]", n);
> +    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
> +
> +    if (irqn == SYSBUS_DYNAMIC) {
> +        /* Find the first available IRQ */
> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
> +    }
> +
> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
> +        hw_error("IRQ %d is already allocated or no free IRQ left", irqn);
> +    }
> +
> +    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
> +    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
> +
> +    g_free(prop);
> +    return 0;
> +}
> +
> +static int platform_bus_map_mmio(DynSysbusParams *params,
> +                                 SysBusDevice *sbdev,
> +                                 int n, unsigned long *used_mem,
> +                                 MemoryRegion *pmem)
> +{
> +    MemoryRegion *device_mem = sbdev->mmio[n].memory;
> +    uint64_t size = memory_region_size(device_mem);
> +    uint64_t page_size = (1 << params->page_shift);
> +    uint64_t page_mask = page_size - 1;
> +    uint64_t size_pages = (size + page_mask) >> params->page_shift;
> +    uint64_t max_size = params->platform_bus_size;
> +    uint64_t max_pages = max_size >> params->page_shift;
> +    char *prop = g_strdup_printf("mmio[%d]", n);
> +    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
> +    int page;
> +    int i;
> +
> +    page = addr >> params->page_shift;
> +    if (addr == SYSBUS_DYNAMIC) {
> +        uint64_t size_pages_align;
> +
> +        /* Align the region to at least its own size granularity */
> +        if (is_power_of_2(size_pages)) {
> +            size_pages_align = size_pages;
> +        } else {
> +            size_pages_align = pow2floor(size_pages) << 1;
> +        }
> +
> +        /* Find the first available region that fits */
> +        page = bitmap_find_next_zero_area(used_mem, max_pages, 0, size_pages,
> +                                          size_pages_align);
> +
> +        addr = (uint64_t)page << params->page_shift;
> +    }
> +
> +    if (page >= max_pages || test_bit(page, used_mem) ||
> +        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
> +        hw_error("Memory [%"PRIx64":%"PRIx64" is already allocated or "
> +                 "no slot left", addr, size);
> +    }
> +
> +    for (i = page; i < (page + size_pages); i++) {
> +        set_bit(i, used_mem);
> +    }
> +
> +    memory_region_add_subregion(pmem, addr, device_mem);
> +    sbdev->mmio[n].addr = addr;
> +    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
> +
> +    g_free(prop);
> +    return 0;
> +}
> +
> +static int sysbus_device_check(Object *obj, void *opaque)
> +{
> +    PlatformBusInitData *init = opaque;
> +    Object *dev;
> +    SysBusDevice *sbdev;
> +    int i;
> +
> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> +    sbdev = (SysBusDevice *)dev;
> +
> +    if (!sbdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, sysbus_device_check, opaque);
> +    }
> +
> +    /* Connect sysbus device to virtual platform bus */
> +    for (i = 0; i < sbdev->num_irq; i++) {
> +        if (!sbdev->irqp[i]) {
> +            /* This IRQ is an incoming IRQ, we can't wire those here */
> +            continue;
> +        }
> +        platform_bus_map_irq(init->params, sbdev, i,
> +                             init->used_irqs, init->irqs);
> +    }
> +
> +    for (i = 0; i < sbdev->num_mmio; i++) {
> +        platform_bus_map_mmio(init->params, sbdev, i,
> +                              init->used_mem, init->mem);
> +    }
> +
> +    return 0;
> +}
> +
> +static void platform_bus_init(DynSysbusParams *params,
> +                       MemoryRegion *address_space_mem,
> +                       qemu_irq *mpic)
> +{
> +    uint64_t max_size = params->platform_bus_size;
> +    uint64_t max_pages = max_size >> params->page_shift;
> +    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
> +    DECLARE_BITMAP(used_mem, max_pages);
> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
> +    Object *container;
> +    PlatformBusInitData init = {
> +        .used_irqs = used_irqs,
> +        .used_mem = used_mem,
> +        .mem = platform_region,
> +        .irqs = &mpic[params->platform_bus_first_irq],
> +        .params = params,
> +    };
> +
> +    memory_region_init(platform_region, NULL, "platform devices",
> +                       params->platform_bus_size);
> +
> +    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
> +    bitmap_clear(used_mem, 0, max_pages);
> +
> +    /* Loop through all sysbus devices that were spawened outside the machine */
> +    container = container_get(qdev_get_machine(), "/peripheral");
> +    sysbus_device_check(container, &init);
> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
> +    sysbus_device_check(container, &init);
> +
> +    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
> +                                platform_region);
> +}
> +
> +void platform_bus_init_notify(Notifier *notifier, void *data)
> +{
> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
> +}
> diff --git a/include/hw/misc/dyn_sysbus_binding.h b/include/hw/misc/dyn_sysbus_binding.h
> new file mode 100644
> index 0000000..961c9c7
> --- /dev/null
> +++ b/include/hw/misc/dyn_sysbus_binding.h
> @@ -0,0 +1,24 @@
> +#ifndef HW_MISC_PLATFORM_DEVICES_H
> +#define HW_MISC_PLATFORM_DEVICES_H
> +
> +#include "qemu-common.h"
> +#include "hw/sysbus.h"
> +
> +typedef struct {
> +    bool has_platform_bus;
> +    hwaddr platform_bus_base;
> +    hwaddr platform_bus_size;
> +    int platform_bus_first_irq;
> +    int platform_bus_num_irqs;
> +    int page_shift;
> +} DynSysbusParams;
> +
> +typedef struct DynSysbusNotifier {
> +    Notifier notifier;
> +    MemoryRegion *address_space_mem;
> +    qemu_irq *mpic;
> +    DynSysbusParams params;
> +} DynSysbusNotifier;
> +
> +void platform_bus_init_notify(Notifier *notifier, void *data);
> +#endif
> 

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Paolo Bonzini]

Il 09/09/2014 09:54, Eric Auger ha scritto:
> This module will be used by ARM machine files to generate
> device tree nodes of dynamically instantiated sysbus devices (ie.
> those instantiated with -device option).
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v2 -> v3:
> - add arm_ prefix
> - arm_sysbus_device_create_devtree becomes static
> 
> v1 -> v2:
> - Code moved in an arch specific file to accomodate architecture
>   dependent specificities.
> - remove platform_bus_base from PlatformDevtreeData
> 
> v1: code originally written by Alex Graf in e500.c and reused for ARM
>     [Eric Auger]
>     code originally moved in hw/misc/platform_devices and device itself
> ---
>  hw/arm/Makefile.objs                |  1 +
>  hw/arm/dyn_sysbus_devtree.c         | 66 +++++++++++++++++++++++++++++++++++++

File names in QEMU typically use a dash instead of an underscore.  Also,
I see the "fdt" moniker used more often than "devtree" (ouch, I checked
now and it's 7 vs. 851 uses :)).  So what about hw/arm/sysbus-fdt.c?

>  include/hw/arm/dyn_sysbus_devtree.h | 16 +++++++++
>  3 files changed, 83 insertions(+)
>  create mode 100644 hw/arm/dyn_sysbus_devtree.c
>  create mode 100644 include/hw/arm/dyn_sysbus_devtree.h
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 6088e53..bc5e014 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
> +obj-y += dyn_sysbus_devtree.o
>  
>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>  obj-$(CONFIG_DIGIC) += digic.o
> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
> new file mode 100644
> index 0000000..6375024
> --- /dev/null
> +++ b/hw/arm/dyn_sysbus_devtree.c
> @@ -0,0 +1,66 @@
> +#include "hw/arm/dyn_sysbus_devtree.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/device_tree.h"
> +
> +static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    Object *dev;
> +    SysBusDevice *sbdev;
> +    bool matched = false;
> +
> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> +    sbdev = (SysBusDevice *)dev;
> +
> +    if (!sbdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj,
> +                                    arm_sysbus_device_create_devtree, data);
> +    }
> +
> +    if (!matched) {

Who is going to set "matched", since it doesn't escape?

> +        error_report("Device %s is not supported by this machine yet.",
> +                     qdev_fw_name(DEVICE(dev)));
> +        exit(1);
> +    }
> +
> +    return 0;
> +}
> +
> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
> +                                 void *fdt, const char *intc)

Let's just call this arm_create_fdt_dynamic.

> +{
> +    gchar *node = g_strdup_printf("/platform@%"PRIx64,
> +                                  params->platform_bus_base);
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformDevtreeData data;
> +    Object *container;
> +    uint64_t addr = params->platform_bus_base;
> +    uint64_t size = params->platform_bus_size;
> +    int irq_start = params->platform_bus_first_irq;
> +
> +    /* Create a /platform node that we can put all devices into */
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +
> +    /* Our platform bus region is less than 32bit big, so 1 cell is enough for
> +       address and size */
> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, size);
> +
> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
> +
> +    /* Loop through all devices and create nodes for known ones */
> +    data.fdt = fdt;
> +    data.intc = intc;
> +    data.irq_start = irq_start;
> +    data.node = node;

Why does arm_sysbus_device_create_devtree need intc and irq_start?

> +
> +    container = container_get(qdev_get_machine(), "/peripheral");
> +    arm_sysbus_device_create_devtree(container, &data);
> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
> +    arm_sysbus_device_create_devtree(container, &data);
> +
> +    g_free(node);
> +}
> diff --git a/include/hw/arm/dyn_sysbus_devtree.h b/include/hw/arm/dyn_sysbus_devtree.h
> new file mode 100644
> index 0000000..b072365
> --- /dev/null
> +++ b/include/hw/arm/dyn_sysbus_devtree.h
> @@ -0,0 +1,16 @@
> +#ifndef HW_ARM_DYN_SYSBUS_DEVTREE_H
> +#define HW_ARM_DYN_SYSBUS_DEVTREE_H
> +
> +#include "hw/misc/dyn_sysbus_binding.h"
> +
> +typedef struct PlatformDevtreeData {
> +    void *fdt;
> +    const char *intc;
> +    int irq_start;
> +    const char *node;
> +} PlatformDevtreeData;
> +
> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
> +                                    void *fdt, const char *intc);
> +
> +#endif
> 

Re: [Qemu-devel] [PATCH v3 5/6] hw/arm/virt: new add_fdt_*_node functions

[Paolo Bonzini]

Il 09/09/2014 09:54, Eric Auger ha scritto:
> Create new functions:
> - add_fdt_uart_node
> - add_fdt_rtc_node
> - add_fdt_virtio_nodes

Actually they are fdt_add_uart_node etc.

> They will be used for dynamic sysbus instantiation.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v2 -> v3:
> reword title to avoid content violation
> ---
>  hw/arm/virt.c | 67 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d6fffc7..9085b88 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -351,18 +351,15 @@ static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
>      fdt_add_gic_node(vbi);
>  }
>  
> -static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static void fdt_add_uart_node(const VirtBoardInfo *vbi)
>  {
> -    char *nodename;
>      hwaddr base = vbi->memmap[VIRT_UART].base;
>      hwaddr size = vbi->memmap[VIRT_UART].size;
>      int irq = vbi->irqmap[VIRT_UART];
>      const char compat[] = "arm,pl011\0arm,primecell";
>      const char clocknames[] = "uartclk\0apb_pclk";
> +    char *nodename = g_strdup_printf("/pl011@%" PRIx64, base);
>  
> -    sysbus_create_simple("pl011", base, pic[irq]);
> -
> -    nodename = g_strdup_printf("/pl011@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>      /* Note that we can't use setprop_string because of the embedded NUL */
>      qemu_fdt_setprop(vbi->fdt, nodename, "compatible",
> @@ -379,17 +376,23 @@ static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
>      g_free(nodename);
>  }
>  
> -static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    hwaddr base = vbi->memmap[VIRT_UART].base;
> +    int irq = vbi->irqmap[VIRT_UART];
> +
> +    sysbus_create_simple("pl011", base, pic[irq]);
> +    fdt_add_uart_node(vbi);
> +}
> +
> +static void fdt_add_rtc_node(const VirtBoardInfo *vbi)
>  {
> -    char *nodename;
>      hwaddr base = vbi->memmap[VIRT_RTC].base;
>      hwaddr size = vbi->memmap[VIRT_RTC].size;
>      int irq = vbi->irqmap[VIRT_RTC];
>      const char compat[] = "arm,pl031\0arm,primecell";
> +    char *nodename = g_strdup_printf("/pl031@%" PRIx64, base);
>  
> -    sysbus_create_simple("pl031", base, pic[irq]);
> -
> -    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>      qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
> @@ -402,22 +405,20 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
>      g_free(nodename);
>  }
>  
> -static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
>  {
> -    int i;
> -    hwaddr size = vbi->memmap[VIRT_MMIO].size;
> +    hwaddr base = vbi->memmap[VIRT_RTC].base;
> +    int irq = vbi->irqmap[VIRT_RTC];
>  
> -    /* Note that we have to create the transports in forwards order
> -     * so that command line devices are inserted lowest address first,
> -     * and then add dtb nodes in reverse order so that they appear in
> -     * the finished device tree lowest address first.
> -     */
> -    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
> -        int irq = vbi->irqmap[VIRT_MMIO] + i;
> -        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
> +    sysbus_create_simple("pl031", base, pic[irq]);
>  
> -        sysbus_create_simple("virtio-mmio", base, pic[irq]);
> -    }
> +    fdt_add_rtc_node(vbi);
> +}
> +
> +static void fdt_add_virtio_nodes(const VirtBoardInfo *vbi)
> +{
> +    int i;
> +    hwaddr size = vbi->memmap[VIRT_MMIO].size;
>  
>      for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
>          char *nodename;
> @@ -437,6 +438,26 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>      }
>  }
>  
> +static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    int i;
> +    hwaddr size = vbi->memmap[VIRT_MMIO].size;
> +
> +    /* Note that we have to create the transports in forwards order
> +     * so that command line devices are inserted lowest address first,
> +     * and then add dtb nodes in reverse order so that they appear in
> +     * the finished device tree lowest address first.
> +     */
> +    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
> +        int irq = vbi->irqmap[VIRT_MMIO] + i;
> +        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
> +
> +        sysbus_create_simple("virtio-mmio", base, pic[irq]);
> +    }
> +
> +    fdt_add_virtio_nodes(vbi);
> +}
> +
>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>  {
>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
> 

Re: [Qemu-devel] [PATCH v3 6/6] hw/arm/virt: Support dynamically spawned sysbus devices

[Paolo Bonzini]

Il 09/09/2014 09:54, Eric Auger ha scritto:
> Allows sysbus devices to be instantiated from command line by
> using -device option
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v2 -> v3
> - renaming of arm_platform_bus_create_devtree and arm_load_dtb
> - add copyright in hw/arm/dyn_sysbus_devtree.c
> 
> v1 -> v2:
> - remove useless vfio-platform.h include file
> - s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
> - use dyn_sysbus_binding and dyn_sysbus_devtree
> - dynamic sysbus platform buse size shrinked to 4MB and
>   moved between RTC and MMIO
> 
> v1:
> 
> Inspired from what Alex Graf did in ppc e500
> https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
> ---
>  hw/arm/dyn_sysbus_devtree.c | 26 +++++++++++++++++++
>  hw/arm/virt.c               | 62 +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
> index 6375024..61e5b5f 100644
> --- a/hw/arm/dyn_sysbus_devtree.c
> +++ b/hw/arm/dyn_sysbus_devtree.c
> @@ -1,7 +1,30 @@
> +/*
> + * ARM Platform Bus device tree generation helpers 
> + *
> + * Copyright (c) 2014 Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/arm/dyn_sysbus_devtree.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/device_tree.h"
>  
> +/**
> + * arm_sysbus_device_create_devtree - create the node of devices
> + * attached to the platform bus
> + */
>  static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>  {
>      PlatformDevtreeData *data = opaque;
> @@ -27,6 +50,9 @@ static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>      return 0;
>  }
>  
> +/**
> + * arm_platform_bus_create_devtree - create the platform bus node
> + */
>  void arm_platform_bus_create_devtree(DynSysbusParams *params,
>                                   void *fdt, const char *intc)
>  {

All this should go in patch 2.  For the documentation comments, please
follow the model of include/hw/memory.h (including putting the
documentation in the header for public functions).

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9085b88..16acf44 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -40,6 +40,8 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
> +#include "hw/misc/dyn_sysbus_binding.h"
> +#include "hw/arm/dyn_sysbus_devtree.h"
>  
>  #define NUM_VIRTIO_TRANSPORTS 32
>  
> @@ -57,6 +59,14 @@
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>  
> +#define MACHVIRT_PLATFORM_BASE         0x9400000
> +#define MACHVIRT_PLATFORM_SIZE         (4ULL * 1024 * 1024) /* 4 MB */
> +#define MACHVIRT_PLATFORM_PAGE_SHIFT   12
> +#define MACHVIRT_PLATFORM_SIZE_PAGES   (MACHVIRT_PLATFORM_SIZE >> \
> +                                    MACHVIRT_PLATFORM_PAGE_SHIFT)
> +#define MACHVIRT_PLATFORM_FIRST_IRQ    48
> +#define MACHVIRT_PLATFORM_NUM_IRQS     20
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -66,6 +76,7 @@ enum {
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> +    VIRT_PLATFORM,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -105,16 +116,27 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
> +    [VIRT_PLATFORM] = {MACHVIRT_PLATFORM_BASE , MACHVIRT_PLATFORM_SIZE},
>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      /* 0x10000000 .. 0x40000000 reserved for PCI */
> -    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> +    [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>  };
>  
>  static const int a15irqmap[] = {
>      [VIRT_UART] = 1,
>      [VIRT_RTC] = 2,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
> +    [VIRT_PLATFORM] = MACHVIRT_PLATFORM_FIRST_IRQ,
> +};
> +
> +static DynSysbusParams dyn_sysbus_platform_params = {
> +    .has_platform_bus = true,
> +    .platform_bus_base = MACHVIRT_PLATFORM_BASE,
> +    .platform_bus_size = MACHVIRT_PLATFORM_SIZE,
> +    .platform_bus_first_irq = MACHVIRT_PLATFORM_FIRST_IRQ,
> +    .platform_bus_num_irqs = MACHVIRT_PLATFORM_NUM_IRQS,
> +    .page_shift = MACHVIRT_PLATFORM_PAGE_SHIFT,
>  };
>  
>  static VirtBoardInfo machines[] = {
> @@ -458,6 +480,18 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>      fdt_add_virtio_nodes(vbi);
>  }
>  
> +static void machvirt_prep_device_tree(VirtBoardInfo *vbi)
> +{
> +    create_fdt(vbi);
> +    fdt_add_timer_nodes(vbi);
> +    fdt_add_cpu_nodes(vbi);
> +    fdt_add_psci_node(vbi);
> +    fdt_add_gic_node(vbi);
> +    fdt_add_uart_node(vbi);
> +    fdt_add_rtc_node(vbi);
> +    fdt_add_virtio_nodes(vbi);
> +}
> +
>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>  {
>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
> @@ -466,14 +500,28 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>      return board->fdt;
>  }
>  
> +static void machvirt_reset_device_tree(void *opaque)
> +{
> +    VirtBoardInfo *board = (VirtBoardInfo *)opaque;
> +    struct arm_boot_info *info = &board->bootinfo;
> +    hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + info->initrd_size,
> +                                     4096);
> +    machvirt_prep_device_tree(board);
> +    arm_platform_bus_create_devtree(&dyn_sysbus_platform_params,
> +                                board->fdt, "/intc");

Why do you have to recreate the device tree every time (as opposed to
just doing the load)?  And if arm_load_dtb used rom_add_blob_fixed
instead of cpu_physical_memory_write, you wouldn't need a reset hook at all.

Paolo

> +    arm_load_dtb(dtb_start, info);
> +}
> +
>  static void machvirt_init(MachineState *machine)
>  {
> -    qemu_irq pic[NUM_IRQS];
> +    qemu_irq *pic = g_new(qemu_irq, NUM_IRQS);
>      MemoryRegion *sysmem = get_system_memory();
>      int n;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      const char *cpu_model = machine->cpu_model;
>      VirtBoardInfo *vbi;
> +    DynSysbusNotifier *notifier;
>  
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -547,6 +595,13 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>  
> +    notifier = g_new(DynSysbusNotifier, 1);
> +    notifier->notifier.notify = platform_bus_init_notify;
> +    notifier->address_space_mem = sysmem;
> +    notifier->mpic = pic;
> +    notifier->params = dyn_sysbus_platform_params;
> +    qemu_add_machine_init_done_notifier(&notifier->notifier);
> +
>      vbi->bootinfo.ram_size = machine->ram_size;
>      vbi->bootinfo.kernel_filename = machine->kernel_filename;
>      vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -556,6 +611,8 @@ static void machvirt_init(MachineState *machine)
>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>      vbi->bootinfo.get_dtb = machvirt_dtb;
>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
> +
> +    qemu_register_reset(machvirt_reset_device_tree, vbi);
>  }
>  
>  static QEMUMachine machvirt_a15_machine = {
> @@ -563,6 +620,7 @@ static QEMUMachine machvirt_a15_machine = {
>      .desc = "ARM Virtual Machine",
>      .init = machvirt_init,
>      .max_cpus = 8,
> +    .has_dynamic_sysbus = true,
>  };
>  
>  static void machvirt_machine_init(void)
> 

Re: [Qemu-devel] [PATCH v3 6/6] hw/arm/virt: Support dynamically spawned sysbus devices

[Peter Maydell]

On 9 September 2014 12:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> And if arm_load_dtb used rom_add_blob_fixed
> instead of cpu_physical_memory_write, you wouldn't need a reset hook at all.

We need to do this anyway, because currently we don't do
anything to ensure the DTB hangs around for the OS to
find again on reset, even in the simple "just boot Linux"
setups.

-- PMM

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Peter Crosthwaite]

On Tue, Sep 9, 2014 at 9:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> This module will be used by ARM machine files to generate
>> device tree nodes of dynamically instantiated sysbus devices (ie.
>> those instantiated with -device option).
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v2 -> v3:
>> - add arm_ prefix
>> - arm_sysbus_device_create_devtree becomes static
>>
>> v1 -> v2:
>> - Code moved in an arch specific file to accomodate architecture
>>   dependent specificities.
>> - remove platform_bus_base from PlatformDevtreeData
>>
>> v1: code originally written by Alex Graf in e500.c and reused for ARM
>>     [Eric Auger]
>>     code originally moved in hw/misc/platform_devices and device itself
>> ---
>>  hw/arm/Makefile.objs                |  1 +
>>  hw/arm/dyn_sysbus_devtree.c         | 66 +++++++++++++++++++++++++++++++++++++
>
> File names in QEMU typically use a dash instead of an underscore.  Also,
> I see the "fdt" moniker used more often than "devtree" (ouch, I checked
> now and it's 7 vs. 851 uses :)).  So what about hw/arm/sysbus-fdt.c?
>

Yes, "devtree" is legacy. Please use fdt or dtb as appropriate.

Regards,
Peter

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Eric Auger]

On 09/09/2014 12:56 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> This new module implements routines which help in dynamic device
>> binding (mmio regions, irq). They are supposed to be used by machine
>> files that support dynamic sysbus instantiation.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - platform_devices renamed into dyn_sysbus_binding
> 
> Please use "dynamic" instead of dyn.
Hi Paolo,
Thanks for the quick review.
I will remove such abbreviation.
> 
>> - PlatformParams renamed into DynSysbusParams
>> - PlatformBusNotifier renamed into DynSysbusNotifier
>> - platform_bus_map_irq, platform_bus_map_mmio, sysbus_device_check,
>>   platform_bus_init become static
>> - PlatformBusInitData becomes private to the module
>> - page_shift becomes a member of DynSysbusParams
>>
>> v1: Dynamic sysbus device allocation fully written by Alex Graf.
>> Those functions were initially in ppc e500 machine file. Now moved to a
>> separate module.
>> PPCE500Params is replaced by a generic struct named PlatformParams
>> ---
>>  hw/misc/Makefile.objs                |   1 +
>>  hw/misc/dyn_sysbus_binding.c         | 163 +++++++++++++++++++++++++++++++++++
> 
> Please put these in hw/core/sysbus.c and include/hw/sysbus.h, and rename
> everything called "Platform" or "platform_" to "Sysbus" or "sysbus_".
OK
> 
>> +void platform_bus_init_notify(Notifier *notifier, void *data)
>> +{
>> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
>> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
>> +}
> 
> Please provide a wrapper like sysbus_register_dynamic that takes the
> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
> and DynSysbusNotifier can remain hidden within the .c file.
Sorry I do not catch what you mean here. platform_bus_init_notify &
DynSysbusNotifier are currently used in virt.c to initialize & register
the machine_init_done_notifier.
> 
> Also, why not move the address_space_mem and mpic into the parameters
> struct?
Yes sure

Best Regards

Eric
> 
> Paolo
> 
>>  include/hw/misc/dyn_sysbus_binding.h |  24 ++++++
>>  3 files changed, 188 insertions(+)
>>  create mode 100644 hw/misc/dyn_sysbus_binding.c
>>  create mode 100644 include/hw/misc/dyn_sysbus_binding.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..86f6243 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -41,3 +41,4 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
>>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>>  
>>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>> +obj-y += dyn_sysbus_binding.o
>> diff --git a/hw/misc/dyn_sysbus_binding.c b/hw/misc/dyn_sysbus_binding.c
>> new file mode 100644
>> index 0000000..0f34f0b
>> --- /dev/null
>> +++ b/hw/misc/dyn_sysbus_binding.c
>> @@ -0,0 +1,163 @@
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +#include "qemu/error-report.h"
>> +
>> +typedef struct PlatformBusInitData {
>> +    unsigned long *used_irqs;
>> +    unsigned long *used_mem;
>> +    MemoryRegion *mem;
>> +    qemu_irq *irqs;
>> +    int device_count;
>> +    DynSysbusParams *params;
>> +} PlatformBusInitData;
>> +
>> +
>> +static int platform_bus_map_irq(DynSysbusParams *params,
>> +                                SysBusDevice *sbdev,
>> +                                int n, unsigned long *used_irqs,
>> +                                qemu_irq *platform_irqs)
>> +{
>> +    int max_irqs = params->platform_bus_num_irqs;
>> +    char *prop = g_strdup_printf("irq[%d]", n);
>> +    int irqn = object_property_get_int(OBJECT(sbdev), prop, NULL);
>> +
>> +    if (irqn == SYSBUS_DYNAMIC) {
>> +        /* Find the first available IRQ */
>> +        irqn = find_first_zero_bit(used_irqs, max_irqs);
>> +    }
>> +
>> +    if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
>> +        hw_error("IRQ %d is already allocated or no free IRQ left", irqn);
>> +    }
>> +
>> +    sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
>> +    object_property_set_int(OBJECT(sbdev), irqn, prop, NULL);
>> +
>> +    g_free(prop);
>> +    return 0;
>> +}
>> +
>> +static int platform_bus_map_mmio(DynSysbusParams *params,
>> +                                 SysBusDevice *sbdev,
>> +                                 int n, unsigned long *used_mem,
>> +                                 MemoryRegion *pmem)
>> +{
>> +    MemoryRegion *device_mem = sbdev->mmio[n].memory;
>> +    uint64_t size = memory_region_size(device_mem);
>> +    uint64_t page_size = (1 << params->page_shift);
>> +    uint64_t page_mask = page_size - 1;
>> +    uint64_t size_pages = (size + page_mask) >> params->page_shift;
>> +    uint64_t max_size = params->platform_bus_size;
>> +    uint64_t max_pages = max_size >> params->page_shift;
>> +    char *prop = g_strdup_printf("mmio[%d]", n);
>> +    hwaddr addr = object_property_get_int(OBJECT(sbdev), prop, NULL);
>> +    int page;
>> +    int i;
>> +
>> +    page = addr >> params->page_shift;
>> +    if (addr == SYSBUS_DYNAMIC) {
>> +        uint64_t size_pages_align;
>> +
>> +        /* Align the region to at least its own size granularity */
>> +        if (is_power_of_2(size_pages)) {
>> +            size_pages_align = size_pages;
>> +        } else {
>> +            size_pages_align = pow2floor(size_pages) << 1;
>> +        }
>> +
>> +        /* Find the first available region that fits */
>> +        page = bitmap_find_next_zero_area(used_mem, max_pages, 0, size_pages,
>> +                                          size_pages_align);
>> +
>> +        addr = (uint64_t)page << params->page_shift;
>> +    }
>> +
>> +    if (page >= max_pages || test_bit(page, used_mem) ||
>> +        (find_next_bit(used_mem, max_pages, page) < size_pages)) {
>> +        hw_error("Memory [%"PRIx64":%"PRIx64" is already allocated or "
>> +                 "no slot left", addr, size);
>> +    }
>> +
>> +    for (i = page; i < (page + size_pages); i++) {
>> +        set_bit(i, used_mem);
>> +    }
>> +
>> +    memory_region_add_subregion(pmem, addr, device_mem);
>> +    sbdev->mmio[n].addr = addr;
>> +    object_property_set_int(OBJECT(sbdev), addr, prop, NULL);
>> +
>> +    g_free(prop);
>> +    return 0;
>> +}
>> +
>> +static int sysbus_device_check(Object *obj, void *opaque)
>> +{
>> +    PlatformBusInitData *init = opaque;
>> +    Object *dev;
>> +    SysBusDevice *sbdev;
>> +    int i;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>> +    sbdev = (SysBusDevice *)dev;
>> +
>> +    if (!sbdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj, sysbus_device_check, opaque);
>> +    }
>> +
>> +    /* Connect sysbus device to virtual platform bus */
>> +    for (i = 0; i < sbdev->num_irq; i++) {
>> +        if (!sbdev->irqp[i]) {
>> +            /* This IRQ is an incoming IRQ, we can't wire those here */
>> +            continue;
>> +        }
>> +        platform_bus_map_irq(init->params, sbdev, i,
>> +                             init->used_irqs, init->irqs);
>> +    }
>> +
>> +    for (i = 0; i < sbdev->num_mmio; i++) {
>> +        platform_bus_map_mmio(init->params, sbdev, i,
>> +                              init->used_mem, init->mem);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void platform_bus_init(DynSysbusParams *params,
>> +                       MemoryRegion *address_space_mem,
>> +                       qemu_irq *mpic)
>> +{
>> +    uint64_t max_size = params->platform_bus_size;
>> +    uint64_t max_pages = max_size >> params->page_shift;
>> +    DECLARE_BITMAP(used_irqs, params->platform_bus_num_irqs);
>> +    DECLARE_BITMAP(used_mem, max_pages);
>> +    MemoryRegion *platform_region = g_new(MemoryRegion, 1);
>> +    Object *container;
>> +    PlatformBusInitData init = {
>> +        .used_irqs = used_irqs,
>> +        .used_mem = used_mem,
>> +        .mem = platform_region,
>> +        .irqs = &mpic[params->platform_bus_first_irq],
>> +        .params = params,
>> +    };
>> +
>> +    memory_region_init(platform_region, NULL, "platform devices",
>> +                       params->platform_bus_size);
>> +
>> +    bitmap_clear(used_irqs, 0, params->platform_bus_num_irqs);
>> +    bitmap_clear(used_mem, 0, max_pages);
>> +
>> +    /* Loop through all sysbus devices that were spawened outside the machine */
>> +    container = container_get(qdev_get_machine(), "/peripheral");
>> +    sysbus_device_check(container, &init);
>> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
>> +    sysbus_device_check(container, &init);
>> +
>> +    memory_region_add_subregion(address_space_mem, params->platform_bus_base,
>> +                                platform_region);
>> +}
>> +
>> +void platform_bus_init_notify(Notifier *notifier, void *data)
>> +{
>> +    DynSysbusNotifier *pn = (DynSysbusNotifier *)notifier;
>> +    platform_bus_init(&pn->params, pn->address_space_mem, pn->mpic);
>> +}
>> diff --git a/include/hw/misc/dyn_sysbus_binding.h b/include/hw/misc/dyn_sysbus_binding.h
>> new file mode 100644
>> index 0000000..961c9c7
>> --- /dev/null
>> +++ b/include/hw/misc/dyn_sysbus_binding.h
>> @@ -0,0 +1,24 @@
>> +#ifndef HW_MISC_PLATFORM_DEVICES_H
>> +#define HW_MISC_PLATFORM_DEVICES_H
>> +
>> +#include "qemu-common.h"
>> +#include "hw/sysbus.h"
>> +
>> +typedef struct {
>> +    bool has_platform_bus;
>> +    hwaddr platform_bus_base;
>> +    hwaddr platform_bus_size;
>> +    int platform_bus_first_irq;
>> +    int platform_bus_num_irqs;
>> +    int page_shift;
>> +} DynSysbusParams;
>> +
>> +typedef struct DynSysbusNotifier {
>> +    Notifier notifier;
>> +    MemoryRegion *address_space_mem;
>> +    qemu_irq *mpic;
>> +    DynSysbusParams params;
>> +} DynSysbusNotifier;
>> +
>> +void platform_bus_init_notify(Notifier *notifier, void *data);
>> +#endif
>>
> 

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Eric Auger]

On 09/09/2014 01:04 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> This module will be used by ARM machine files to generate
>> device tree nodes of dynamically instantiated sysbus devices (ie.
>> those instantiated with -device option).
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v2 -> v3:
>> - add arm_ prefix
>> - arm_sysbus_device_create_devtree becomes static
>>
>> v1 -> v2:
>> - Code moved in an arch specific file to accomodate architecture
>>   dependent specificities.
>> - remove platform_bus_base from PlatformDevtreeData
>>
>> v1: code originally written by Alex Graf in e500.c and reused for ARM
>>     [Eric Auger]
>>     code originally moved in hw/misc/platform_devices and device itself
>> ---
>>  hw/arm/Makefile.objs                |  1 +
>>  hw/arm/dyn_sysbus_devtree.c         | 66 +++++++++++++++++++++++++++++++++++++
> 
> File names in QEMU typically use a dash instead of an underscore.  Also,
> I see the "fdt" moniker used more often than "devtree" (ouch, I checked
> now and it's 7 vs. 851 uses :)).  So what about hw/arm/sysbus-fdt.c?

Sure I will correct _ and rename.
> 
>>  include/hw/arm/dyn_sysbus_devtree.h | 16 +++++++++
>>  3 files changed, 83 insertions(+)
>>  create mode 100644 hw/arm/dyn_sysbus_devtree.c
>>  create mode 100644 include/hw/arm/dyn_sysbus_devtree.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 6088e53..bc5e014 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
>>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>>  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>> +obj-y += dyn_sysbus_devtree.o
>>  
>>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>>  obj-$(CONFIG_DIGIC) += digic.o
>> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
>> new file mode 100644
>> index 0000000..6375024
>> --- /dev/null
>> +++ b/hw/arm/dyn_sysbus_devtree.c
>> @@ -0,0 +1,66 @@
>> +#include "hw/arm/dyn_sysbus_devtree.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/device_tree.h"
>> +
>> +static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    Object *dev;
>> +    SysBusDevice *sbdev;
>> +    bool matched = false;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>> +    sbdev = (SysBusDevice *)dev;
>> +
>> +    if (!sbdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj,
>> +                                    arm_sysbus_device_create_devtree, data);
>> +    }

When we add support for a dynamically instantiable device we add
something like

    if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
        create_devtree_etsec(ETSEC_COMMON(dev), data);
        matched = true;
    }
>> +
>> +    if (!matched) {
> 
> Who is going to set "matched", since it doesn't escape?



> 
>> +        error_report("Device %s is not supported by this machine yet.",
>> +                     qdev_fw_name(DEVICE(dev)));
>> +        exit(1);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
>> +                                 void *fdt, const char *intc)
> 
> Let's just call this arm_create_fdt_dynamic.
OK
> 
>> +{
>> +    gchar *node = g_strdup_printf("/platform@%"PRIx64,
>> +                                  params->platform_bus_base);
>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>> +    PlatformDevtreeData data;
>> +    Object *container;
>> +    uint64_t addr = params->platform_bus_base;
>> +    uint64_t size = params->platform_bus_size;
>> +    int irq_start = params->platform_bus_first_irq;
>> +
>> +    /* Create a /platform node that we can put all devices into */
>> +    qemu_fdt_add_subnode(fdt, node);
>> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
>> +
>> +    /* Our platform bus region is less than 32bit big, so 1 cell is enough for
>> +       address and size */
>> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, size);
>> +
>> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
>> +
>> +    /* Loop through all devices and create nodes for known ones */
>> +    data.fdt = fdt;
>> +    data.intc = intc;
>> +    data.irq_start = irq_start;
>> +    data.node = node;
> 
> Why does arm_sysbus_device_create_devtree need intc and irq_start?

irq_start: needed because when the "interrupts" property is set for the
leaf component the irq number is irq_start +
object_property_get_int(obj, "irq[i]", NULL)
irq[i] being in [0, params->platform_bus_num_irqs]

intc: this was in case the leaf component would use "interrupt-parent"
prop. I miss experience on device trees and I don't know if it make
sense the leaf component uses a different interrupt controller than the
parent platform bus or if such property is mandatory in some cases.
Maybe not needed indeed.

Best Regards

Eric
> 
>> +
>> +    container = container_get(qdev_get_machine(), "/peripheral");
>> +    arm_sysbus_device_create_devtree(container, &data);
>> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
>> +    arm_sysbus_device_create_devtree(container, &data);
>> +
>> +    g_free(node);
>> +}
>> diff --git a/include/hw/arm/dyn_sysbus_devtree.h b/include/hw/arm/dyn_sysbus_devtree.h
>> new file mode 100644
>> index 0000000..b072365
>> --- /dev/null
>> +++ b/include/hw/arm/dyn_sysbus_devtree.h
>> @@ -0,0 +1,16 @@
>> +#ifndef HW_ARM_DYN_SYSBUS_DEVTREE_H
>> +#define HW_ARM_DYN_SYSBUS_DEVTREE_H
>> +
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +
>> +typedef struct PlatformDevtreeData {
>> +    void *fdt;
>> +    const char *intc;
>> +    int irq_start;
>> +    const char *node;
>> +} PlatformDevtreeData;
>> +
>> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
>> +                                    void *fdt, const char *intc);
>> +
>> +#endif
>>
> 

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 09/09/2014 17:25, Eric Auger ha scritto:
>> > 
>> > Please provide a wrapper like sysbus_register_dynamic that takes the
>> > params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>> > and DynSysbusNotifier can remain hidden within the .c file.
> Sorry I do not catch what you mean here. platform_bus_init_notify &
> DynSysbusNotifier are currently used in virt.c to initialize & register
> the machine_init_done_notifier.

Yeah, please do the registration in sysbus.c, not in virt.c.  There is
no reason to make the platform_bus_init_notify+DynSysbusNotifier
interface public.  The code in sysbus.c can fill in the fields.

Paolo

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Peter Maydell]

On 9 September 2014 16:56, Eric Auger <eric.auger@linaro.org> wrote:
> irq_start: needed because when the "interrupts" property is set for the
> leaf component the irq number is irq_start +
> object_property_get_int(obj, "irq[i]", NULL)
> irq[i] being in [0, params->platform_bus_num_irqs]
>
> intc: this was in case the leaf component would use "interrupt-parent"
> prop. I miss experience on device trees and I don't know if it make
> sense the leaf component uses a different interrupt controller than the
> parent platform bus or if such property is mandatory in some cases.
> Maybe not needed indeed.

Somewhat tangential, but for passthrough devices how
do we decide whether the device tree node needs to
mark the interrupt as edge or level triggered?
Presumably this is going to be a "depends on what
the passed through hardware is" thing...

-- PMM

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Paolo Bonzini]

Il 09/09/2014 17:56, Eric Auger ha scritto:
>>> >> +    if (!sbdev) {
>>> >> +        /* Container, traverse it for children */
>>> >> +        return object_child_foreach(obj,
>>> >> +                                    arm_sysbus_device_create_devtree, data);
>>> >> +    }
> When we add support for a dynamically instantiable device we add
> something like
> 
>     if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
>         create_devtree_etsec(ETSEC_COMMON(dev), data);
>         matched = true;
>     }
>>> >> +
>>> >> +    if (!matched) {
>> > 
>> > Who is going to set "matched", since it doesn't escape?
> 
> 

That's not part of this patch though, right?  So this code for now is
dead.  Please remove the dead code if it is not used in this series.

We really should make that an interface, so that the code can do just

    if (object_dynamic_cast(obj, TYPE_FDT_BUILDER)) {
        fdt_builder_create_fdt(FDT_BUILDER(dev), data);
    } else {
        ...
    }

(and so can the generic virt.c code) but that can come later.

>> Why does arm_sysbus_device_create_devtree need intc and irq_start?
> 
> irq_start: needed because when the "interrupts" property is set for the
> leaf component the irq number is irq_start +
> object_property_get_int(obj, "irq[i]", NULL)
> irq[i] being in [0, params->platform_bus_num_irqs]

Ah, it's passed to the not-yet-existing create_* functions.

> intc: this was in case the leaf component would use "interrupt-parent"
> prop. I miss experience on device trees and I don't know if it make
> sense the leaf component uses a different interrupt controller than the
> parent platform bus or if such property is mandatory in some cases.
> Maybe not needed indeed.

No idea, sorry.

Paolo

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Eric Auger]

On 09/09/2014 06:00 PM, Peter Maydell wrote:
> On 9 September 2014 16:56, Eric Auger <eric.auger@linaro.org> wrote:
>> irq_start: needed because when the "interrupts" property is set for the
>> leaf component the irq number is irq_start +
>> object_property_get_int(obj, "irq[i]", NULL)
>> irq[i] being in [0, params->platform_bus_num_irqs]
>>
>> intc: this was in case the leaf component would use "interrupt-parent"
>> prop. I miss experience on device trees and I don't know if it make
>> sense the leaf component uses a different interrupt controller than the
>> parent platform bus or if such property is mandatory in some cases.
>> Maybe not needed indeed.
> 
> Somewhat tangential, but for passthrough devices how
> do we decide whether the device tree node needs to
> mark the interrupt as edge or level triggered?
> Presumably this is going to be a "depends on what
> the passed through hardware is" thing...

Hi Peter,

Yes I think this is one of the reasons why Alex insisted on having the
device node creation specialized for each device and not common to VFIO
devices.

Besides, I did not looked sufficiently at Antonios' patch "VFIO:
PLATFORM: Return device tree info for a platform device node" but I
guess this would enable to retrieve the property values.
https://www.mail-archive.com/kvm@vger.kernel.org/msg106282.html.

But I remember I read somewhere such interrupts properties were not
always correct in dt?

Best Regards

Eric
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Eric Auger]

On 09/09/2014 06:03 PM, Paolo Bonzini wrote:
> Il 09/09/2014 17:56, Eric Auger ha scritto:
>>>>>> +    if (!sbdev) {
>>>>>> +        /* Container, traverse it for children */
>>>>>> +        return object_child_foreach(obj,
>>>>>> +                                    arm_sysbus_device_create_devtree, data);
>>>>>> +    }
>> When we add support for a dynamically instantiable device we add
>> something like
>>
>>     if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
>>         create_devtree_etsec(ETSEC_COMMON(dev), data);
>>         matched = true;
>>     }
>>>>>> +
>>>>>> +    if (!matched) {
>>>>
>>>> Who is going to set "matched", since it doesn't escape?
>>
>>
> 
> That's not part of this patch though, right?

right
  So this code for now is
> dead.  Please remove the dead code if it is not used in this series.
> 
> We really should make that an interface, so that the code can do just
> 
>     if (object_dynamic_cast(obj, TYPE_FDT_BUILDER)) {
>         fdt_builder_create_fdt(FDT_BUILDER(dev), data);
>     } else {
>         ...
>     }
> 
> (and so can the generic virt.c code) but that can come later.

OK I will correct
> 
>>> Why does arm_sysbus_device_create_devtree need intc and irq_start?
>>
>> irq_start: needed because when the "interrupts" property is set for the
>> leaf component the irq number is irq_start +
>> object_property_get_int(obj, "irq[i]", NULL)
>> irq[i] being in [0, params->platform_bus_num_irqs]
> 
> Ah, it's passed to the not-yet-existing create_* functions.
yep
> 
>> intc: this was in case the leaf component would use "interrupt-parent"
>> prop. I miss experience on device trees and I don't know if it make
>> sense the leaf component uses a different interrupt controller than the
>> parent platform bus or if such property is mandatory in some cases.
>> Maybe not needed indeed.
> 
> No idea, sorry.
- Eric
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Eric Auger]

On 09/09/2014 05:59 PM, Paolo Bonzini wrote:
> Il 09/09/2014 17:25, Eric Auger ha scritto:
>>>>
>>>> Please provide a wrapper like sysbus_register_dynamic that takes the
>>>> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>>>> and DynSysbusNotifier can remain hidden within the .c file.
>> Sorry I do not catch what you mean here. platform_bus_init_notify &
>> DynSysbusNotifier are currently used in virt.c to initialize & register
>> the machine_init_done_notifier.
> 
> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
> no reason to make the platform_bus_init_notify+DynSysbusNotifier
> interface public.  The code in sysbus.c can fill in the fields.
OK I will investigate that.

Thanks

Eric
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

On 09.09.14 17:59, Paolo Bonzini wrote:
> Il 09/09/2014 17:25, Eric Auger ha scritto:
>>>>
>>>> Please provide a wrapper like sysbus_register_dynamic that takes the
>>>> params/address_space_mem/mpic as parameter.  platform_bus_init_notify
>>>> and DynSysbusNotifier can remain hidden within the .c file.
>> Sorry I do not catch what you mean here. platform_bus_init_notify &
>> DynSysbusNotifier are currently used in virt.c to initialize & register
>> the machine_init_done_notifier.
> 
> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
> no reason to make the platform_bus_init_notify+DynSysbusNotifier
> interface public.  The code in sysbus.c can fill in the fields.

Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
pseudo-bus that we put all devices onto that we consider unsorted.

Platform bus is a machine representation of an actual bus that devices
are attached to. These devices usually are sysbus devices.


Alex

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 11:31, Alexander Graf ha scritto:
>> > Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>> > no reason to make the platform_bus_init_notify+DynSysbusNotifier
>> > interface public.  The code in sysbus.c can fill in the fields.
> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
> pseudo-bus that we put all devices onto that we consider unsorted.
> 
> Platform bus is a machine representation of an actual bus that devices
> are attached to. These devices usually are sysbus devices.

Is there any difference between the two?

Take a machine that has two chips, a SoC that does everything except
USB, and a USB controller chip.

Strictly speaking the USB controller chip would be on a "platform bus",
but we would likely put it on sysbus.

Why should it matter whether the devices are static or dynamic, for the
sake of calling something the "system" or the "platform" bus?  I would
say that QEMU calls "sysbus" the platform bus.

Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
in ARM) should probably not be in sysbus at all, and should attach
directly to the CPU address space.  But that is a quirk in the modeling
of those devices, it shouldn't mean that sysbus is not a "platform" bus.

Paolo

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

On 10.09.14 11:43, Paolo Bonzini wrote:
> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>> interface public.  The code in sysbus.c can fill in the fields.
>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>> pseudo-bus that we put all devices onto that we consider unsorted.
>>
>> Platform bus is a machine representation of an actual bus that devices
>> are attached to. These devices usually are sysbus devices.
> 
> Is there any difference between the two?
> 
> Take a machine that has two chips, a SoC that does everything except
> USB, and a USB controller chip.
> 
> Strictly speaking the USB controller chip would be on a "platform bus",
> but we would likely put it on sysbus.
> 
> Why should it matter whether the devices are static or dynamic, for the
> sake of calling something the "system" or the "platform" bus?  I would
> say that QEMU calls "sysbus" the platform bus.
> 
> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
> in ARM) should probably not be in sysbus at all, and should attach
> directly to the CPU address space.  But that is a quirk in the modeling
> of those devices, it shouldn't mean that sysbus is not a "platform" bus.

On e500 for example, we have a predefined CCSR region. That is a machine
defined "platform bus". The offsets inside that region are strictly
defined by the spec.

Now take the serial ports. We have space for 2 serial ports inside of
that CCSR region. We can spawn these 2 ports in the machine file based
on -serial, but if you want to spawn them with -device, how do you tell
the machine whether they should go into the "big bucket platform bus" or
the "CCSR platform bus"?

In fact, thinking about this a bit more, maybe we should just have an
actual bus structure. Then we could have the legacy "big bucket" sysbus
bus that nobody may ever dynamically put devices into. For CCSR we could
create another bucket that the machine file can control where each
device goes and can also detect if a device doesn't fit. And then we
just declare the virt "platform bus" sysbus the default bus for cmdline
-device sysbusdevice and everything resolves automatically.


Alex

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 11:56, Alexander Graf ha scritto:
> 
> 
> On 10.09.14 11:43, Paolo Bonzini wrote:
>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>
>>> Platform bus is a machine representation of an actual bus that devices
>>> are attached to. These devices usually are sysbus devices.
>>
>> Is there any difference between the two?
>>
>> Take a machine that has two chips, a SoC that does everything except
>> USB, and a USB controller chip.
>>
>> Strictly speaking the USB controller chip would be on a "platform bus",
>> but we would likely put it on sysbus.
>>
>> Why should it matter whether the devices are static or dynamic, for the
>> sake of calling something the "system" or the "platform" bus?  I would
>> say that QEMU calls "sysbus" the platform bus.
>>
>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>> in ARM) should probably not be in sysbus at all, and should attach
>> directly to the CPU address space.  But that is a quirk in the modeling
>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
> 
> On e500 for example, we have a predefined CCSR region. That is a machine
> defined "platform bus". The offsets inside that region are strictly
> defined by the spec.
> 
> Now take the serial ports. We have space for 2 serial ports inside of
> that CCSR region. We can spawn these 2 ports in the machine file based
> on -serial, but if you want to spawn them with -device, how do you tell
> the machine whether they should go into the "big bucket platform bus" or
> the "CCSR platform bus"?

Two possibilities:

1) you would use two instances of sysbus (one default, one created by
the board) and specify ",bus=ccsr" on the command line when you want to
add the device to the CCSR region.

The two would work exactly the same way, only with different algorithms
for resource allocation.

2) similar to ISA, you would create a new ccsr-bus device and a new
ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
to specify which of the two serial ports this is for.  Most of the fdt
magic could be shared by the sysbus and CCSR cases.

I think I prefer (2)...

Paolo

> In fact, thinking about this a bit more, maybe we should just have an
> actual bus structure. Then we could have the legacy "big bucket" sysbus
> bus that nobody may ever dynamically put devices into. For CCSR we could
> create another bucket that the machine file can control where each
> device goes and can also detect if a device doesn't fit. And then we
> just declare the virt "platform bus" sysbus the default bus for cmdline
> -device sysbusdevice and everything resolves automatically.

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 11:56, Alexander Graf ha scritto:
> 
> 
> On 10.09.14 11:43, Paolo Bonzini wrote:
>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>
>>> Platform bus is a machine representation of an actual bus that devices
>>> are attached to. These devices usually are sysbus devices.
>>
>> Is there any difference between the two?
>>
>> Take a machine that has two chips, a SoC that does everything except
>> USB, and a USB controller chip.
>>
>> Strictly speaking the USB controller chip would be on a "platform bus",
>> but we would likely put it on sysbus.
>>
>> Why should it matter whether the devices are static or dynamic, for the
>> sake of calling something the "system" or the "platform" bus?  I would
>> say that QEMU calls "sysbus" the platform bus.
>>
>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>> in ARM) should probably not be in sysbus at all, and should attach
>> directly to the CPU address space.  But that is a quirk in the modeling
>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
> 
> On e500 for example, we have a predefined CCSR region. That is a machine
> defined "platform bus". The offsets inside that region are strictly
> defined by the spec.
> 
> Now take the serial ports. We have space for 2 serial ports inside of
> that CCSR region. We can spawn these 2 ports in the machine file based
> on -serial, but if you want to spawn them with -device, how do you tell
> the machine whether they should go into the "big bucket platform bus" or
> the "CCSR platform bus"?

Two possibilities:

1) you would use two instances of sysbus (one default, one created by
the board) and specify ",bus=ccsr" on the command line when you want to
add the device to the CCSR region.

The two would work exactly the same way, only with different algorithms
for resource allocation.

2) similar to ISA, you would create a new ccsr-bus device and a new
ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
to specify which of the two serial ports this is for.  Perhaps some of
the fdt magic could be shared by the sysbus and CCSR cases.

Paolo

> In fact, thinking about this a bit more, maybe we should just have an
> actual bus structure. Then we could have the legacy "big bucket" sysbus
> bus that nobody may ever dynamically put devices into. For CCSR we could
> create another bucket that the machine file can control where each
> device goes and can also detect if a device doesn't fit. And then we
> just declare the virt "platform bus" sysbus the default bus for cmdline
> -device sysbusdevice and everything resolves automatically.

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

On 10.09.14 12:05, Paolo Bonzini wrote:
> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>
>>
>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>
>>>> Platform bus is a machine representation of an actual bus that devices
>>>> are attached to. These devices usually are sysbus devices.
>>>
>>> Is there any difference between the two?
>>>
>>> Take a machine that has two chips, a SoC that does everything except
>>> USB, and a USB controller chip.
>>>
>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>> but we would likely put it on sysbus.
>>>
>>> Why should it matter whether the devices are static or dynamic, for the
>>> sake of calling something the "system" or the "platform" bus?  I would
>>> say that QEMU calls "sysbus" the platform bus.
>>>
>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>> in ARM) should probably not be in sysbus at all, and should attach
>>> directly to the CPU address space.  But that is a quirk in the modeling
>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>
>> On e500 for example, we have a predefined CCSR region. That is a machine
>> defined "platform bus". The offsets inside that region are strictly
>> defined by the spec.
>>
>> Now take the serial ports. We have space for 2 serial ports inside of
>> that CCSR region. We can spawn these 2 ports in the machine file based
>> on -serial, but if you want to spawn them with -device, how do you tell
>> the machine whether they should go into the "big bucket platform bus" or
>> the "CCSR platform bus"?
> 
> Two possibilities:
> 
> 1) you would use two instances of sysbus (one default, one created by
> the board) and specify ",bus=ccsr" on the command line when you want to
> add the device to the CCSR region.
> 
> The two would work exactly the same way, only with different algorithms
> for resource allocation.
> 
> 2) similar to ISA, you would create a new ccsr-bus device and a new
> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
> to specify which of the two serial ports this is for.  Most of the fdt
> magic could be shared by the sysbus and CCSR cases.
> 
> I think I prefer (2)...

Fair enough.

As far as moving "platform bus" logic into sysbus, I'd really like to
hold back and see what this whole thing ends up getting used for first.

So for now, I'd definitely prefer to keep "platform bus" logic and
"sysbus" logic separate. If we realize that every user only ever uses
the dynamic sysbus creation in conjunction with our "platform bus"
implementation, we can merge them.


Alex

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 12:09, Alexander Graf ha scritto:
> Fair enough.
> 
> As far as moving "platform bus" logic into sysbus, I'd really like to
> hold back and see what this whole thing ends up getting used for first.
> 
> So for now, I'd definitely prefer to keep "platform bus" logic and
> "sysbus" logic separate. If we realize that every user only ever uses
> the dynamic sysbus creation in conjunction with our "platform bus"
> implementation, we can merge them.

I agree.  As you pointed out, we have two usecases:

1) arbitrary dynamic sysbus devices, because you're playing with board
design or because you're working on a virtualized platform

2) pluggable components in a fixed board design (e.g. CCSR)

The only thing they share is FDT creation.  The other part, which is
assigning the interrupts and memory regions, is different: case (1) has
it driven by command line or simply bottom-to-top; case (2) has it
driven by an implementation of a spec.

It's not even clear to me that E500 CCSR devices should be sysbus, in
fact...

Paolo

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

On 10.09.14 12:21, Paolo Bonzini wrote:
> Il 10/09/2014 12:09, Alexander Graf ha scritto:
>> Fair enough.
>>
>> As far as moving "platform bus" logic into sysbus, I'd really like to
>> hold back and see what this whole thing ends up getting used for first.
>>
>> So for now, I'd definitely prefer to keep "platform bus" logic and
>> "sysbus" logic separate. If we realize that every user only ever uses
>> the dynamic sysbus creation in conjunction with our "platform bus"
>> implementation, we can merge them.
> 
> I agree.  As you pointed out, we have two usecases:
> 
> 1) arbitrary dynamic sysbus devices, because you're playing with board
> design or because you're working on a virtualized platform
> 
> 2) pluggable components in a fixed board design (e.g. CCSR)
> 
> The only thing they share is FDT creation.  The other part, which is
> assigning the interrupts and memory regions, is different: case (1) has
> it driven by command line or simply bottom-to-top; case (2) has it
> driven by an implementation of a spec.
> 
> It's not even clear to me that E500 CCSR devices should be sysbus, in
> fact...

The problem if you continue that thought process is that we'd end up
with 500 different buses and 500 different uart boilerplate devices just
to fit into the respective buses ;).

Otherwise I agree.


Alex

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 12:26, Alexander Graf ha scritto:
> > It's not even clear to me that E500 CCSR devices should be sysbus, in
> > fact...
>
> The problem if you continue that thought process is that we'd end up
> with 500 different buses and 500 different uart boilerplate devices just
> to fit into the respective buses ;).

True.  The alternative is to hardcode the knowledge of the spec in the
management layers (since you cannot do index=0|1, you have to do
something akin to iobase=0x3f8 for the x86 COM1 port).  I guess you will
still need two sysbuses so that you get the correct hierarchy in the
device tree, right?

And we're back to the beginning of the discussion: the distinction
between a "sysbus" and a "platform bus" disappears, and in fact it even
feels more accurate to just call these things "sysbuses"...

Paolo

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Eric Auger]

On 09/10/2014 12:09 PM, Alexander Graf wrote:
> 
> 
> On 10.09.14 12:05, Paolo Bonzini wrote:
>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>
>>>
>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>
>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>> are attached to. These devices usually are sysbus devices.
>>>>
>>>> Is there any difference between the two?
>>>>
>>>> Take a machine that has two chips, a SoC that does everything except
>>>> USB, and a USB controller chip.
>>>>
>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>> but we would likely put it on sysbus.
>>>>
>>>> Why should it matter whether the devices are static or dynamic, for the
>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>> say that QEMU calls "sysbus" the platform bus.
>>>>
>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>
>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>> defined "platform bus". The offsets inside that region are strictly
>>> defined by the spec.
>>>
>>> Now take the serial ports. We have space for 2 serial ports inside of
>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>> on -serial, but if you want to spawn them with -device, how do you tell
>>> the machine whether they should go into the "big bucket platform bus" or
>>> the "CCSR platform bus"?
>>
>> Two possibilities:
>>
>> 1) you would use two instances of sysbus (one default, one created by
>> the board) and specify ",bus=ccsr" on the command line when you want to
>> add the device to the CCSR region.
>>
>> The two would work exactly the same way, only with different algorithms
>> for resource allocation.
>>
>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>> to specify which of the two serial ports this is for.  Most of the fdt
>> magic could be shared by the sysbus and CCSR cases.
>>
>> I think I prefer (2)...
> 
> Fair enough.
> 
> As far as moving "platform bus" logic into sysbus, I'd really like to
> hold back and see what this whole thing ends up getting used for first.
> 
> So for now, I'd definitely prefer to keep "platform bus" logic and
> "sysbus" logic separate. If we realize that every user only ever uses
> the dynamic sysbus creation in conjunction with our "platform bus"
> implementation, we can merge them.

Hi Paolo, Alex,

I understand I keep the code in a separate module from sysbus.c. Is that
the shared conclusion?

Thanks

Best Regards

Eric
> 
> 
> Alex
> 

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 15:51, Eric Auger ha scritto:
> On 09/10/2014 12:09 PM, Alexander Graf wrote:
>>
>>
>> On 10.09.14 12:05, Paolo Bonzini wrote:
>>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>>
>>>>
>>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>>
>>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>>> are attached to. These devices usually are sysbus devices.
>>>>>
>>>>> Is there any difference between the two?
>>>>>
>>>>> Take a machine that has two chips, a SoC that does everything except
>>>>> USB, and a USB controller chip.
>>>>>
>>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>>> but we would likely put it on sysbus.
>>>>>
>>>>> Why should it matter whether the devices are static or dynamic, for the
>>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>>> say that QEMU calls "sysbus" the platform bus.
>>>>>
>>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>>
>>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>>> defined "platform bus". The offsets inside that region are strictly
>>>> defined by the spec.
>>>>
>>>> Now take the serial ports. We have space for 2 serial ports inside of
>>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>>> on -serial, but if you want to spawn them with -device, how do you tell
>>>> the machine whether they should go into the "big bucket platform bus" or
>>>> the "CCSR platform bus"?
>>>
>>> Two possibilities:
>>>
>>> 1) you would use two instances of sysbus (one default, one created by
>>> the board) and specify ",bus=ccsr" on the command line when you want to
>>> add the device to the CCSR region.
>>>
>>> The two would work exactly the same way, only with different algorithms
>>> for resource allocation.
>>>
>>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>>> to specify which of the two serial ports this is for.  Most of the fdt
>>> magic could be shared by the sysbus and CCSR cases.
>>>
>>> I think I prefer (2)...
>>
>> Fair enough.
>>
>> As far as moving "platform bus" logic into sysbus, I'd really like to
>> hold back and see what this whole thing ends up getting used for first.
>>
>> So for now, I'd definitely prefer to keep "platform bus" logic and
>> "sysbus" logic separate. If we realize that every user only ever uses
>> the dynamic sysbus creation in conjunction with our "platform bus"
>> implementation, we can merge them.
> 
> Hi Paolo, Alex,
> 
> I understand I keep the code in a separate module from sysbus.c. Is that
> the shared conclusion?

I don't think so, but maybe I misunderstood what Alex wrote.

Paolo

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

> Am 10.09.2014 um 16:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 10/09/2014 15:51, Eric Auger ha scritto:
>>> On 09/10/2014 12:09 PM, Alexander Graf wrote:
>>> 
>>> 
>>>> On 10.09.14 12:05, Paolo Bonzini wrote:
>>>> Il 10/09/2014 11:56, Alexander Graf ha scritto:
>>>>> 
>>>>> 
>>>>>> On 10.09.14 11:43, Paolo Bonzini wrote:
>>>>>> Il 10/09/2014 11:31, Alexander Graf ha scritto:
>>>>>>>>> Yeah, please do the registration in sysbus.c, not in virt.c.  There is
>>>>>>>>> no reason to make the platform_bus_init_notify+DynSysbusNotifier
>>>>>>>>> interface public.  The code in sysbus.c can fill in the fields.
>>>>>>> Sysbus != Platform bus. Sysbus is an in-QEMU representation of a
>>>>>>> pseudo-bus that we put all devices onto that we consider unsorted.
>>>>>>> 
>>>>>>> Platform bus is a machine representation of an actual bus that devices
>>>>>>> are attached to. These devices usually are sysbus devices.
>>>>>> 
>>>>>> Is there any difference between the two?
>>>>>> 
>>>>>> Take a machine that has two chips, a SoC that does everything except
>>>>>> USB, and a USB controller chip.
>>>>>> 
>>>>>> Strictly speaking the USB controller chip would be on a "platform bus",
>>>>>> but we would likely put it on sysbus.
>>>>>> 
>>>>>> Why should it matter whether the devices are static or dynamic, for the
>>>>>> sake of calling something the "system" or the "platform" bus?  I would
>>>>>> say that QEMU calls "sysbus" the platform bus.
>>>>>> 
>>>>>> Some devices (e.g. the local APIC in x86, or the in-core timers and GIC
>>>>>> in ARM) should probably not be in sysbus at all, and should attach
>>>>>> directly to the CPU address space.  But that is a quirk in the modeling
>>>>>> of those devices, it shouldn't mean that sysbus is not a "platform" bus.
>>>>> 
>>>>> On e500 for example, we have a predefined CCSR region. That is a machine
>>>>> defined "platform bus". The offsets inside that region are strictly
>>>>> defined by the spec.
>>>>> 
>>>>> Now take the serial ports. We have space for 2 serial ports inside of
>>>>> that CCSR region. We can spawn these 2 ports in the machine file based
>>>>> on -serial, but if you want to spawn them with -device, how do you tell
>>>>> the machine whether they should go into the "big bucket platform bus" or
>>>>> the "CCSR platform bus"?
>>>> 
>>>> Two possibilities:
>>>> 
>>>> 1) you would use two instances of sysbus (one default, one created by
>>>> the board) and specify ",bus=ccsr" on the command line when you want to
>>>> add the device to the CCSR region.
>>>> 
>>>> The two would work exactly the same way, only with different algorithms
>>>> for resource allocation.
>>>> 
>>>> 2) similar to ISA, you would create a new ccsr-bus device and a new
>>>> ccsr-serial device, and use -device ccsr-serial,index=[0|1],chardev=foo
>>>> to specify which of the two serial ports this is for.  Most of the fdt
>>>> magic could be shared by the sysbus and CCSR cases.
>>>> 
>>>> I think I prefer (2)...
>>> 
>>> Fair enough.
>>> 
>>> As far as moving "platform bus" logic into sysbus, I'd really like to
>>> hold back and see what this whole thing ends up getting used for first.
>>> 
>>> So for now, I'd definitely prefer to keep "platform bus" logic and
>>> "sysbus" logic separate. If we realize that every user only ever uses
>>> the dynamic sysbus creation in conjunction with our "platform bus"
>>> implementation, we can merge them.
>> 
>> Hi Paolo, Alex,
>> 
>> I understand I keep the code in a separate module from sysbus.c. Is that
>> the shared conclusion?
> 
> I don't think so, but maybe I misunderstood what Alex wrote.

I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.

Alex

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Paolo Bonzini]

Il 10/09/2014 16:38, Alexander Graf ha scritto:
>> > I don't think so, but maybe I misunderstood what Alex wrote.
> I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.

If you want to separate the files, that's fine.  It should still be in
hw/core.

Paolo

Re: [Qemu-devel] [PATCH v3 1/6] hw/misc/dyn_sysbus_binding: helpers for sysbus device dynamic binding

[Alexander Graf]

> Am 10.09.2014 um 16:39 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 10/09/2014 16:38, Alexander Graf ha scritto:
>>>> I don't think so, but maybe I misunderstood what Alex wrote.
>> I still think it shouldn't be inside the sysbus files. You can use sysbus devices without a platform bus.
> 
> If you want to separate the files, that's fine.  It should still be in
> hw/core.

Agreed :)

Alex

> 
> Paolo

Re: [Qemu-devel] [PATCH v3 6/6] hw/arm/virt: Support dynamically spawned sysbus devices

[Eric Auger]

On 09/09/2014 01:11 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> Allows sysbus devices to be instantiated from command line by
>> using -device option
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v2 -> v3
>> - renaming of arm_platform_bus_create_devtree and arm_load_dtb
>> - add copyright in hw/arm/dyn_sysbus_devtree.c
>>
>> v1 -> v2:
>> - remove useless vfio-platform.h include file
>> - s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
>> - use dyn_sysbus_binding and dyn_sysbus_devtree
>> - dynamic sysbus platform buse size shrinked to 4MB and
>>   moved between RTC and MMIO
>>
>> v1:
>>
>> Inspired from what Alex Graf did in ppc e500
>> https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
>> ---
>>  hw/arm/dyn_sysbus_devtree.c | 26 +++++++++++++++++++
>>  hw/arm/virt.c               | 62 +++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 86 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
>> index 6375024..61e5b5f 100644
>> --- a/hw/arm/dyn_sysbus_devtree.c
>> +++ b/hw/arm/dyn_sysbus_devtree.c
>> @@ -1,7 +1,30 @@
>> +/*
>> + * ARM Platform Bus device tree generation helpers 
>> + *
>> + * Copyright (c) 2014 Linaro Limited
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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/arm/dyn_sysbus_devtree.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/device_tree.h"
>>  
>> +/**
>> + * arm_sysbus_device_create_devtree - create the node of devices
>> + * attached to the platform bus
>> + */
>>  static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>>  {
>>      PlatformDevtreeData *data = opaque;
>> @@ -27,6 +50,9 @@ static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>>      return 0;
>>  }
>>  
>> +/**
>> + * arm_platform_bus_create_devtree - create the platform bus node
>> + */
>>  void arm_platform_bus_create_devtree(DynSysbusParams *params,
>>                                   void *fdt, const char *intc)
>>  {
> 
> All this should go in patch 2.  For the documentation comments, please
> follow the model of include/hw/memory.h (including putting the
> documentation in the header for public functions).
> 
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 9085b88..16acf44 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -40,6 +40,8 @@
>>  #include "exec/address-spaces.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +#include "hw/arm/dyn_sysbus_devtree.h"
>>  
>>  #define NUM_VIRTIO_TRANSPORTS 32
>>  
>> @@ -57,6 +59,14 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>  
>> +#define MACHVIRT_PLATFORM_BASE         0x9400000
>> +#define MACHVIRT_PLATFORM_SIZE         (4ULL * 1024 * 1024) /* 4 MB */
>> +#define MACHVIRT_PLATFORM_PAGE_SHIFT   12
>> +#define MACHVIRT_PLATFORM_SIZE_PAGES   (MACHVIRT_PLATFORM_SIZE >> \
>> +                                    MACHVIRT_PLATFORM_PAGE_SHIFT)
>> +#define MACHVIRT_PLATFORM_FIRST_IRQ    48
>> +#define MACHVIRT_PLATFORM_NUM_IRQS     20
>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> @@ -66,6 +76,7 @@ enum {
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_PLATFORM,
>>  };
>>  
>>  typedef struct MemMapEntry {
>> @@ -105,16 +116,27 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> +    [VIRT_PLATFORM] = {MACHVIRT_PLATFORM_BASE , MACHVIRT_PLATFORM_SIZE},
>>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>> -    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>> +    [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>>  };
>>  
>>  static const int a15irqmap[] = {
>>      [VIRT_UART] = 1,
>>      [VIRT_RTC] = 2,
>>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> +    [VIRT_PLATFORM] = MACHVIRT_PLATFORM_FIRST_IRQ,
>> +};
>> +
>> +static DynSysbusParams dyn_sysbus_platform_params = {
>> +    .has_platform_bus = true,
>> +    .platform_bus_base = MACHVIRT_PLATFORM_BASE,
>> +    .platform_bus_size = MACHVIRT_PLATFORM_SIZE,
>> +    .platform_bus_first_irq = MACHVIRT_PLATFORM_FIRST_IRQ,
>> +    .platform_bus_num_irqs = MACHVIRT_PLATFORM_NUM_IRQS,
>> +    .page_shift = MACHVIRT_PLATFORM_PAGE_SHIFT,
>>  };
>>  
>>  static VirtBoardInfo machines[] = {
>> @@ -458,6 +480,18 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>>      fdt_add_virtio_nodes(vbi);
>>  }
>>  
>> +static void machvirt_prep_device_tree(VirtBoardInfo *vbi)
>> +{
>> +    create_fdt(vbi);
>> +    fdt_add_timer_nodes(vbi);
>> +    fdt_add_cpu_nodes(vbi);
>> +    fdt_add_psci_node(vbi);
>> +    fdt_add_gic_node(vbi);
>> +    fdt_add_uart_node(vbi);
>> +    fdt_add_rtc_node(vbi);
>> +    fdt_add_virtio_nodes(vbi);
>> +}
>> +
>>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>>  {
>>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
>> @@ -466,14 +500,28 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>>      return board->fdt;
>>  }
>>  
>> +static void machvirt_reset_device_tree(void *opaque)
>> +{
>> +    VirtBoardInfo *board = (VirtBoardInfo *)opaque;
>> +    struct arm_boot_info *info = &board->bootinfo;
>> +    hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + info->initrd_size,
>> +                                     4096);
>> +    machvirt_prep_device_tree(board);
>> +    arm_platform_bus_create_devtree(&dyn_sysbus_platform_params,
>> +                                board->fdt, "/intc");
> 
> Why do you have to recreate the device tree every time (as opposed to
> just doing the load)?  And if arm_load_dtb used rom_add_blob_fixed
> instead of cpu_physical_memory_write, you wouldn't need a reset hook at all.
+
"
We need to do this anyway, because currently we don't do
anything to ensure the DTB hangs around for the OS to
find again on reset, even in the simple "just boot Linux"
setups.

-- PMM
"

Hi Paolo, Peter, Ard,

I come back to this topic since I must acknowledge I do not have a good
understanding of what to do here.

My goal is to add some dt nodes (dynamic sysbus ones) after the first
device tree was created by the machine init function. at machine init
the relevant info to build the dynamic sysbus nodes are not yet
available.  I registered a reset handler to recreate the device tree
including the new nodes.

The arm_load_dtb implementation recently changed and now uses
rom_add_blob_fixed - as Paolo recommended - instead of
cpu_physical_memory_write. But now when the reset handler is called I
now get an HW error: "ROM images must be loaded at startup".

Please could you advise about how to implement what I did before with
this new implementation.

Thank you in advance

Best Regards

Eric
> 
> Paolo
> 
>> +    arm_load_dtb(dtb_start, info);
>> +}
>> +
>>  static void machvirt_init(MachineState *machine)
>>  {
>> -    qemu_irq pic[NUM_IRQS];
>> +    qemu_irq *pic = g_new(qemu_irq, NUM_IRQS);
>>      MemoryRegion *sysmem = get_system_memory();
>>      int n;
>>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>>      const char *cpu_model = machine->cpu_model;
>>      VirtBoardInfo *vbi;
>> +    DynSysbusNotifier *notifier;
>>  
>>      if (!cpu_model) {
>>          cpu_model = "cortex-a15";
>> @@ -547,6 +595,13 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      create_virtio_devices(vbi, pic);
>>  
>> +    notifier = g_new(DynSysbusNotifier, 1);
>> +    notifier->notifier.notify = platform_bus_init_notify;
>> +    notifier->address_space_mem = sysmem;
>> +    notifier->mpic = pic;
>> +    notifier->params = dyn_sysbus_platform_params;
>> +    qemu_add_machine_init_done_notifier(&notifier->notifier);
>> +
>>      vbi->bootinfo.ram_size = machine->ram_size;
>>      vbi->bootinfo.kernel_filename = machine->kernel_filename;
>>      vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> @@ -556,6 +611,8 @@ static void machvirt_init(MachineState *machine)
>>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>>      vbi->bootinfo.get_dtb = machvirt_dtb;
>>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>> +
>> +    qemu_register_reset(machvirt_reset_device_tree, vbi);
>>  }
>>  
>>  static QEMUMachine machvirt_a15_machine = {
>> @@ -563,6 +620,7 @@ static QEMUMachine machvirt_a15_machine = {
>>      .desc = "ARM Virtual Machine",
>>      .init = machvirt_init,
>>      .max_cpus = 8,
>> +    .has_dynamic_sysbus = true,
>>  };
>>  
>>  static void machvirt_machine_init(void)
>>
>