[Qemu-devel] [PATCH for-2.13 v2 0/5] arm: isolate and clean up dtb generation

[Qemu-devel] [PATCH for-2.13 v2 0/5] arm: isolate and clean up dtb generation

[Igor Mammedov]

Changelog v1->v2:
   * drop "arm: reuse  arm_boot_address_space() in armv7m_load_kernel()"
   * move "arm: always start from first_cpu  when registering loader cpu reset callback"
     at the begigning of series and rebase
   * add "ppc: e500: switch E500 based machines to full  machine definition"
     and rebase 4/5 on top of it
   * fixup typo in virt_machine_get_hotpug_handler() name
   * add doc comment to skip_dtb_autoload field
   * 1-2/5 are queued in respective arm/ppc trees and are included for series
     completness so it would be easier to test, I expect series to go through
     arm tree
   

While working on cpu hotplug for mach-virt, I've noticed that DTB
is generated incrementally across whole machvirt_init(). While it's
fine for machines with static DTB, it won't really work in case of
cpu hotplug and followed up reset since machine will load old DTB
that doesn't account for hotplugged CPUs.
So I've set on a quest to consolidate DTB generation and make it
reentrant so that on reset guest would see update DTB.

It's preliminary series which makes possible to call load_dtb()
later outside of arm_load_kernel() and in process of it drops
several machine_done notifiers, that were introduced to make
plaform-bus-devices work. Hopefully it makes code easier to follow.
It replaces machine_done notifiers with device hotplug framework
to allow for dynamic sysbus devices mapping at the moment they
are created instead of waiting for machine_done time and trying to
juggle with notifiers order to do initialization in propper order.

Mostly 'make check' tested +
manually with "ppce500" machine type and eTSEC device
(eTSEC is still initialized with the same IRQs as before series)


CC: qemu-arm@nongnu.org
CC: peter.maydell@linaro.org
CC: eric.auger@redhat.com


Igor Mammedov (5):
  arm: always start from first_cpu when registering loader cpu reset
    callback
  ppc: e500: switch E500 based machines to full machine definition
  pc: simplify MachineClass::get_hotplug_handler handling
  platform-bus-device: use device plug callback instead of machine_done
    notifier
  arm/boot: split load_dtb() from arm_load_kernel()

 hw/ppc/e500.h               |  34 ++++++++--
 include/hw/arm/arm.h        |  44 ++++++++++---
 include/hw/arm/sysbus-fdt.h |  37 ++---------
 include/hw/arm/virt.h       |   1 +
 include/hw/i386/pc.h        |   8 ---
 include/hw/platform-bus.h   |   4 +-
 hw/arm/boot.c               |  73 ++++++---------------
 hw/arm/sysbus-fdt.c         |  67 ++------------------
 hw/arm/virt.c               |  95 ++++++++++++++++++----------
 hw/core/platform-bus.c      |  29 ++-------
 hw/i386/pc.c                |   6 +-
 hw/ppc/e500.c               | 151 +++++++++++++++++++++++---------------------
 hw/ppc/e500plat.c           |  95 ++++++++++++++++++++--------
 hw/ppc/mpc8544ds.c          |  47 +++++++++-----
 14 files changed, 342 insertions(+), 349 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.13 v2 1/5] arm: always start from first_cpu when registering loader cpu reset callback

[Igor Mammedov]

if arm_load_kernel() were passed non first_cpu, QEMU would end up
with partially set do_cpu_reset() callback leaving some CPUs without it.

Make sure that do_cpu_reset() is registered for all CPUs by enumerating
CPUs from first_cpu.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 26184bc..9ae6ab2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1188,7 +1188,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
      * actually loading a kernel, the handler is also responsible for
      * arranging that we start it correctly.
      */
-    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
         qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
     }
 }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.13 v2 1/5] arm: always start from first_cpu when registering loader cpu reset callback

[Philippe Mathieu-Daudé]

On 04/18/2018 11:28 AM, Igor Mammedov wrote:
> if arm_load_kernel() were passed non first_cpu, QEMU would end up
> with partially set do_cpu_reset() callback leaving some CPUs without it.
> 
> Make sure that do_cpu_reset() is registered for all CPUs by enumerating
> CPUs from first_cpu.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 26184bc..9ae6ab2 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1188,7 +1188,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>       * actually loading a kernel, the handler is also responsible for
>       * arranging that we start it correctly.
>       */
> -    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> +    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
>          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>      }
>  }
> 

[Qemu-devel] [PATCH for-2.13 v2 2/5] ppc: e500: switch E500 based machines to full machine definition

[Igor Mammedov]

Convert PPCE500Params to PCCE500MachineClass which it essentially is,
and introduce PCCE500MachineState to keep track of E500 specific
state instead of adding global variables or extra parameters to
functions when we need to keep data beyond machine init
(i.e. make it look like typical fully defined machine).

It's pretty shallow conversion instead of currently used trivial
DEFINE_MACHINE() macro. It adds extra 60LOC of boilerplate code
of full machine definition.

The patch on top[1] will use PCCE500MachineState to keep track of
platform_bus device and add E500Plate specific machine class
to use HOTPLUG_HANDLER for explicitly initializing dynamic
sysbus devices at the time they are added instead of delaying
it to machine done time by platform_bus_init_notify() which is
being removed.

1)  <1523551221-11612-3-git-send-email-imammedo@redhat.com>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
---

CC: Alexander Graf <agraf@suse.de> (supporter:e500)
CC: David Gibson <david@gibson.dropbear.id.au> (maintainer:PowerPC)
CC: qemu-ppc@nongnu.org (open list:e500)
---
 hw/ppc/e500.h      |  29 ++++++++++---
 hw/ppc/e500.c      | 119 ++++++++++++++++++++++++++++-------------------------
 hw/ppc/e500plat.c  |  64 +++++++++++++++++-----------
 hw/ppc/mpc8544ds.c |  47 +++++++++++++--------
 4 files changed, 156 insertions(+), 103 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 70ba1d8..621403b 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -3,12 +3,21 @@
 
 #include "hw/boards.h"
 
-typedef struct PPCE500Params {
-    int pci_first_slot;
-    int pci_nr_slots;
+typedef struct PPCE500MachineState {
+    /*< private >*/
+    MachineState parent_obj;
+
+} PPCE500MachineState;
+
+typedef struct PPCE500MachineClass {
+    /*< private >*/
+    MachineClass parent_class;
 
     /* required -- must at least add toplevel board compatible */
-    void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
+    void (*fixup_devtree)(void *fdt);
+
+    int pci_first_slot;
+    int pci_nr_slots;
 
     int mpic_version;
     bool has_mpc8xxx_gpio;
@@ -22,10 +31,18 @@ typedef struct PPCE500Params {
     hwaddr pci_mmio_base;
     hwaddr pci_mmio_bus_base;
     hwaddr spin_base;
-} PPCE500Params;
+} PPCE500MachineClass;
 
-void ppce500_init(MachineState *machine, PPCE500Params *params);
+void ppce500_init(MachineState *machine);
 
 hwaddr booke206_page_size_to_tlb(uint64_t size);
 
+#define TYPE_PPCE500_MACHINE      "ppce500-base-machine"
+#define PPCE500_MACHINE(obj) \
+    OBJECT_CHECK(PPCE500MachineState, (obj), TYPE_PPCE500_MACHINE)
+#define PPCE500_MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PPCE500MachineClass, obj, TYPE_PPCE500_MACHINE)
+#define PPCE500_MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PPCE500MachineClass, klass, TYPE_PPCE500_MACHINE)
+
 #endif
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 9a85a41..30b42a8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -221,14 +221,14 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
-                                        const char *mpic)
+static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
+                                        void *fdt, const char *mpic)
 {
-    gchar *node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
+    gchar *node = g_strdup_printf("/platform@%"PRIx64, pmc->platform_bus_base);
     const char platcomp[] = "qemu,platform\0simple-bus";
-    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 = pmc->platform_bus_base;
+    uint64_t size = pmc->platform_bus_size;
+    int irq_start = pmc->platform_bus_first_irq;
     PlatformBusDevice *pbus;
     DeviceState *dev;
 
@@ -265,8 +265,7 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
     g_free(node);
 }
 
-static int ppce500_load_device_tree(MachineState *machine,
-                                    PPCE500Params *params,
+static int ppce500_load_device_tree(PPCE500MachineState *pms,
                                     hwaddr addr,
                                     hwaddr initrd_base,
                                     hwaddr initrd_size,
@@ -274,6 +273,8 @@ static int ppce500_load_device_tree(MachineState *machine,
                                     hwaddr kernel_size,
                                     bool dry_run)
 {
+    MachineState *machine = MACHINE(pms);
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
     CPUPPCState *env = first_cpu->env_ptr;
     int ret = -1;
     uint64_t mem_reg_property[] = { 0, cpu_to_be64(machine->ram_size) };
@@ -295,12 +296,12 @@ static int ppce500_load_device_tree(MachineState *machine,
     int len;
     uint32_t pci_ranges[14] =
         {
-            0x2000000, 0x0, params->pci_mmio_bus_base,
-            params->pci_mmio_base >> 32, params->pci_mmio_base,
+            0x2000000, 0x0, pmc->pci_mmio_bus_base,
+            pmc->pci_mmio_base >> 32, pmc->pci_mmio_base,
             0x0, 0x20000000,
 
             0x1000000, 0x0, 0x0,
-            params->pci_pio_base >> 32, params->pci_pio_base,
+            pmc->pci_pio_base >> 32, pmc->pci_pio_base,
             0x0, 0x10000,
         };
     QemuOpts *machine_opts = qemu_get_machine_opts();
@@ -391,7 +392,7 @@ static int ppce500_load_device_tree(MachineState *machine,
     for (i = smp_cpus - 1; i >= 0; i--) {
         CPUState *cpu;
         char cpu_name[128];
-        uint64_t cpu_release_addr = params->spin_base + (i * 0x20);
+        uint64_t cpu_release_addr = pmc->spin_base + (i * 0x20);
 
         cpu = qemu_get_cpu(i);
         if (cpu == NULL) {
@@ -425,7 +426,7 @@ static int ppce500_load_device_tree(MachineState *machine,
 
     qemu_fdt_add_subnode(fdt, "/aliases");
     /* XXX These should go into their respective devices' code */
-    snprintf(soc, sizeof(soc), "/soc@%"PRIx64, params->ccsrbar_base);
+    snprintf(soc, sizeof(soc), "/soc@%"PRIx64, pmc->ccsrbar_base);
     qemu_fdt_add_subnode(fdt, soc);
     qemu_fdt_setprop_string(fdt, soc, "device_type", "soc");
     qemu_fdt_setprop(fdt, soc, "compatible", compatible_sb,
@@ -433,7 +434,7 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cell(fdt, soc, "#address-cells", 1);
     qemu_fdt_setprop_cell(fdt, soc, "#size-cells", 1);
     qemu_fdt_setprop_cells(fdt, soc, "ranges", 0x0,
-                           params->ccsrbar_base >> 32, params->ccsrbar_base,
+                           pmc->ccsrbar_base >> 32, pmc->ccsrbar_base,
                            MPC8544_CCSRBAR_SIZE);
     /* XXX should contain a reasonable value */
     qemu_fdt_setprop_cell(fdt, soc, "bus-frequency", 0);
@@ -493,7 +494,7 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cell(fdt, msi, "linux,phandle", msi_ph);
 
     snprintf(pci, sizeof(pci), "/pci@%llx",
-             params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET);
+             pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET);
     qemu_fdt_add_subnode(fdt, pci);
     qemu_fdt_setprop_cell(fdt, pci, "cell-index", 0);
     qemu_fdt_setprop_string(fdt, pci, "compatible", "fsl,mpc8540-pci");
@@ -501,7 +502,7 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
                            0x0, 0x7);
     pci_map = pci_map_create(fdt, qemu_fdt_get_phandle(fdt, mpic),
-                             params->pci_first_slot, params->pci_nr_slots,
+                             pmc->pci_first_slot, pmc->pci_nr_slots,
                              &len);
     qemu_fdt_setprop(fdt, pci, "interrupt-map", pci_map, len);
     qemu_fdt_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
@@ -513,8 +514,8 @@ static int ppce500_load_device_tree(MachineState *machine,
     qemu_fdt_setprop_cell(fdt, pci, "fsl,msi", msi_ph);
     qemu_fdt_setprop(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
     qemu_fdt_setprop_cells(fdt, pci, "reg",
-                           (params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET) >> 32,
-                           (params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET),
+                           (pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET) >> 32,
+                           (pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET),
                            0, 0x1000);
     qemu_fdt_setprop_cell(fdt, pci, "clock-frequency", 66666666);
     qemu_fdt_setprop_cell(fdt, pci, "#interrupt-cells", 1);
@@ -522,15 +523,15 @@ 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_mpc8xxx_gpio) {
+    if (pmc->has_mpc8xxx_gpio) {
         create_dt_mpc8xxx_gpio(fdt, soc, mpic);
     }
 
-    if (params->has_platform_bus) {
-        platform_bus_create_devtree(params, fdt, mpic);
+    if (pmc->has_platform_bus) {
+        platform_bus_create_devtree(pmc, fdt, mpic);
     }
 
-    params->fixup_devtree(params, fdt);
+    pmc->fixup_devtree(fdt);
 
     if (toplevel_compat) {
         qemu_fdt_setprop(fdt, "/", "compatible", toplevel_compat,
@@ -551,8 +552,7 @@ out:
 }
 
 typedef struct DeviceTreeParams {
-    MachineState *machine;
-    PPCE500Params params;
+    PPCE500MachineState *machine;
     hwaddr addr;
     hwaddr initrd_base;
     hwaddr initrd_size;
@@ -564,7 +564,7 @@ typedef struct DeviceTreeParams {
 static void ppce500_reset_device_tree(void *opaque)
 {
     DeviceTreeParams *p = opaque;
-    ppce500_load_device_tree(p->machine, &p->params, p->addr, p->initrd_base,
+    ppce500_load_device_tree(p->machine, p->addr, p->initrd_base,
                              p->initrd_size, p->kernel_base, p->kernel_size,
                              false);
 }
@@ -575,8 +575,7 @@ static void ppce500_init_notify(Notifier *notifier, void *data)
     ppce500_reset_device_tree(p);
 }
 
-static int ppce500_prep_device_tree(MachineState *machine,
-                                    PPCE500Params *params,
+static int ppce500_prep_device_tree(PPCE500MachineState *machine,
                                     hwaddr addr,
                                     hwaddr initrd_base,
                                     hwaddr initrd_size,
@@ -585,7 +584,6 @@ static int ppce500_prep_device_tree(MachineState *machine,
 {
     DeviceTreeParams *p = g_new(DeviceTreeParams, 1);
     p->machine = machine;
-    p->params = *params;
     p->addr = addr;
     p->initrd_base = initrd_base;
     p->initrd_size = initrd_size;
@@ -597,9 +595,8 @@ static int ppce500_prep_device_tree(MachineState *machine,
     qemu_add_machine_init_done_notifier(&p->notifier);
 
     /* Issue the device tree loader once, so that we get the size of the blob */
-    return ppce500_load_device_tree(machine, params, addr, initrd_base,
-                                    initrd_size, kernel_base, kernel_size,
-                                    true);
+    return ppce500_load_device_tree(machine, addr, initrd_base, initrd_size,
+                                    kernel_base, kernel_size, true);
 }
 
 /* Create -kernel TLB entries for BookE.  */
@@ -685,17 +682,19 @@ static void ppce500_cpu_reset(void *opaque)
     mmubooke_create_initial_mapping(env);
 }
 
-static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
+static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
                                            qemu_irq **irqs)
 {
     DeviceState *dev;
     SysBusDevice *s;
     int i, j, k;
+    MachineState *machine = MACHINE(pms);
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
 
     dev = qdev_create(NULL, TYPE_OPENPIC);
-    object_property_add_child(qdev_get_machine(), "pic", OBJECT(dev),
+    object_property_add_child(OBJECT(machine), "pic", OBJECT(dev),
                               &error_fatal);
-    qdev_prop_set_uint32(dev, "model", params->mpic_version);
+    qdev_prop_set_uint32(dev, "model", pmc->mpic_version);
     qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
 
     qdev_init_nofail(dev);
@@ -711,7 +710,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
     return dev;
 }
 
-static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
+static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
                                           qemu_irq **irqs, Error **errp)
 {
     Error *err = NULL;
@@ -719,7 +718,7 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
     CPUState *cs;
 
     dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
-    qdev_prop_set_uint32(dev, "model", params->mpic_version);
+    qdev_prop_set_uint32(dev, "model", pmc->mpic_version);
 
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err) {
@@ -739,11 +738,12 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
     return dev;
 }
 
-static DeviceState *ppce500_init_mpic(MachineState *machine,
-                                      PPCE500Params *params,
+static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
                                       MemoryRegion *ccsr,
                                       qemu_irq **irqs)
 {
+    MachineState *machine = MACHINE(pms);
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
     DeviceState *dev = NULL;
     SysBusDevice *s;
 
@@ -751,7 +751,7 @@ static DeviceState *ppce500_init_mpic(MachineState *machine,
         Error *err = NULL;
 
         if (machine_kernel_irqchip_allowed(machine)) {
-            dev = ppce500_init_mpic_kvm(params, irqs, &err);
+            dev = ppce500_init_mpic_kvm(pmc, irqs, &err);
         }
         if (machine_kernel_irqchip_required(machine) && !dev) {
             error_reportf_err(err,
@@ -761,7 +761,7 @@ static DeviceState *ppce500_init_mpic(MachineState *machine,
     }
 
     if (!dev) {
-        dev = ppce500_init_mpic_qemu(params, irqs);
+        dev = ppce500_init_mpic_qemu(pms, irqs);
     }
 
     s = SYS_BUS_DEVICE(dev);
@@ -778,10 +778,12 @@ static void ppce500_power_off(void *opaque, int line, int on)
     }
 }
 
-void ppce500_init(MachineState *machine, PPCE500Params *params)
+void ppce500_init(MachineState *machine)
 {
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
+    PPCE500MachineState *pms = PPCE500_MACHINE(machine);
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
     PCIBus *pci_bus;
     CPUPPCState *env = NULL;
     uint64_t loadaddr;
@@ -835,8 +837,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
         irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
         env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
-        env->mpic_iack = params->ccsrbar_base +
-                         MPC8544_MPIC_REGS_OFFSET + 0xa0;
+        env->mpic_iack = pmc->ccsrbar_base + MPC8544_MPIC_REGS_OFFSET + 0xa0;
 
         ppc_booke_timers_init(cpu, 400000000, PPC_TIMER_E500);
 
@@ -869,10 +870,10 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     qdev_init_nofail(dev);
     ccsr = CCSR(dev);
     ccsr_addr_space = &ccsr->ccsr_space;
-    memory_region_add_subregion(address_space_mem, params->ccsrbar_base,
+    memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
                                 ccsr_addr_space);
 
-    mpicdev = ppce500_init_mpic(machine, params, ccsr_addr_space, irqs);
+    mpicdev = ppce500_init_mpic(pms, ccsr_addr_space, irqs);
 
     /* Serial */
     if (serial_hds[0]) {
@@ -898,7 +899,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     dev = qdev_create(NULL, "e500-pcihost");
     object_property_add_child(qdev_get_machine(), "pci-host", OBJECT(dev),
                               &error_abort);
-    qdev_prop_set_uint32(dev, "first_slot", params->pci_first_slot);
+    qdev_prop_set_uint32(dev, "first_slot", pmc->pci_first_slot);
     qdev_prop_set_uint32(dev, "first_pin_irq", pci_irq_nrs[0]);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
@@ -921,9 +922,9 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     }
 
     /* Register spinning region */
-    sysbus_create_simple("e500-spin", params->spin_base, NULL);
+    sysbus_create_simple("e500-spin", pmc->spin_base, NULL);
 
-    if (params->has_mpc8xxx_gpio) {
+    if (pmc->has_mpc8xxx_gpio) {
         qemu_irq poweroff_irq;
 
         dev = qdev_create(NULL, "mpc8xxx_gpio");
@@ -939,21 +940,21 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     }
 
     /* Platform Bus Device */
-    if (params->has_platform_bus) {
+    if (pmc->has_platform_bus) {
         dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
         dev->id = TYPE_PLATFORM_BUS_DEVICE;
-        qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
-        qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
+        qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
+        qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
         qdev_init_nofail(dev);
         s = SYS_BUS_DEVICE(dev);
 
-        for (i = 0; i < params->platform_bus_num_irqs; i++) {
-            int irqn = params->platform_bus_first_irq + i;
+        for (i = 0; i < pmc->platform_bus_num_irqs; i++) {
+            int irqn = pmc->platform_bus_first_irq + i;
             sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
         }
 
         memory_region_add_subregion(address_space_mem,
-                                    params->platform_bus_base,
+                                    pmc->platform_bus_base,
                                     sysbus_mmio_get_region(s, 0));
     }
 
@@ -1056,7 +1057,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
             exit(1);
     }
 
-    dt_size = ppce500_prep_device_tree(machine, params, dt_base,
+    dt_size = ppce500_prep_device_tree(pms, dt_base,
                                        initrd_base, initrd_size,
                                        kernel_base, kernel_size);
     if (dt_size < 0) {
@@ -1085,9 +1086,17 @@ static const TypeInfo e500_ccsr_info = {
     .instance_init = e500_ccsr_initfn,
 };
 
+static const TypeInfo ppce500_info = {
+    .name          = TYPE_PPCE500_MACHINE,
+    .parent        = TYPE_MACHINE,
+    .abstract      = true,
+    .class_size    = sizeof(PPCE500MachineClass),
+};
+
 static void e500_register_types(void)
 {
     type_register_static(&e500_ccsr_info);
+    type_register_static(&ppce500_info);
 }
 
 type_init(e500_register_types)
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 81d03e1..f69aadb 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -21,7 +21,7 @@
 #include "hw/ppc/openpic.h"
 #include "kvm_ppc.h"
 
-static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
+static void e500plat_fixup_devtree(void *fdt)
 {
     const char model[] = "QEMU ppce500";
     const char compatible[] = "fsl,qemu-e500";
@@ -33,40 +33,54 @@ static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
 
 static void e500plat_init(MachineState *machine)
 {
-    PPCE500Params params = {
-        .pci_first_slot = 0x1,
-        .pci_nr_slots = PCI_SLOT_MAX - 1,
-        .fixup_devtree = e500plat_fixup_devtree,
-        .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
-        .has_mpc8xxx_gpio = true,
-        .has_platform_bus = true,
-        .platform_bus_base = 0xf00000000ULL,
-        .platform_bus_size = (128ULL * 1024 * 1024),
-        .platform_bus_first_irq = 5,
-        .platform_bus_num_irqs = 10,
-        .ccsrbar_base = 0xFE0000000ULL,
-        .pci_pio_base = 0xFE1000000ULL,
-        .pci_mmio_base = 0xC00000000ULL,
-        .pci_mmio_bus_base = 0xE0000000ULL,
-        .spin_base = 0xFEF000000ULL,
-    };
-
+    PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
     /* Older KVM versions don't support EPR which breaks guests when we announce
        MPIC variants that support EPR. Revert to an older one for those */
     if (kvm_enabled() && !kvmppc_has_cap_epr()) {
-        params.mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
+        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
     }
 
-    ppce500_init(machine, &params);
+    ppce500_init(machine);
 }
 
-static void e500plat_machine_init(MachineClass *mc)
+#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
+
+static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
+    PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    pmc->pci_first_slot = 0x1;
+    pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
+    pmc->fixup_devtree = e500plat_fixup_devtree;
+    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
+    pmc->has_mpc8xxx_gpio = true;
+    pmc->has_platform_bus = true;
+    pmc->platform_bus_base = 0xf00000000ULL;
+    pmc->platform_bus_size = (128ULL * 1024 * 1024);
+    pmc->platform_bus_first_irq = 5;
+    pmc->platform_bus_num_irqs = 10;
+    pmc->ccsrbar_base = 0xFE0000000ULL;
+    pmc->pci_pio_base = 0xFE1000000ULL;
+    pmc->pci_mmio_base = 0xC00000000ULL;
+    pmc->pci_mmio_bus_base = 0xE0000000ULL;
+    pmc->spin_base = 0xFEF000000ULL;
+
     mc->desc = "generic paravirt e500 platform";
     mc->init = e500plat_init;
     mc->max_cpus = 32;
-    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
-}
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
+ }
+
+static const TypeInfo e500plat_info = {
+    .name          = TYPE_E500PLAT_MACHINE,
+    .parent        = TYPE_PPCE500_MACHINE,
+    .class_init    = e500plat_machine_class_init,
+};
 
-DEFINE_MACHINE("ppce500", e500plat_machine_init)
+static void e500plat_register_types(void)
+{
+    type_register_static(&e500plat_info);
+}
+type_init(e500plat_register_types)
diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
index 1717953..ab30a2a 100644
--- a/hw/ppc/mpc8544ds.c
+++ b/hw/ppc/mpc8544ds.c
@@ -18,7 +18,7 @@
 #include "qemu/error-report.h"
 #include "cpu.h"
 
-static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
+static void mpc8544ds_fixup_devtree(void *fdt)
 {
     const char model[] = "MPC8544DS";
     const char compatible[] = "MPC8544DS\0MPC85xxDS";
@@ -30,33 +30,46 @@ static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
 
 static void mpc8544ds_init(MachineState *machine)
 {
-    PPCE500Params params = {
-        .pci_first_slot = 0x11,
-        .pci_nr_slots = 2,
-        .fixup_devtree = mpc8544ds_fixup_devtree,
-        .mpic_version = OPENPIC_MODEL_FSL_MPIC_20,
-        .ccsrbar_base = 0xE0000000ULL,
-        .pci_mmio_base = 0xC0000000ULL,
-        .pci_mmio_bus_base = 0xC0000000ULL,
-        .pci_pio_base = 0xE1000000ULL,
-        .spin_base = 0xEF000000ULL,
-    };
-
     if (machine->ram_size > 0xc0000000) {
         error_report("The MPC8544DS board only supports up to 3GB of RAM");
         exit(1);
     }
 
-    ppce500_init(machine, &params);
+    ppce500_init(machine);
 }
 
-
-static void ppce500_machine_init(MachineClass *mc)
+static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+    PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
+
+    pmc->pci_first_slot = 0x11;
+    pmc->pci_nr_slots = 2;
+    pmc->fixup_devtree = mpc8544ds_fixup_devtree;
+    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
+    pmc->ccsrbar_base = 0xE0000000ULL;
+    pmc->pci_mmio_base = 0xC0000000ULL;
+    pmc->pci_mmio_bus_base = 0xC0000000ULL;
+    pmc->pci_pio_base = 0xE1000000ULL;
+    pmc->spin_base = 0xEF000000ULL;
+
     mc->desc = "mpc8544ds";
     mc->init = mpc8544ds_init;
     mc->max_cpus = 15;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
 }
 
-DEFINE_MACHINE("mpc8544ds", ppce500_machine_init)
+#define TYPE_MPC8544DS_MACHINE  MACHINE_TYPE_NAME("mpc8544ds")
+
+static const TypeInfo mpc8544ds_info = {
+    .name          = TYPE_MPC8544DS_MACHINE,
+    .parent        = TYPE_PPCE500_MACHINE,
+    .class_init    = e500plat_machine_class_init,
+};
+
+static void mpc8544ds_register_types(void)
+{
+    type_register_static(&mpc8544ds_info);
+}
+
+type_init(mpc8544ds_register_types)
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.13 v2 2/5] ppc: e500: switch E500 based machines to full machine definition

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 27001 bytes --]

On Wed, Apr 18, 2018 at 04:28:02PM +0200, Igor Mammedov wrote:
> Convert PPCE500Params to PCCE500MachineClass which it essentially is,
> and introduce PCCE500MachineState to keep track of E500 specific
> state instead of adding global variables or extra parameters to
> functions when we need to keep data beyond machine init
> (i.e. make it look like typical fully defined machine).
> 
> It's pretty shallow conversion instead of currently used trivial
> DEFINE_MACHINE() macro. It adds extra 60LOC of boilerplate code
> of full machine definition.
> 
> The patch on top[1] will use PCCE500MachineState to keep track of
> platform_bus device and add E500Plate specific machine class
> to use HOTPLUG_HANDLER for explicitly initializing dynamic
> sysbus devices at the time they are added instead of delaying
> it to machine done time by platform_bus_init_notify() which is
> being removed.
> 
> 1)  <1523551221-11612-3-git-send-email-imammedo@redhat.com>
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>

As noted elsewhere, I've already merged this into my ppc-for-2.13
tree.  However, don't let that stop you from posting and/or queueing
it elsewhere.  Whoever ends up merging first once 2.13 opens, it
should be easy to resolve.

> ---
> 
> CC: Alexander Graf <agraf@suse.de> (supporter:e500)
> CC: David Gibson <david@gibson.dropbear.id.au> (maintainer:PowerPC)
> CC: qemu-ppc@nongnu.org (open list:e500)
> ---
>  hw/ppc/e500.h      |  29 ++++++++++---
>  hw/ppc/e500.c      | 119 ++++++++++++++++++++++++++++-------------------------
>  hw/ppc/e500plat.c  |  64 +++++++++++++++++-----------
>  hw/ppc/mpc8544ds.c |  47 +++++++++++++--------
>  4 files changed, 156 insertions(+), 103 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8..621403b 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -3,12 +3,21 @@
>  
>  #include "hw/boards.h"
>  
> -typedef struct PPCE500Params {
> -    int pci_first_slot;
> -    int pci_nr_slots;
> +typedef struct PPCE500MachineState {
> +    /*< private >*/
> +    MachineState parent_obj;
> +
> +} PPCE500MachineState;
> +
> +typedef struct PPCE500MachineClass {
> +    /*< private >*/
> +    MachineClass parent_class;
>  
>      /* required -- must at least add toplevel board compatible */
> -    void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
> +    void (*fixup_devtree)(void *fdt);
> +
> +    int pci_first_slot;
> +    int pci_nr_slots;
>  
>      int mpic_version;
>      bool has_mpc8xxx_gpio;
> @@ -22,10 +31,18 @@ typedef struct PPCE500Params {
>      hwaddr pci_mmio_base;
>      hwaddr pci_mmio_bus_base;
>      hwaddr spin_base;
> -} PPCE500Params;
> +} PPCE500MachineClass;
>  
> -void ppce500_init(MachineState *machine, PPCE500Params *params);
> +void ppce500_init(MachineState *machine);
>  
>  hwaddr booke206_page_size_to_tlb(uint64_t size);
>  
> +#define TYPE_PPCE500_MACHINE      "ppce500-base-machine"
> +#define PPCE500_MACHINE(obj) \
> +    OBJECT_CHECK(PPCE500MachineState, (obj), TYPE_PPCE500_MACHINE)
> +#define PPCE500_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(PPCE500MachineClass, obj, TYPE_PPCE500_MACHINE)
> +#define PPCE500_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(PPCE500MachineClass, klass, TYPE_PPCE500_MACHINE)
> +
>  #endif
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 9a85a41..30b42a8 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -221,14 +221,14 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
>      }
>  }
>  
> -static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
> -                                        const char *mpic)
> +static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
> +                                        void *fdt, const char *mpic)
>  {
> -    gchar *node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
> +    gchar *node = g_strdup_printf("/platform@%"PRIx64, pmc->platform_bus_base);
>      const char platcomp[] = "qemu,platform\0simple-bus";
> -    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 = pmc->platform_bus_base;
> +    uint64_t size = pmc->platform_bus_size;
> +    int irq_start = pmc->platform_bus_first_irq;
>      PlatformBusDevice *pbus;
>      DeviceState *dev;
>  
> @@ -265,8 +265,7 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
>      g_free(node);
>  }
>  
> -static int ppce500_load_device_tree(MachineState *machine,
> -                                    PPCE500Params *params,
> +static int ppce500_load_device_tree(PPCE500MachineState *pms,
>                                      hwaddr addr,
>                                      hwaddr initrd_base,
>                                      hwaddr initrd_size,
> @@ -274,6 +273,8 @@ static int ppce500_load_device_tree(MachineState *machine,
>                                      hwaddr kernel_size,
>                                      bool dry_run)
>  {
> +    MachineState *machine = MACHINE(pms);
> +    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
>      CPUPPCState *env = first_cpu->env_ptr;
>      int ret = -1;
>      uint64_t mem_reg_property[] = { 0, cpu_to_be64(machine->ram_size) };
> @@ -295,12 +296,12 @@ static int ppce500_load_device_tree(MachineState *machine,
>      int len;
>      uint32_t pci_ranges[14] =
>          {
> -            0x2000000, 0x0, params->pci_mmio_bus_base,
> -            params->pci_mmio_base >> 32, params->pci_mmio_base,
> +            0x2000000, 0x0, pmc->pci_mmio_bus_base,
> +            pmc->pci_mmio_base >> 32, pmc->pci_mmio_base,
>              0x0, 0x20000000,
>  
>              0x1000000, 0x0, 0x0,
> -            params->pci_pio_base >> 32, params->pci_pio_base,
> +            pmc->pci_pio_base >> 32, pmc->pci_pio_base,
>              0x0, 0x10000,
>          };
>      QemuOpts *machine_opts = qemu_get_machine_opts();
> @@ -391,7 +392,7 @@ static int ppce500_load_device_tree(MachineState *machine,
>      for (i = smp_cpus - 1; i >= 0; i--) {
>          CPUState *cpu;
>          char cpu_name[128];
> -        uint64_t cpu_release_addr = params->spin_base + (i * 0x20);
> +        uint64_t cpu_release_addr = pmc->spin_base + (i * 0x20);
>  
>          cpu = qemu_get_cpu(i);
>          if (cpu == NULL) {
> @@ -425,7 +426,7 @@ static int ppce500_load_device_tree(MachineState *machine,
>  
>      qemu_fdt_add_subnode(fdt, "/aliases");
>      /* XXX These should go into their respective devices' code */
> -    snprintf(soc, sizeof(soc), "/soc@%"PRIx64, params->ccsrbar_base);
> +    snprintf(soc, sizeof(soc), "/soc@%"PRIx64, pmc->ccsrbar_base);
>      qemu_fdt_add_subnode(fdt, soc);
>      qemu_fdt_setprop_string(fdt, soc, "device_type", "soc");
>      qemu_fdt_setprop(fdt, soc, "compatible", compatible_sb,
> @@ -433,7 +434,7 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, soc, "#address-cells", 1);
>      qemu_fdt_setprop_cell(fdt, soc, "#size-cells", 1);
>      qemu_fdt_setprop_cells(fdt, soc, "ranges", 0x0,
> -                           params->ccsrbar_base >> 32, params->ccsrbar_base,
> +                           pmc->ccsrbar_base >> 32, pmc->ccsrbar_base,
>                             MPC8544_CCSRBAR_SIZE);
>      /* XXX should contain a reasonable value */
>      qemu_fdt_setprop_cell(fdt, soc, "bus-frequency", 0);
> @@ -493,7 +494,7 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, msi, "linux,phandle", msi_ph);
>  
>      snprintf(pci, sizeof(pci), "/pci@%llx",
> -             params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET);
> +             pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET);
>      qemu_fdt_add_subnode(fdt, pci);
>      qemu_fdt_setprop_cell(fdt, pci, "cell-index", 0);
>      qemu_fdt_setprop_string(fdt, pci, "compatible", "fsl,mpc8540-pci");
> @@ -501,7 +502,7 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
>                             0x0, 0x7);
>      pci_map = pci_map_create(fdt, qemu_fdt_get_phandle(fdt, mpic),
> -                             params->pci_first_slot, params->pci_nr_slots,
> +                             pmc->pci_first_slot, pmc->pci_nr_slots,
>                               &len);
>      qemu_fdt_setprop(fdt, pci, "interrupt-map", pci_map, len);
>      qemu_fdt_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
> @@ -513,8 +514,8 @@ static int ppce500_load_device_tree(MachineState *machine,
>      qemu_fdt_setprop_cell(fdt, pci, "fsl,msi", msi_ph);
>      qemu_fdt_setprop(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
>      qemu_fdt_setprop_cells(fdt, pci, "reg",
> -                           (params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET) >> 32,
> -                           (params->ccsrbar_base + MPC8544_PCI_REGS_OFFSET),
> +                           (pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET) >> 32,
> +                           (pmc->ccsrbar_base + MPC8544_PCI_REGS_OFFSET),
>                             0, 0x1000);
>      qemu_fdt_setprop_cell(fdt, pci, "clock-frequency", 66666666);
>      qemu_fdt_setprop_cell(fdt, pci, "#interrupt-cells", 1);
> @@ -522,15 +523,15 @@ 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_mpc8xxx_gpio) {
> +    if (pmc->has_mpc8xxx_gpio) {
>          create_dt_mpc8xxx_gpio(fdt, soc, mpic);
>      }
>  
> -    if (params->has_platform_bus) {
> -        platform_bus_create_devtree(params, fdt, mpic);
> +    if (pmc->has_platform_bus) {
> +        platform_bus_create_devtree(pmc, fdt, mpic);
>      }
>  
> -    params->fixup_devtree(params, fdt);
> +    pmc->fixup_devtree(fdt);
>  
>      if (toplevel_compat) {
>          qemu_fdt_setprop(fdt, "/", "compatible", toplevel_compat,
> @@ -551,8 +552,7 @@ out:
>  }
>  
>  typedef struct DeviceTreeParams {
> -    MachineState *machine;
> -    PPCE500Params params;
> +    PPCE500MachineState *machine;
>      hwaddr addr;
>      hwaddr initrd_base;
>      hwaddr initrd_size;
> @@ -564,7 +564,7 @@ typedef struct DeviceTreeParams {
>  static void ppce500_reset_device_tree(void *opaque)
>  {
>      DeviceTreeParams *p = opaque;
> -    ppce500_load_device_tree(p->machine, &p->params, p->addr, p->initrd_base,
> +    ppce500_load_device_tree(p->machine, p->addr, p->initrd_base,
>                               p->initrd_size, p->kernel_base, p->kernel_size,
>                               false);
>  }
> @@ -575,8 +575,7 @@ static void ppce500_init_notify(Notifier *notifier, void *data)
>      ppce500_reset_device_tree(p);
>  }
>  
> -static int ppce500_prep_device_tree(MachineState *machine,
> -                                    PPCE500Params *params,
> +static int ppce500_prep_device_tree(PPCE500MachineState *machine,
>                                      hwaddr addr,
>                                      hwaddr initrd_base,
>                                      hwaddr initrd_size,
> @@ -585,7 +584,6 @@ static int ppce500_prep_device_tree(MachineState *machine,
>  {
>      DeviceTreeParams *p = g_new(DeviceTreeParams, 1);
>      p->machine = machine;
> -    p->params = *params;
>      p->addr = addr;
>      p->initrd_base = initrd_base;
>      p->initrd_size = initrd_size;
> @@ -597,9 +595,8 @@ static int ppce500_prep_device_tree(MachineState *machine,
>      qemu_add_machine_init_done_notifier(&p->notifier);
>  
>      /* Issue the device tree loader once, so that we get the size of the blob */
> -    return ppce500_load_device_tree(machine, params, addr, initrd_base,
> -                                    initrd_size, kernel_base, kernel_size,
> -                                    true);
> +    return ppce500_load_device_tree(machine, addr, initrd_base, initrd_size,
> +                                    kernel_base, kernel_size, true);
>  }
>  
>  /* Create -kernel TLB entries for BookE.  */
> @@ -685,17 +682,19 @@ static void ppce500_cpu_reset(void *opaque)
>      mmubooke_create_initial_mapping(env);
>  }
>  
> -static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
> +static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
>                                             qemu_irq **irqs)
>  {
>      DeviceState *dev;
>      SysBusDevice *s;
>      int i, j, k;
> +    MachineState *machine = MACHINE(pms);
> +    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
>  
>      dev = qdev_create(NULL, TYPE_OPENPIC);
> -    object_property_add_child(qdev_get_machine(), "pic", OBJECT(dev),
> +    object_property_add_child(OBJECT(machine), "pic", OBJECT(dev),
>                                &error_fatal);
> -    qdev_prop_set_uint32(dev, "model", params->mpic_version);
> +    qdev_prop_set_uint32(dev, "model", pmc->mpic_version);
>      qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
>  
>      qdev_init_nofail(dev);
> @@ -711,7 +710,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500Params *params,
>      return dev;
>  }
>  
> -static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
> +static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
>                                            qemu_irq **irqs, Error **errp)
>  {
>      Error *err = NULL;
> @@ -719,7 +718,7 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
>      CPUState *cs;
>  
>      dev = qdev_create(NULL, TYPE_KVM_OPENPIC);
> -    qdev_prop_set_uint32(dev, "model", params->mpic_version);
> +    qdev_prop_set_uint32(dev, "model", pmc->mpic_version);
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err) {
> @@ -739,11 +738,12 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
>      return dev;
>  }
>  
> -static DeviceState *ppce500_init_mpic(MachineState *machine,
> -                                      PPCE500Params *params,
> +static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
>                                        MemoryRegion *ccsr,
>                                        qemu_irq **irqs)
>  {
> +    MachineState *machine = MACHINE(pms);
> +    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
>      DeviceState *dev = NULL;
>      SysBusDevice *s;
>  
> @@ -751,7 +751,7 @@ static DeviceState *ppce500_init_mpic(MachineState *machine,
>          Error *err = NULL;
>  
>          if (machine_kernel_irqchip_allowed(machine)) {
> -            dev = ppce500_init_mpic_kvm(params, irqs, &err);
> +            dev = ppce500_init_mpic_kvm(pmc, irqs, &err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !dev) {
>              error_reportf_err(err,
> @@ -761,7 +761,7 @@ static DeviceState *ppce500_init_mpic(MachineState *machine,
>      }
>  
>      if (!dev) {
> -        dev = ppce500_init_mpic_qemu(params, irqs);
> +        dev = ppce500_init_mpic_qemu(pms, irqs);
>      }
>  
>      s = SYS_BUS_DEVICE(dev);
> @@ -778,10 +778,12 @@ static void ppce500_power_off(void *opaque, int line, int on)
>      }
>  }
>  
> -void ppce500_init(MachineState *machine, PPCE500Params *params)
> +void ppce500_init(MachineState *machine)
>  {
>      MemoryRegion *address_space_mem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    PPCE500MachineState *pms = PPCE500_MACHINE(machine);
> +    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>      PCIBus *pci_bus;
>      CPUPPCState *env = NULL;
>      uint64_t loadaddr;
> @@ -835,8 +837,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          irqs[i][OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT];
>          irqs[i][OPENPIC_OUTPUT_CINT] = input[PPCE500_INPUT_CINT];
>          env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
> -        env->mpic_iack = params->ccsrbar_base +
> -                         MPC8544_MPIC_REGS_OFFSET + 0xa0;
> +        env->mpic_iack = pmc->ccsrbar_base + MPC8544_MPIC_REGS_OFFSET + 0xa0;
>  
>          ppc_booke_timers_init(cpu, 400000000, PPC_TIMER_E500);
>  
> @@ -869,10 +870,10 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      qdev_init_nofail(dev);
>      ccsr = CCSR(dev);
>      ccsr_addr_space = &ccsr->ccsr_space;
> -    memory_region_add_subregion(address_space_mem, params->ccsrbar_base,
> +    memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
>                                  ccsr_addr_space);
>  
> -    mpicdev = ppce500_init_mpic(machine, params, ccsr_addr_space, irqs);
> +    mpicdev = ppce500_init_mpic(pms, ccsr_addr_space, irqs);
>  
>      /* Serial */
>      if (serial_hds[0]) {
> @@ -898,7 +899,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      dev = qdev_create(NULL, "e500-pcihost");
>      object_property_add_child(qdev_get_machine(), "pci-host", OBJECT(dev),
>                                &error_abort);
> -    qdev_prop_set_uint32(dev, "first_slot", params->pci_first_slot);
> +    qdev_prop_set_uint32(dev, "first_slot", pmc->pci_first_slot);
>      qdev_prop_set_uint32(dev, "first_pin_irq", pci_irq_nrs[0]);
>      qdev_init_nofail(dev);
>      s = SYS_BUS_DEVICE(dev);
> @@ -921,9 +922,9 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      }
>  
>      /* Register spinning region */
> -    sysbus_create_simple("e500-spin", params->spin_base, NULL);
> +    sysbus_create_simple("e500-spin", pmc->spin_base, NULL);
>  
> -    if (params->has_mpc8xxx_gpio) {
> +    if (pmc->has_mpc8xxx_gpio) {
>          qemu_irq poweroff_irq;
>  
>          dev = qdev_create(NULL, "mpc8xxx_gpio");
> @@ -939,21 +940,21 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>      }
>  
>      /* Platform Bus Device */
> -    if (params->has_platform_bus) {
> +    if (pmc->has_platform_bus) {
>          dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
>          dev->id = TYPE_PLATFORM_BUS_DEVICE;
> -        qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
> -        qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
> +        qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
> +        qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
>          qdev_init_nofail(dev);
>          s = SYS_BUS_DEVICE(dev);
>  
> -        for (i = 0; i < params->platform_bus_num_irqs; i++) {
> -            int irqn = params->platform_bus_first_irq + i;
> +        for (i = 0; i < pmc->platform_bus_num_irqs; i++) {
> +            int irqn = pmc->platform_bus_first_irq + i;
>              sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
>          }
>  
>          memory_region_add_subregion(address_space_mem,
> -                                    params->platform_bus_base,
> +                                    pmc->platform_bus_base,
>                                      sysbus_mmio_get_region(s, 0));
>      }
>  
> @@ -1056,7 +1057,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>              exit(1);
>      }
>  
> -    dt_size = ppce500_prep_device_tree(machine, params, dt_base,
> +    dt_size = ppce500_prep_device_tree(pms, dt_base,
>                                         initrd_base, initrd_size,
>                                         kernel_base, kernel_size);
>      if (dt_size < 0) {
> @@ -1085,9 +1086,17 @@ static const TypeInfo e500_ccsr_info = {
>      .instance_init = e500_ccsr_initfn,
>  };
>  
> +static const TypeInfo ppce500_info = {
> +    .name          = TYPE_PPCE500_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .abstract      = true,
> +    .class_size    = sizeof(PPCE500MachineClass),
> +};
> +
>  static void e500_register_types(void)
>  {
>      type_register_static(&e500_ccsr_info);
> +    type_register_static(&ppce500_info);
>  }
>  
>  type_init(e500_register_types)
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 81d03e1..f69aadb 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -21,7 +21,7 @@
>  #include "hw/ppc/openpic.h"
>  #include "kvm_ppc.h"
>  
> -static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
> +static void e500plat_fixup_devtree(void *fdt)
>  {
>      const char model[] = "QEMU ppce500";
>      const char compatible[] = "fsl,qemu-e500";
> @@ -33,40 +33,54 @@ static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
>  
>  static void e500plat_init(MachineState *machine)
>  {
> -    PPCE500Params params = {
> -        .pci_first_slot = 0x1,
> -        .pci_nr_slots = PCI_SLOT_MAX - 1,
> -        .fixup_devtree = e500plat_fixup_devtree,
> -        .mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
> -        .has_mpc8xxx_gpio = true,
> -        .has_platform_bus = true,
> -        .platform_bus_base = 0xf00000000ULL,
> -        .platform_bus_size = (128ULL * 1024 * 1024),
> -        .platform_bus_first_irq = 5,
> -        .platform_bus_num_irqs = 10,
> -        .ccsrbar_base = 0xFE0000000ULL,
> -        .pci_pio_base = 0xFE1000000ULL,
> -        .pci_mmio_base = 0xC00000000ULL,
> -        .pci_mmio_bus_base = 0xE0000000ULL,
> -        .spin_base = 0xFEF000000ULL,
> -    };
> -
> +    PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>      /* Older KVM versions don't support EPR which breaks guests when we announce
>         MPIC variants that support EPR. Revert to an older one for those */
>      if (kvm_enabled() && !kvmppc_has_cap_epr()) {
> -        params.mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
> +        pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
>      }
>  
> -    ppce500_init(machine, &params);
> +    ppce500_init(machine);
>  }
>  
> -static void e500plat_machine_init(MachineClass *mc)
> +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> +
> +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>  {
> +    PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    pmc->pci_first_slot = 0x1;
> +    pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
> +    pmc->fixup_devtree = e500plat_fixup_devtree;
> +    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_42;
> +    pmc->has_mpc8xxx_gpio = true;
> +    pmc->has_platform_bus = true;
> +    pmc->platform_bus_base = 0xf00000000ULL;
> +    pmc->platform_bus_size = (128ULL * 1024 * 1024);
> +    pmc->platform_bus_first_irq = 5;
> +    pmc->platform_bus_num_irqs = 10;
> +    pmc->ccsrbar_base = 0xFE0000000ULL;
> +    pmc->pci_pio_base = 0xFE1000000ULL;
> +    pmc->pci_mmio_base = 0xC00000000ULL;
> +    pmc->pci_mmio_bus_base = 0xE0000000ULL;
> +    pmc->spin_base = 0xFEF000000ULL;
> +
>      mc->desc = "generic paravirt e500 platform";
>      mc->init = e500plat_init;
>      mc->max_cpus = 32;
> -    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
> -}
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
> + }
> +
> +static const TypeInfo e500plat_info = {
> +    .name          = TYPE_E500PLAT_MACHINE,
> +    .parent        = TYPE_PPCE500_MACHINE,
> +    .class_init    = e500plat_machine_class_init,
> +};
>  
> -DEFINE_MACHINE("ppce500", e500plat_machine_init)
> +static void e500plat_register_types(void)
> +{
> +    type_register_static(&e500plat_info);
> +}
> +type_init(e500plat_register_types)
> diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
> index 1717953..ab30a2a 100644
> --- a/hw/ppc/mpc8544ds.c
> +++ b/hw/ppc/mpc8544ds.c
> @@ -18,7 +18,7 @@
>  #include "qemu/error-report.h"
>  #include "cpu.h"
>  
> -static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
> +static void mpc8544ds_fixup_devtree(void *fdt)
>  {
>      const char model[] = "MPC8544DS";
>      const char compatible[] = "MPC8544DS\0MPC85xxDS";
> @@ -30,33 +30,46 @@ static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
>  
>  static void mpc8544ds_init(MachineState *machine)
>  {
> -    PPCE500Params params = {
> -        .pci_first_slot = 0x11,
> -        .pci_nr_slots = 2,
> -        .fixup_devtree = mpc8544ds_fixup_devtree,
> -        .mpic_version = OPENPIC_MODEL_FSL_MPIC_20,
> -        .ccsrbar_base = 0xE0000000ULL,
> -        .pci_mmio_base = 0xC0000000ULL,
> -        .pci_mmio_bus_base = 0xC0000000ULL,
> -        .pci_pio_base = 0xE1000000ULL,
> -        .spin_base = 0xEF000000ULL,
> -    };
> -
>      if (machine->ram_size > 0xc0000000) {
>          error_report("The MPC8544DS board only supports up to 3GB of RAM");
>          exit(1);
>      }
>  
> -    ppce500_init(machine, &params);
> +    ppce500_init(machine);
>  }
>  
> -
> -static void ppce500_machine_init(MachineClass *mc)
> +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>  {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
> +
> +    pmc->pci_first_slot = 0x11;
> +    pmc->pci_nr_slots = 2;
> +    pmc->fixup_devtree = mpc8544ds_fixup_devtree;
> +    pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20;
> +    pmc->ccsrbar_base = 0xE0000000ULL;
> +    pmc->pci_mmio_base = 0xC0000000ULL;
> +    pmc->pci_mmio_bus_base = 0xC0000000ULL;
> +    pmc->pci_pio_base = 0xE1000000ULL;
> +    pmc->spin_base = 0xEF000000ULL;
> +
>      mc->desc = "mpc8544ds";
>      mc->init = mpc8544ds_init;
>      mc->max_cpus = 15;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>  }
>  
> -DEFINE_MACHINE("mpc8544ds", ppce500_machine_init)
> +#define TYPE_MPC8544DS_MACHINE  MACHINE_TYPE_NAME("mpc8544ds")
> +
> +static const TypeInfo mpc8544ds_info = {
> +    .name          = TYPE_MPC8544DS_MACHINE,
> +    .parent        = TYPE_PPCE500_MACHINE,
> +    .class_init    = e500plat_machine_class_init,
> +};
> +
> +static void mpc8544ds_register_types(void)
> +{
> +    type_register_static(&mpc8544ds_info);
> +}
> +
> +type_init(mpc8544ds_register_types)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-2.13 v2 2/5] ppc: e500: switch E500 based machines to full machine definition

[Igor Mammedov]

On Thu, 19 Apr 2018 14:15:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Apr 18, 2018 at 04:28:02PM +0200, Igor Mammedov wrote:
> > Convert PPCE500Params to PCCE500MachineClass which it essentially is,
> > and introduce PCCE500MachineState to keep track of E500 specific
> > state instead of adding global variables or extra parameters to
> > functions when we need to keep data beyond machine init
> > (i.e. make it look like typical fully defined machine).
> > 
> > It's pretty shallow conversion instead of currently used trivial
> > DEFINE_MACHINE() macro. It adds extra 60LOC of boilerplate code
> > of full machine definition.
> > 
> > The patch on top[1] will use PCCE500MachineState to keep track of
> > platform_bus device and add E500Plate specific machine class
> > to use HOTPLUG_HANDLER for explicitly initializing dynamic
> > sysbus devices at the time they are added instead of delaying
> > it to machine done time by platform_bus_init_notify() which is
> > being removed.
> > 
> > 1)  <1523551221-11612-3-git-send-email-imammedo@redhat.com>
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>  
> 
> As noted elsewhere, I've already merged this into my ppc-for-2.13
> tree.  However, don't let that stop you from posting and/or queueing
> it elsewhere.  Whoever ends up merging first once 2.13 opens, it
> should be easy to resolve.
Yep, that's been intention, i.e. have whole series on list so
reader won't have to hunt for queued dependencies in different trees.
After all it's trivial to fix merge conflict when path is merged
in several trees.

Re: [Qemu-devel] [PATCH for-2.13 v2 2/5] ppc: e500: switch E500 based machines to full machine definition

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]

On Thu, Apr 19, 2018 at 10:30:19AM +0200, Igor Mammedov wrote:
> On Thu, 19 Apr 2018 14:15:20 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Apr 18, 2018 at 04:28:02PM +0200, Igor Mammedov wrote:
> > > Convert PPCE500Params to PCCE500MachineClass which it essentially is,
> > > and introduce PCCE500MachineState to keep track of E500 specific
> > > state instead of adding global variables or extra parameters to
> > > functions when we need to keep data beyond machine init
> > > (i.e. make it look like typical fully defined machine).
> > > 
> > > It's pretty shallow conversion instead of currently used trivial
> > > DEFINE_MACHINE() macro. It adds extra 60LOC of boilerplate code
> > > of full machine definition.
> > > 
> > > The patch on top[1] will use PCCE500MachineState to keep track of
> > > platform_bus device and add E500Plate specific machine class
> > > to use HOTPLUG_HANDLER for explicitly initializing dynamic
> > > sysbus devices at the time they are added instead of delaying
> > > it to machine done time by platform_bus_init_notify() which is
> > > being removed.
> > > 
> > > 1)  <1523551221-11612-3-git-send-email-imammedo@redhat.com>
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>  
> > 
> > As noted elsewhere, I've already merged this into my ppc-for-2.13
> > tree.  However, don't let that stop you from posting and/or queueing
> > it elsewhere.  Whoever ends up merging first once 2.13 opens, it
> > should be easy to resolve.
> Yep, that's been intention, i.e. have whole series on list so
> reader won't have to hunt for queued dependencies in different trees.
> After all it's trivial to fix merge conflict when path is merged
> in several trees.

Right, I figured.  Just making sure it was clear that me merging it
didn't mean I wanted you to exclude it from your own queue.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH for-2.13 v2 3/5] pc: simplify MachineClass::get_hotplug_handler handling

[Igor Mammedov]

By default MachineClass::get_hotplug_handler is NULL and concrete board
should set it to it's own handler.
Considering there isn't any default handler, drop saving empty
MachineClass::get_hotplug_handler in child class and make PC code
consistent with spapr/s390x boards.

We can bring this back when actual usecase surfaces and do it
consistently across boards that use get_hotplug_handler().

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: mst@redhat.com
CC: ehabkost@redhat.com
---
 include/hw/i386/pc.h | 8 --------
 hw/i386/pc.c         | 6 +-----
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ffee841..fac6689 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -84,10 +84,6 @@ struct PCMachineState {
 /**
  * PCMachineClass:
  *
- * Methods:
- *
- * @get_hotplug_handler: pointer to parent class callback @get_hotplug_handler
- *
  * Compat fields:
  *
  * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
@@ -107,10 +103,6 @@ struct PCMachineClass {
 
     /*< public >*/
 
-    /* Methods: */
-    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
-                                           DeviceState *dev);
-
     /* Device configuration: */
     bool pci_enabled;
     bool kvmclock_enabled;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d36bac8..bc144d2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2056,15 +2056,12 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
         object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         return HOTPLUG_HANDLER(machine);
     }
 
-    return pcmc->get_hotplug_handler ?
-        pcmc->get_hotplug_handler(machine, dev) : NULL;
+    return NULL;
 }
 
 static void
@@ -2344,7 +2341,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
 
-    pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     pcmc->pci_enabled = true;
     pcmc->has_acpi_build = true;
     pcmc->rsdp_in_ram = true;
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.13 v2 3/5] pc: simplify MachineClass::get_hotplug_handler handling

[Eduardo Habkost]

On Wed, Apr 18, 2018 at 04:28:03PM +0200, Igor Mammedov wrote:
> By default MachineClass::get_hotplug_handler is NULL and concrete board
> should set it to it's own handler.
> Considering there isn't any default handler, drop saving empty
> MachineClass::get_hotplug_handler in child class and make PC code
> consistent with spapr/s390x boards.
> 
> We can bring this back when actual usecase surfaces and do it
> consistently across boards that use get_hotplug_handler().
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


[...]
> @@ -2344,7 +2341,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>      NMIClass *nc = NMI_CLASS(oc);
>  
> -    pcmc->get_hotplug_handler = mc->get_hotplug_handler;

What about assert(!mc->get_hotplug_handler) just before
overwriting it?

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.13 v2 3/5] pc: simplify MachineClass::get_hotplug_handler handling

[Philippe Mathieu-Daudé]

On 04/18/2018 11:56 AM, Eduardo Habkost wrote:
> On Wed, Apr 18, 2018 at 04:28:03PM +0200, Igor Mammedov wrote:
>> By default MachineClass::get_hotplug_handler is NULL and concrete board
>> should set it to it's own handler.
>> Considering there isn't any default handler, drop saving empty
>> MachineClass::get_hotplug_handler in child class and make PC code
>> consistent with spapr/s390x boards.
>>
>> We can bring this back when actual usecase surfaces and do it
>> consistently across boards that use get_hotplug_handler().
>>
>> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> [...]
>> @@ -2344,7 +2341,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>>      NMIClass *nc = NMI_CLASS(oc);
>>  
>> -    pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> 
> What about assert(!mc->get_hotplug_handler) just before
> overwriting it?

Good idea.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH for-2.13 v2 3/5] pc: simplify MachineClass::get_hotplug_handler handling

[Igor Mammedov]

On Wed, 18 Apr 2018 11:56:03 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 18, 2018 at 04:28:03PM +0200, Igor Mammedov wrote:
> > By default MachineClass::get_hotplug_handler is NULL and concrete board
> > should set it to it's own handler.
> > Considering there isn't any default handler, drop saving empty
> > MachineClass::get_hotplug_handler in child class and make PC code
> > consistent with spapr/s390x boards.
> > 
> > We can bring this back when actual usecase surfaces and do it
> > consistently across boards that use get_hotplug_handler().
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> [...]
> > @@ -2344,7 +2341,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >      NMIClass *nc = NMI_CLASS(oc);
> >  
> > -    pcmc->get_hotplug_handler = mc->get_hotplug_handler;  
> 
> What about assert(!mc->get_hotplug_handler) just before
> overwriting it?
I'll make it patch on top of series to make sure that
every board that sets get_hotplug_handler will do it
consistently.

[Qemu-devel] [PATCH for-2.13 v2 4/5] platform-bus-device: use device plug callback instead of machine_done notifier

[Igor Mammedov]

platform-bus were using machine_done notifier to get and map
(assign irq/mmio resources) dynamically added sysbus devices
after all '-device' options had been processed.
That however creates non obvious dependencies on ordering of
machine_done notifiers and requires carefull line juggling
to keep it working. For example see comment above
create_platform_bus() and 'straitforward' arm_load_kernel()
had to converted to machine_done notifier and that lead to
yet another machine_done notifier to keep it working
arm_register_platform_bus_fdt_creator().

Instead of hiding resource assignment in platform-bus-device
to magically initialize sysbus devices, use device plug
callback and assign resources explicitly at board level
at the moment each -device option is being processed.

That adds a bunch of machine declaration boiler plate to
e500plat board, similar to ARM/x86 but gets rid of hidden
machine_done notifier and would allow to remove the dependent
notifiers in ARM code simplifying it and making code flow
easier to follow.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: agraf@suse.de
CC: david@gibson.dropbear.id.au
CC: qemu-ppc@nongnu.org

v2:
  - don't save original MachineClass::get_hotplug_handler, just overwrite it.
    (there isn't any use case for chaining get_hotplug_handler() yet,
     so keep things simple for now) (David Gibson <david@gibson.dropbear.id.au>)
  - s/hotpug/hotplug/ (Peter Maydell <peter.maydell@linaro.org>)
  - ppc: rebase on top (ppc: e500: switch E500 based machines to full  machine definition)
---
 hw/ppc/e500.h             |  5 +++++
 include/hw/arm/virt.h     |  1 +
 include/hw/platform-bus.h |  4 ++--
 hw/arm/sysbus-fdt.c       |  3 ---
 hw/arm/virt.c             | 31 +++++++++++++++++++++++++++++++
 hw/core/platform-bus.c    | 29 +++++------------------------
 hw/ppc/e500.c             | 38 +++++++++++++++++---------------------
 hw/ppc/e500plat.c         | 31 +++++++++++++++++++++++++++++++
 8 files changed, 92 insertions(+), 50 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 621403b..3fd9f82 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -2,11 +2,16 @@
 #define PPCE500_H
 
 #include "hw/boards.h"
+#include "hw/platform-bus.h"
 
 typedef struct PPCE500MachineState {
     /*< private >*/
     MachineState parent_obj;
 
+    /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
+     * board supports dynamic sysbus devices
+     */
+    PlatformBusDevice *pbus_dev;
 } PPCE500MachineState;
 
 typedef struct PPCE500MachineClass {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ba0c1a4..e4e3e46 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -91,6 +91,7 @@ typedef struct {
 typedef struct {
     MachineState parent;
     Notifier machine_done;
+    DeviceState *platform_bus_dev;
     FWCfgState *fw_cfg;
     bool secure;
     bool highmem;
diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
index a00775c..19e20c5 100644
--- a/include/hw/platform-bus.h
+++ b/include/hw/platform-bus.h
@@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
 struct PlatformBusDevice {
     /*< private >*/
     SysBusDevice parent_obj;
-    Notifier notifier;
-    bool done_gathering;
 
     /*< public >*/
     uint32_t mmio_size;
@@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, SysBusDevice *sbdev,
 hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
                                   int n);
 
+void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
+
 #endif /* HW_PLATFORM_BUS_H */
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dc..80ff70e 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -506,9 +506,6 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
     dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
     pbus = PLATFORM_BUS_DEVICE(dev);
 
-    /* We can only create dt nodes for dynamic devices when they're ready */
-    assert(pbus->done_gathering);
-
     PlatformBusFDTData data = {
         .fdt = fdt,
         .irq_start = irq_start,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 94dcb12..112c367 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
     qdev_prop_set_uint32(dev, "mmio_size",
         platform_bus_params.platform_bus_size);
     qdev_init_nofail(dev);
+    vms->platform_bus_dev = dev;
     s = SYS_BUS_DEVICE(dev);
 
     for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
@@ -1536,9 +1537,33 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
+static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        if (vms->platform_bus_dev) {
+            platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
+                                     SYS_BUS_DEVICE(dev));
+        }
+    }
+}
+
+static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
+                                                        DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return NULL;
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = machvirt_init;
     /* Start max_cpus at the maximum QEMU supports. We'll further restrict
@@ -1557,6 +1582,8 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
+    hc->plug = virt_machine_device_plug_cb;
 }
 
 static const TypeInfo virt_machine_info = {
@@ -1566,6 +1593,10 @@ static const TypeInfo virt_machine_info = {
     .instance_size = sizeof(VirtMachineState),
     .class_size    = sizeof(VirtMachineClass),
     .class_init    = virt_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    },
 };
 
 static void machvirt_machine_init(void)
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index 33d32fb..807cb5c 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -103,7 +103,6 @@ static void plaform_bus_refresh_irqs(PlatformBusDevice *pbus)
 {
     bitmap_zero(pbus->used_irqs, pbus->num_irqs);
     foreach_dynamic_sysbus_device(platform_bus_count_irqs, pbus);
-    pbus->done_gathering = true;
 }
 
 static void platform_bus_map_irq(PlatformBusDevice *pbus, SysBusDevice *sbdev,
@@ -163,12 +162,11 @@ static void platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
 }
 
 /*
- * For each sysbus device, look for unassigned IRQ lines as well as
- * unassociated MMIO regions. Connect them to the platform bus if available.
+ * Look for unassigned IRQ lines as well as unassociated MMIO regions.
+ * Connect them to the platform bus if available.
  */
-static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
+void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev)
 {
-    PlatformBusDevice *pbus = opaque;
     int i;
 
     for (i = 0; sysbus_has_irq(sbdev, i); i++) {
@@ -180,19 +178,6 @@ static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static void platform_bus_init_notify(Notifier *notifier, void *data)
-{
-    PlatformBusDevice *pb = container_of(notifier, PlatformBusDevice, notifier);
-
-    /*
-     * Generate a bitmap of used IRQ lines, as the user might have specified
-     * them on the command line.
-     */
-    plaform_bus_refresh_irqs(pb);
-
-    foreach_dynamic_sysbus_device(link_sysbus_device, pb);
-}
-
 static void platform_bus_realize(DeviceState *dev, Error **errp)
 {
     PlatformBusDevice *pbus;
@@ -211,12 +196,8 @@ static void platform_bus_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(d, &pbus->irqs[i]);
     }
 
-    /*
-     * Register notifier that allows us to gather dangling devices once the
-     * machine is completely assembled
-     */
-    pbus->notifier.notify = platform_bus_init_notify;
-    qemu_add_machine_init_done_notifier(&pbus->notifier);
+    /* some devices might be initialized before so update used IRQs map */
+    plaform_bus_refresh_irqs(pbus);
 }
 
 static Property platform_bus_properties[] = {
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 30b42a8..e91a5f6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -221,16 +221,15 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
+static void platform_bus_create_devtree(PPCE500MachineState *pms,
                                         void *fdt, const char *mpic)
 {
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
     gchar *node = g_strdup_printf("/platform@%"PRIx64, pmc->platform_bus_base);
     const char platcomp[] = "qemu,platform\0simple-bus";
     uint64_t addr = pmc->platform_bus_base;
     uint64_t size = pmc->platform_bus_size;
     int irq_start = pmc->platform_bus_first_irq;
-    PlatformBusDevice *pbus;
-    DeviceState *dev;
 
     /* Create a /platform node that we can put all devices into */
 
@@ -245,22 +244,17 @@ static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
 
     qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
 
-    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
-    pbus = PLATFORM_BUS_DEVICE(dev);
-
-    /* We can only create dt nodes for dynamic devices when they're ready */
-    if (pbus->done_gathering) {
-        PlatformDevtreeData data = {
-            .fdt = fdt,
-            .mpic = mpic,
-            .irq_start = irq_start,
-            .node = node,
-            .pbus = pbus,
-        };
+    /* Create dt nodes for dynamic devices */
+    PlatformDevtreeData data = {
+        .fdt = fdt,
+        .mpic = mpic,
+        .irq_start = irq_start,
+        .node = node,
+        .pbus = pms->pbus_dev,
+    };
 
-        /* Loop through all dynamic sysbus devices and create nodes for them */
-        foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
-    }
+    /* Loop through all dynamic sysbus devices and create nodes for them */
+    foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
 
     g_free(node);
 }
@@ -527,8 +521,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
         create_dt_mpc8xxx_gpio(fdt, soc, mpic);
     }
 
-    if (pmc->has_platform_bus) {
-        platform_bus_create_devtree(pmc, fdt, mpic);
+    if (pms->pbus_dev) {
+        platform_bus_create_devtree(pms, fdt, mpic);
     }
 
     pmc->fixup_devtree(fdt);
@@ -946,8 +940,9 @@ void ppce500_init(MachineState *machine)
         qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
         qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
         qdev_init_nofail(dev);
-        s = SYS_BUS_DEVICE(dev);
+        pms->pbus_dev = PLATFORM_BUS_DEVICE(dev);
 
+        s = SYS_BUS_DEVICE(pms->pbus_dev);
         for (i = 0; i < pmc->platform_bus_num_irqs; i++) {
             int irqn = pmc->platform_bus_first_irq + i;
             sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
@@ -1090,6 +1085,7 @@ static const TypeInfo ppce500_info = {
     .name          = TYPE_PPCE500_MACHINE,
     .parent        = TYPE_MACHINE,
     .abstract      = true,
+    .instance_size = sizeof(PPCE500MachineState),
     .class_size    = sizeof(PPCE500MachineClass),
 };
 
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index f69aadb..1a469ba 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -43,13 +43,40 @@ static void e500plat_init(MachineState *machine)
     ppce500_init(machine);
 }
 
+static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                            DeviceState *dev, Error **errp)
+{
+    PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
+
+    if (pms->pbus_dev) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+            platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
+        }
+    }
+}
+
+static
+HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
+                                                    DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return NULL;
+}
+
 #define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
 
 static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
     PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     MachineClass *mc = MACHINE_CLASS(oc);
 
+    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
+    hc->plug = e500plat_machine_device_plug_cb;
+
     pmc->pci_first_slot = 0x1;
     pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
     pmc->fixup_devtree = e500plat_fixup_devtree;
@@ -77,6 +104,10 @@ static const TypeInfo e500plat_info = {
     .name          = TYPE_E500PLAT_MACHINE,
     .parent        = TYPE_PPCE500_MACHINE,
     .class_init    = e500plat_machine_class_init,
+    .interfaces    = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    }
 };
 
 static void e500plat_register_types(void)
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.13 v2 4/5] platform-bus-device: use device plug callback instead of machine_done notifier

[Philippe Mathieu-Daudé]

On 04/18/2018 11:28 AM, Igor Mammedov wrote:
> platform-bus were using machine_done notifier to get and map
> (assign irq/mmio resources) dynamically added sysbus devices
> after all '-device' options had been processed.
> That however creates non obvious dependencies on ordering of
> machine_done notifiers and requires carefull line juggling
> to keep it working. For example see comment above
> create_platform_bus() and 'straitforward' arm_load_kernel()
> had to converted to machine_done notifier and that lead to
> yet another machine_done notifier to keep it working
> arm_register_platform_bus_fdt_creator().
> 
> Instead of hiding resource assignment in platform-bus-device
> to magically initialize sysbus devices, use device plug
> callback and assign resources explicitly at board level
> at the moment each -device option is being processed.
> 
> That adds a bunch of machine declaration boiler plate to
> e500plat board, similar to ARM/x86 but gets rid of hidden
> machine_done notifier and would allow to remove the dependent
> notifiers in ARM code simplifying it and making code flow
> easier to follow.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: agraf@suse.de
> CC: david@gibson.dropbear.id.au
> CC: qemu-ppc@nongnu.org
> 
> v2:
>   - don't save original MachineClass::get_hotplug_handler, just overwrite it.
>     (there isn't any use case for chaining get_hotplug_handler() yet,
>      so keep things simple for now) (David Gibson <david@gibson.dropbear.id.au>)
>   - s/hotpug/hotplug/ (Peter Maydell <peter.maydell@linaro.org>)
>   - ppc: rebase on top (ppc: e500: switch E500 based machines to full  machine definition)
> ---
>  hw/ppc/e500.h             |  5 +++++
>  include/hw/arm/virt.h     |  1 +
>  include/hw/platform-bus.h |  4 ++--
>  hw/arm/sysbus-fdt.c       |  3 ---
>  hw/arm/virt.c             | 31 +++++++++++++++++++++++++++++++
>  hw/core/platform-bus.c    | 29 +++++------------------------
>  hw/ppc/e500.c             | 38 +++++++++++++++++---------------------
>  hw/ppc/e500plat.c         | 31 +++++++++++++++++++++++++++++++
>  8 files changed, 92 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 621403b..3fd9f82 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -2,11 +2,16 @@
>  #define PPCE500_H
>  
>  #include "hw/boards.h"
> +#include "hw/platform-bus.h"
>  
>  typedef struct PPCE500MachineState {
>      /*< private >*/
>      MachineState parent_obj;
>  
> +    /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
> +     * board supports dynamic sysbus devices
> +     */
> +    PlatformBusDevice *pbus_dev;
>  } PPCE500MachineState;
>  
>  typedef struct PPCE500MachineClass {
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ba0c1a4..e4e3e46 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -91,6 +91,7 @@ typedef struct {
>  typedef struct {
>      MachineState parent;
>      Notifier machine_done;
> +    DeviceState *platform_bus_dev;
>      FWCfgState *fw_cfg;
>      bool secure;
>      bool highmem;
> diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> index a00775c..19e20c5 100644
> --- a/include/hw/platform-bus.h
> +++ b/include/hw/platform-bus.h
> @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
>  struct PlatformBusDevice {
>      /*< private >*/
>      SysBusDevice parent_obj;
> -    Notifier notifier;
> -    bool done_gathering;
>  
>      /*< public >*/
>      uint32_t mmio_size;
> @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, SysBusDevice *sbdev,
>  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
>                                    int n);
>  
> +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
> +
>  #endif /* HW_PLATFORM_BUS_H */
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d68e3dc..80ff70e 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -506,9 +506,6 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
>      dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
>      pbus = PLATFORM_BUS_DEVICE(dev);
>  
> -    /* We can only create dt nodes for dynamic devices when they're ready */
> -    assert(pbus->done_gathering);
> -
>      PlatformBusFDTData data = {
>          .fdt = fdt,
>          .irq_start = irq_start,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 94dcb12..112c367 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
>      qdev_prop_set_uint32(dev, "mmio_size",
>          platform_bus_params.platform_bus_size);
>      qdev_init_nofail(dev);
> +    vms->platform_bus_dev = dev;
>      s = SYS_BUS_DEVICE(dev);
>  
>      for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
> @@ -1536,9 +1537,33 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>      return ms->possible_cpus;
>  }
>  
> +static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        if (vms->platform_bus_dev) {

Inverting the if conditions is probably cheaper.

> +            platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
> +                                     SYS_BUS_DEVICE(dev));
> +        }
> +    }
> +}
> +
> +static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
> +                                                        DeviceState *dev)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return NULL;
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      mc->init = machvirt_init;
>      /* Start max_cpus at the maximum QEMU supports. We'll further restrict
> @@ -1557,6 +1582,8 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> +    hc->plug = virt_machine_device_plug_cb;
>  }
>  
>  static const TypeInfo virt_machine_info = {
> @@ -1566,6 +1593,10 @@ static const TypeInfo virt_machine_info = {
>      .instance_size = sizeof(VirtMachineState),
>      .class_size    = sizeof(VirtMachineClass),
>      .class_init    = virt_machine_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +         { TYPE_HOTPLUG_HANDLER },
> +         { }
> +    },
>  };
>  
>  static void machvirt_machine_init(void)
> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
> index 33d32fb..807cb5c 100644
> --- a/hw/core/platform-bus.c
> +++ b/hw/core/platform-bus.c
> @@ -103,7 +103,6 @@ static void plaform_bus_refresh_irqs(PlatformBusDevice *pbus)
>  {
>      bitmap_zero(pbus->used_irqs, pbus->num_irqs);
>      foreach_dynamic_sysbus_device(platform_bus_count_irqs, pbus);
> -    pbus->done_gathering = true;
>  }
>  
>  static void platform_bus_map_irq(PlatformBusDevice *pbus, SysBusDevice *sbdev,
> @@ -163,12 +162,11 @@ static void platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
>  }
>  
>  /*
> - * For each sysbus device, look for unassigned IRQ lines as well as
> - * unassociated MMIO regions. Connect them to the platform bus if available.
> + * Look for unassigned IRQ lines as well as unassociated MMIO regions.
> + * Connect them to the platform bus if available.
>   */
> -static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
> +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev)
>  {
> -    PlatformBusDevice *pbus = opaque;
>      int i;
>  
>      for (i = 0; sysbus_has_irq(sbdev, i); i++) {
> @@ -180,19 +178,6 @@ static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
>      }
>  }
>  
> -static void platform_bus_init_notify(Notifier *notifier, void *data)
> -{
> -    PlatformBusDevice *pb = container_of(notifier, PlatformBusDevice, notifier);
> -
> -    /*
> -     * Generate a bitmap of used IRQ lines, as the user might have specified
> -     * them on the command line.
> -     */
> -    plaform_bus_refresh_irqs(pb);
> -
> -    foreach_dynamic_sysbus_device(link_sysbus_device, pb);
> -}
> -
>  static void platform_bus_realize(DeviceState *dev, Error **errp)
>  {
>      PlatformBusDevice *pbus;
> @@ -211,12 +196,8 @@ static void platform_bus_realize(DeviceState *dev, Error **errp)
>          sysbus_init_irq(d, &pbus->irqs[i]);
>      }
>  
> -    /*
> -     * Register notifier that allows us to gather dangling devices once the
> -     * machine is completely assembled
> -     */
> -    pbus->notifier.notify = platform_bus_init_notify;
> -    qemu_add_machine_init_done_notifier(&pbus->notifier);
> +    /* some devices might be initialized before so update used IRQs map */
> +    plaform_bus_refresh_irqs(pbus);
>  }
>  
>  static Property platform_bus_properties[] = {
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 30b42a8..e91a5f6 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -221,16 +221,15 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
>      }
>  }
>  
> -static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
> +static void platform_bus_create_devtree(PPCE500MachineState *pms,
>                                          void *fdt, const char *mpic)
>  {
> +    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
>      gchar *node = g_strdup_printf("/platform@%"PRIx64, pmc->platform_bus_base);
>      const char platcomp[] = "qemu,platform\0simple-bus";
>      uint64_t addr = pmc->platform_bus_base;
>      uint64_t size = pmc->platform_bus_size;
>      int irq_start = pmc->platform_bus_first_irq;
> -    PlatformBusDevice *pbus;
> -    DeviceState *dev;
>  
>      /* Create a /platform node that we can put all devices into */
>  
> @@ -245,22 +244,17 @@ static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
>  
>      qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
>  
> -    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
> -    pbus = PLATFORM_BUS_DEVICE(dev);
> -
> -    /* We can only create dt nodes for dynamic devices when they're ready */
> -    if (pbus->done_gathering) {
> -        PlatformDevtreeData data = {
> -            .fdt = fdt,
> -            .mpic = mpic,
> -            .irq_start = irq_start,
> -            .node = node,
> -            .pbus = pbus,
> -        };
> +    /* Create dt nodes for dynamic devices */
> +    PlatformDevtreeData data = {
> +        .fdt = fdt,
> +        .mpic = mpic,
> +        .irq_start = irq_start,
> +        .node = node,
> +        .pbus = pms->pbus_dev,
> +    };
>  
> -        /* Loop through all dynamic sysbus devices and create nodes for them */
> -        foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
> -    }
> +    /* Loop through all dynamic sysbus devices and create nodes for them */
> +    foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
>  
>      g_free(node);
>  }
> @@ -527,8 +521,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
>          create_dt_mpc8xxx_gpio(fdt, soc, mpic);
>      }
>  
> -    if (pmc->has_platform_bus) {
> -        platform_bus_create_devtree(pmc, fdt, mpic);
> +    if (pms->pbus_dev) {
> +        platform_bus_create_devtree(pms, fdt, mpic);
>      }
>  
>      pmc->fixup_devtree(fdt);
> @@ -946,8 +940,9 @@ void ppce500_init(MachineState *machine)
>          qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
>          qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
>          qdev_init_nofail(dev);
> -        s = SYS_BUS_DEVICE(dev);
> +        pms->pbus_dev = PLATFORM_BUS_DEVICE(dev);
>  
> +        s = SYS_BUS_DEVICE(pms->pbus_dev);
>          for (i = 0; i < pmc->platform_bus_num_irqs; i++) {
>              int irqn = pmc->platform_bus_first_irq + i;
>              sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> @@ -1090,6 +1085,7 @@ static const TypeInfo ppce500_info = {
>      .name          = TYPE_PPCE500_MACHINE,
>      .parent        = TYPE_MACHINE,
>      .abstract      = true,
> +    .instance_size = sizeof(PPCE500MachineState),
>      .class_size    = sizeof(PPCE500MachineClass),
>  };
>  
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index f69aadb..1a469ba 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -43,13 +43,40 @@ static void e500plat_init(MachineState *machine)
>      ppce500_init(machine);
>  }
>  
> +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                            DeviceState *dev, Error **errp)
> +{
> +    PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
> +
> +    if (pms->pbus_dev) {
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {

Here you do it!

> +            platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
> +        }
> +    }
> +}
> +
> +static
> +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> +                                                    DeviceState *dev)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return NULL;
> +}
> +
>  #define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
>  
>  static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>  {
>      PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>      MachineClass *mc = MACHINE_CLASS(oc);
>  
> +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
> +    hc->plug = e500plat_machine_device_plug_cb;
> +
>      pmc->pci_first_slot = 0x1;
>      pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
>      pmc->fixup_devtree = e500plat_fixup_devtree;
> @@ -77,6 +104,10 @@ static const TypeInfo e500plat_info = {
>      .name          = TYPE_E500PLAT_MACHINE,
>      .parent        = TYPE_PPCE500_MACHINE,
>      .class_init    = e500plat_machine_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +         { TYPE_HOTPLUG_HANDLER },
> +         { }
> +    }
>  };
>  
>  static void e500plat_register_types(void)
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[Qemu-devel] [PATCH for-2.13 v3 4/5] platform-bus-device: use device plug callback instead of machine_done notifier

[Igor Mammedov]

platform-bus were using machine_done notifier to get and map
(assign irq/mmio resources) dynamically added sysbus devices
after all '-device' options had been processed.
That however creates non obvious dependencies on ordering of
machine_done notifiers and requires carefull line juggling
to keep it working. For example see comment above
create_platform_bus() and 'straitforward' arm_load_kernel()
had to converted to machine_done notifier and that lead to
yet another machine_done notifier to keep it working
arm_register_platform_bus_fdt_creator().

Instead of hiding resource assignment in platform-bus-device
to magically initialize sysbus devices, use device plug
callback and assign resources explicitly at board level
at the moment each -device option is being processed.

That adds a bunch of machine declaration boiler plate to
e500plat board, similar to ARM/x86 but gets rid of hidden
machine_done notifier and would allow to remove the dependent
notifiers in ARM code simplifying it and making code flow
easier to follow.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
CC: agraf@suse.de
CC: david@gibson.dropbear.id.au
CC: qemu-ppc@nongnu.org
CC: peter.maydell@linaro.org
CC: eric.auger@redhat.com
CC: qemu-arm@nongnu.org

v2:
  - don't save original MachineClass::get_hotplug_handler, just overwrite it.
    (there isn't any use case for chaining get_hotplug_handler() yet,
     so keep things simple for now) (David Gibson <david@gibson.dropbear.id.au>)
  - s/hotpug/hotplug/ (Peter Maydell <peter.maydell@linaro.org>)
  - ppc: rebase on top (ppc: e500: switch E500 based machines to full  machine definition)
v3:
  - in virt_machine_device_plug_cb() check for vms->platform_bus_dev != NULL first
    before doing more expensive cast check to SYS_BUS_DEVICE
    (Philippe Mathieu-Daudé <f4bug@amsat.org>)
---
 hw/ppc/e500.h             |  5 +++++
 include/hw/arm/virt.h     |  1 +
 include/hw/platform-bus.h |  4 ++--
 hw/arm/sysbus-fdt.c       |  3 ---
 hw/arm/virt.c             | 31 +++++++++++++++++++++++++++++++
 hw/core/platform-bus.c    | 29 +++++------------------------
 hw/ppc/e500.c             | 38 +++++++++++++++++---------------------
 hw/ppc/e500plat.c         | 31 +++++++++++++++++++++++++++++++
 8 files changed, 92 insertions(+), 50 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 621403b..3fd9f82 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -2,11 +2,16 @@
 #define PPCE500_H
 
 #include "hw/boards.h"
+#include "hw/platform-bus.h"
 
 typedef struct PPCE500MachineState {
     /*< private >*/
     MachineState parent_obj;
 
+    /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
+     * board supports dynamic sysbus devices
+     */
+    PlatformBusDevice *pbus_dev;
 } PPCE500MachineState;
 
 typedef struct PPCE500MachineClass {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ba0c1a4..e4e3e46 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -91,6 +91,7 @@ typedef struct {
 typedef struct {
     MachineState parent;
     Notifier machine_done;
+    DeviceState *platform_bus_dev;
     FWCfgState *fw_cfg;
     bool secure;
     bool highmem;
diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
index a00775c..19e20c5 100644
--- a/include/hw/platform-bus.h
+++ b/include/hw/platform-bus.h
@@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
 struct PlatformBusDevice {
     /*< private >*/
     SysBusDevice parent_obj;
-    Notifier notifier;
-    bool done_gathering;
 
     /*< public >*/
     uint32_t mmio_size;
@@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, SysBusDevice *sbdev,
 hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
                                   int n);
 
+void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
+
 #endif /* HW_PLATFORM_BUS_H */
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dc..80ff70e 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -506,9 +506,6 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
     dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
     pbus = PLATFORM_BUS_DEVICE(dev);
 
-    /* We can only create dt nodes for dynamic devices when they're ready */
-    assert(pbus->done_gathering);
-
     PlatformBusFDTData data = {
         .fdt = fdt,
         .irq_start = irq_start,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 94dcb12..9d761b5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
     qdev_prop_set_uint32(dev, "mmio_size",
         platform_bus_params.platform_bus_size);
     qdev_init_nofail(dev);
+    vms->platform_bus_dev = dev;
     s = SYS_BUS_DEVICE(dev);
 
     for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
@@ -1536,9 +1537,33 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
+static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+    if (vms->platform_bus_dev) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+            platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
+                                     SYS_BUS_DEVICE(dev));
+        }
+    }
+}
+
+static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
+                                                        DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return NULL;
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = machvirt_init;
     /* Start max_cpus at the maximum QEMU supports. We'll further restrict
@@ -1557,6 +1582,8 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
+    hc->plug = virt_machine_device_plug_cb;
 }
 
 static const TypeInfo virt_machine_info = {
@@ -1566,6 +1593,10 @@ static const TypeInfo virt_machine_info = {
     .instance_size = sizeof(VirtMachineState),
     .class_size    = sizeof(VirtMachineClass),
     .class_init    = virt_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    },
 };
 
 static void machvirt_machine_init(void)
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index 33d32fb..807cb5c 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -103,7 +103,6 @@ static void plaform_bus_refresh_irqs(PlatformBusDevice *pbus)
 {
     bitmap_zero(pbus->used_irqs, pbus->num_irqs);
     foreach_dynamic_sysbus_device(platform_bus_count_irqs, pbus);
-    pbus->done_gathering = true;
 }
 
 static void platform_bus_map_irq(PlatformBusDevice *pbus, SysBusDevice *sbdev,
@@ -163,12 +162,11 @@ static void platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
 }
 
 /*
- * For each sysbus device, look for unassigned IRQ lines as well as
- * unassociated MMIO regions. Connect them to the platform bus if available.
+ * Look for unassigned IRQ lines as well as unassociated MMIO regions.
+ * Connect them to the platform bus if available.
  */
-static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
+void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev)
 {
-    PlatformBusDevice *pbus = opaque;
     int i;
 
     for (i = 0; sysbus_has_irq(sbdev, i); i++) {
@@ -180,19 +178,6 @@ static void link_sysbus_device(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static void platform_bus_init_notify(Notifier *notifier, void *data)
-{
-    PlatformBusDevice *pb = container_of(notifier, PlatformBusDevice, notifier);
-
-    /*
-     * Generate a bitmap of used IRQ lines, as the user might have specified
-     * them on the command line.
-     */
-    plaform_bus_refresh_irqs(pb);
-
-    foreach_dynamic_sysbus_device(link_sysbus_device, pb);
-}
-
 static void platform_bus_realize(DeviceState *dev, Error **errp)
 {
     PlatformBusDevice *pbus;
@@ -211,12 +196,8 @@ static void platform_bus_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(d, &pbus->irqs[i]);
     }
 
-    /*
-     * Register notifier that allows us to gather dangling devices once the
-     * machine is completely assembled
-     */
-    pbus->notifier.notify = platform_bus_init_notify;
-    qemu_add_machine_init_done_notifier(&pbus->notifier);
+    /* some devices might be initialized before so update used IRQs map */
+    plaform_bus_refresh_irqs(pbus);
 }
 
 static Property platform_bus_properties[] = {
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 30b42a8..e91a5f6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -221,16 +221,15 @@ static void sysbus_device_create_devtree(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
+static void platform_bus_create_devtree(PPCE500MachineState *pms,
                                         void *fdt, const char *mpic)
 {
+    const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(pms);
     gchar *node = g_strdup_printf("/platform@%"PRIx64, pmc->platform_bus_base);
     const char platcomp[] = "qemu,platform\0simple-bus";
     uint64_t addr = pmc->platform_bus_base;
     uint64_t size = pmc->platform_bus_size;
     int irq_start = pmc->platform_bus_first_irq;
-    PlatformBusDevice *pbus;
-    DeviceState *dev;
 
     /* Create a /platform node that we can put all devices into */
 
@@ -245,22 +244,17 @@ static void platform_bus_create_devtree(const PPCE500MachineClass *pmc,
 
     qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
 
-    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
-    pbus = PLATFORM_BUS_DEVICE(dev);
-
-    /* We can only create dt nodes for dynamic devices when they're ready */
-    if (pbus->done_gathering) {
-        PlatformDevtreeData data = {
-            .fdt = fdt,
-            .mpic = mpic,
-            .irq_start = irq_start,
-            .node = node,
-            .pbus = pbus,
-        };
+    /* Create dt nodes for dynamic devices */
+    PlatformDevtreeData data = {
+        .fdt = fdt,
+        .mpic = mpic,
+        .irq_start = irq_start,
+        .node = node,
+        .pbus = pms->pbus_dev,
+    };
 
-        /* Loop through all dynamic sysbus devices and create nodes for them */
-        foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
-    }
+    /* Loop through all dynamic sysbus devices and create nodes for them */
+    foreach_dynamic_sysbus_device(sysbus_device_create_devtree, &data);
 
     g_free(node);
 }
@@ -527,8 +521,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
         create_dt_mpc8xxx_gpio(fdt, soc, mpic);
     }
 
-    if (pmc->has_platform_bus) {
-        platform_bus_create_devtree(pmc, fdt, mpic);
+    if (pms->pbus_dev) {
+        platform_bus_create_devtree(pms, fdt, mpic);
     }
 
     pmc->fixup_devtree(fdt);
@@ -946,8 +940,9 @@ void ppce500_init(MachineState *machine)
         qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
         qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
         qdev_init_nofail(dev);
-        s = SYS_BUS_DEVICE(dev);
+        pms->pbus_dev = PLATFORM_BUS_DEVICE(dev);
 
+        s = SYS_BUS_DEVICE(pms->pbus_dev);
         for (i = 0; i < pmc->platform_bus_num_irqs; i++) {
             int irqn = pmc->platform_bus_first_irq + i;
             sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
@@ -1090,6 +1085,7 @@ static const TypeInfo ppce500_info = {
     .name          = TYPE_PPCE500_MACHINE,
     .parent        = TYPE_MACHINE,
     .abstract      = true,
+    .instance_size = sizeof(PPCE500MachineState),
     .class_size    = sizeof(PPCE500MachineClass),
 };
 
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index f69aadb..1a469ba 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -43,13 +43,40 @@ static void e500plat_init(MachineState *machine)
     ppce500_init(machine);
 }
 
+static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                            DeviceState *dev, Error **errp)
+{
+    PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
+
+    if (pms->pbus_dev) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+            platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
+        }
+    }
+}
+
+static
+HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
+                                                    DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return NULL;
+}
+
 #define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
 
 static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
     PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     MachineClass *mc = MACHINE_CLASS(oc);
 
+    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
+    hc->plug = e500plat_machine_device_plug_cb;
+
     pmc->pci_first_slot = 0x1;
     pmc->pci_nr_slots = PCI_SLOT_MAX - 1;
     pmc->fixup_devtree = e500plat_fixup_devtree;
@@ -77,6 +104,10 @@ static const TypeInfo e500plat_info = {
     .name          = TYPE_E500PLAT_MACHINE,
     .parent        = TYPE_PPCE500_MACHINE,
     .class_init    = e500plat_machine_class_init,
+    .interfaces    = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    }
 };
 
 static void e500plat_register_types(void)
-- 
2.7.4

[Qemu-devel] [PATCH for-2.13 v2 5/5] arm/boot: split load_dtb() from arm_load_kernel()

[Igor Mammedov]

load_dtb() depends on arm_load_kernel() to figure out place
in RAM where it should be loaded, but it's not required for
arm_load_kernel() to work. Sometimes it's neccesary for
devices added with -device/device_add to be enumerated in
DTB as well, which's lead to [1] and surrounding commits to
add 2 more machine_done notifiers with non obvious ordering
to make dynamic sysbus devices initialization happen in
the right order.

However instead of moving whole arm_load_kernel() in to
machine_done, it's sufficient to move only load_dtb() into
virt_machine_done() notifier and remove ArmLoadKernelNotifier/
/PlatformBusFDTNotifierParams notifiers, which saves us ~90LOC
and simplifies code flow quite a bit.
Later would allow to consolidate DTB generation within one
function for 'mach-virt' board and make it reentrant so it
could generate updated DTB in device hotplug secenarios.

While at it rename load_dtb() to arm_load_dtb() since it's
public now.

Add additional field skip_dtb_autoload to struct arm_boot_info
to allow manual DTB load later in mach-virt and to avoid touching
all other boards to explicitly call arm_load_dtb().

 1) (ac9d32e hw/arm/boot: arm_load_kernel implemented as a machine init done notifier)

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - fix rebase conflicts due to dropped
        [PATCH for-2.13 1/4] arm: reuse  arm_boot_address_space() in armv7m_load_kernel()
  - add doc comment to new skip_dtb_autoload field
---
 include/hw/arm/arm.h        | 44 +++++++++++++++++++++-------
 include/hw/arm/sysbus-fdt.h | 37 +++++------------------
 hw/arm/boot.c               | 71 ++++++++++++---------------------------------
 hw/arm/sysbus-fdt.c         | 64 ++++------------------------------------
 hw/arm/virt.c               | 64 +++++++++++++++++++---------------------
 5 files changed, 95 insertions(+), 185 deletions(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index ce769bd..7039956 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -39,15 +39,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
  */
 void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size);
 
-/*
- * struct used as a parameter of the arm_load_kernel machine init
- * done notifier
- */
-typedef struct {
-    Notifier notifier; /* actual notifier */
-    ARMCPU *cpu; /* handle to the first cpu object */
-} ArmLoadKernelNotifier;
-
 /* arm_boot.c */
 struct arm_boot_info {
     uint64_t ram_size;
@@ -56,6 +47,13 @@ struct arm_boot_info {
     const char *initrd_filename;
     const char *dtb_filename;
     hwaddr loader_start;
+    hwaddr dtb_start;
+    hwaddr dtb_limit;
+    /* If set to True, arm_load_kernel() will not load DTB.
+     * It allows board to load DTB manually later.
+     * (default: False)
+     */
+    bool skip_dtb_autoload;
     /* multicore boards that use the default secondary core boot functions
      * need to put the address of the secondary boot code, the boot reg,
      * and the GIC address in the next 3 values, respectively. boards that
@@ -95,7 +93,6 @@ struct arm_boot_info {
      */
     void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
     /* machine init done notifier executing arm_load_dtb */
-    ArmLoadKernelNotifier load_kernel_notifier;
     /* Used internally by arm_boot.c */
     int is_linux;
     hwaddr initrd_start;
@@ -143,6 +140,33 @@ struct arm_boot_info {
  */
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
 
+AddressSpace *arm_boot_address_space(ARMCPU *cpu,
+                                     const struct arm_boot_info *info);
+
+/**
+ * arm_load_dtb() - load a device tree binary image into memory
+ * @addr:       the address to load the image at
+ * @binfo:      struct describing the boot environment
+ * @addr_limit: upper limit of the available memory area at @addr
+ * @as:         address space to load image to
+ *
+ * Load a device tree supplied by the machine or by the user  with the
+ * '-dtb' command line option, and put it at offset @addr in target
+ * memory.
+ *
+ * If @addr_limit contains a meaningful value (i.e., it is strictly greater
+ * than @addr), the device tree is only loaded if its size does not exceed
+ * the limit.
+ *
+ * Returns: the size of the device tree image on success,
+ *          0 if the image size exceeds the limit,
+ *          -1 on errors.
+ *
+ * Note: Must not be called unless have_dtb(binfo) is true.
+ */
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                 hwaddr addr_limit, AddressSpace *as);
+
 /* Write a secure board setup routine with a dummy handler for SMCs */
 void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                             const struct arm_boot_info *info,
diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
index e15bb81..340c382 100644
--- a/include/hw/arm/sysbus-fdt.h
+++ b/include/hw/arm/sysbus-fdt.h
@@ -24,37 +24,14 @@
 #ifndef HW_ARM_SYSBUS_FDT_H
 #define HW_ARM_SYSBUS_FDT_H
 
-#include "hw/arm/arm.h"
-#include "qemu-common.h"
-#include "hw/sysbus.h"
-
-/*
- * struct that contains dimensioning parameters of the platform bus
- */
-typedef struct {
-    hwaddr platform_bus_base; /* start address of the bus */
-    hwaddr platform_bus_size; /* size of the bus */
-    int platform_bus_first_irq; /* first hwirq assigned to the bus */
-    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
-} ARMPlatformBusSystemParams;
-
-/*
- * struct that contains all relevant info to build the fdt nodes of
- * platform bus and attached dynamic sysbus devices
- * in the future might be augmented with additional info
- * such as PHY, CLK handles ...
- */
-typedef struct {
-    const ARMPlatformBusSystemParams *system_params;
-    struct arm_boot_info *binfo;
-    const char *intc; /* parent interrupt controller name */
-} ARMPlatformBusFDTParams;
+#include "exec/hwaddr.h"
 
 /**
- * arm_register_platform_bus_fdt_creator - register a machine init done
- * notifier that creates the device tree nodes of the platform bus and
- * associated dynamic sysbus devices
+ * platform_bus_add_all_fdt_nodes - create all the platform bus nodes
+ *
+ * builds the parent platform bus node and all the nodes of dynamic
+ * sysbus devices attached to it.
  */
-void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params);
-
+void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
+                                    hwaddr bus_size, int irq_start);
 #endif
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9ae6ab2..1f89bc1 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -36,8 +36,8 @@
 #define ARM64_TEXT_OFFSET_OFFSET    8
 #define ARM64_MAGIC_OFFSET          56
 
-static AddressSpace *arm_boot_address_space(ARMCPU *cpu,
-                                            const struct arm_boot_info *info)
+AddressSpace *arm_boot_address_space(ARMCPU *cpu,
+                                     const struct arm_boot_info *info)
 {
     /* Return the address space to use for bootloader reads and writes.
      * We prefer the secure address space if the CPU has it and we're
@@ -486,29 +486,8 @@ static void fdt_add_psci_node(void *fdt)
     qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
 }
 
-/**
- * load_dtb() - load a device tree binary image into memory
- * @addr:       the address to load the image at
- * @binfo:      struct describing the boot environment
- * @addr_limit: upper limit of the available memory area at @addr
- * @as:         address space to load image to
- *
- * Load a device tree supplied by the machine or by the user  with the
- * '-dtb' command line option, and put it at offset @addr in target
- * memory.
- *
- * If @addr_limit contains a meaningful value (i.e., it is strictly greater
- * than @addr), the device tree is only loaded if its size does not exceed
- * the limit.
- *
- * Returns: the size of the device tree image on success,
- *          0 if the image size exceeds the limit,
- *          -1 on errors.
- *
- * Note: Must not be called unless have_dtb(binfo) is true.
- */
-static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-                    hwaddr addr_limit, AddressSpace *as)
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                 hwaddr addr_limit, AddressSpace *as)
 {
     void *fdt = NULL;
     int size, rc;
@@ -935,7 +914,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
     return size;
 }
 
-static void arm_load_kernel_notify(Notifier *notifier, void *data)
+void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
     CPUState *cs;
     int kernel_size;
@@ -945,11 +924,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     int elf_machine;
     hwaddr entry;
     static const ARMInsnFixup *primary_loader;
-    ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
-                                         notifier, notifier);
-    ARMCPU *cpu = n->cpu;
-    struct arm_boot_info *info =
-        container_of(n, struct arm_boot_info, load_kernel_notifier);
     AddressSpace *as = arm_boot_address_space(cpu, info);
 
     /* The board code is not supposed to set secure_board_setup unless
@@ -968,9 +942,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
              * the kernel is supposed to be loaded by the bootloader), copy the
              * DTB to the base of RAM for the bootloader to pick up.
              */
-            if (load_dtb(info->loader_start, info, 0, as) < 0) {
-                exit(1);
-            }
+            info->dtb_start = info->loader_start;
         }
 
         if (info->kernel_filename) {
@@ -1050,15 +1022,14 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
          */
         if (elf_low_addr > info->loader_start
             || elf_high_addr < info->loader_start) {
-            /* Pass elf_low_addr as address limit to load_dtb if it may be
+            /* Set elf_low_addr as address limit for arm_load_dtb if it may be
              * pointing into RAM, otherwise pass '0' (no limit)
              */
             if (elf_low_addr < info->loader_start) {
                 elf_low_addr = 0;
             }
-            if (load_dtb(info->loader_start, info, elf_low_addr, as) < 0) {
-                exit(1);
-            }
+            info->dtb_start = info->loader_start;
+            info->dtb_limit = elf_low_addr;
         }
     }
     entry = elf_entry;
@@ -1116,7 +1087,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
          */
         if (have_dtb(info)) {
             hwaddr align;
-            hwaddr dtb_start;
 
             if (elf_machine == EM_AARCH64) {
                 /*
@@ -1136,11 +1106,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
             }
 
             /* Place the DTB after the initrd in memory with alignment. */
-            dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size, align);
-            if (load_dtb(dtb_start, info, 0, as) < 0) {
-                exit(1);
-            }
-            fixupcontext[FIXUP_ARGPTR] = dtb_start;
+            info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
+                                           align);
+            fixupcontext[FIXUP_ARGPTR] = info->dtb_start;
         } else {
             fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR;
             if (info->ram_size >= (1ULL << 32)) {
@@ -1173,15 +1141,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
         ARM_CPU(cs)->env.boot_info = info;
     }
-}
-
-void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
-{
-    CPUState *cs;
-
-    info->load_kernel_notifier.cpu = cpu;
-    info->load_kernel_notifier.notifier.notify = arm_load_kernel_notify;
-    qemu_add_machine_init_done_notifier(&info->load_kernel_notifier.notifier);
 
     /* CPU objects (unlike devices) are not automatically reset on system
      * reset, so we must always register a handler to do so. If we're
@@ -1191,6 +1150,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
         qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
     }
+
+    if (!info->skip_dtb_autoload) {
+        if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
+            exit(1);
+        }
+    }
 }
 
 static const TypeInfo arm_linux_boot_if_info = {
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 80ff70e..a4dea93 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -49,15 +49,6 @@ typedef struct PlatformBusFDTData {
     PlatformBusDevice *pbus;
 } PlatformBusFDTData;
 
-/*
- * struct used when calling the machine init done notifier
- * that constructs the fdt nodes of platform bus devices
- */
-typedef struct PlatformBusFDTNotifierParams {
-    Notifier notifier;
-    ARMPlatformBusFDTParams *fdt_params;
-} PlatformBusFDTNotifierParams;
-
 /* struct that associates a device type name and a node creation function */
 typedef struct NodeCreationPair {
     const char *typename;
@@ -453,42 +444,17 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
     exit(1);
 }
 
-/**
- * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
- *
- * builds the parent platform bus node and all the nodes of dynamic
- * sysbus devices attached to it.
- */
-static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
+void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
+                                    hwaddr bus_size, int irq_start)
 {
     const char platcomp[] = "qemu,platform\0simple-bus";
-    PlatformBusDevice *pbus;
     DeviceState *dev;
+    PlatformBusDevice *pbus;
     gchar *node;
-    uint64_t addr, size;
-    int irq_start, dtb_size;
-    struct arm_boot_info *info = fdt_params->binfo;
-    const ARMPlatformBusSystemParams *params = fdt_params->system_params;
-    const char *intc = fdt_params->intc;
-    void *fdt = info->get_dtb(info, &dtb_size);
-
-    /*
-     * If the user provided a dtb, we assume the dynamic sysbus nodes
-     * already are integrated there. This corresponds to a use case where
-     * the dynamic sysbus nodes are complex and their generation is not yet
-     * supported. In that case the user can take charge of the guest dt
-     * while qemu takes charge of the qom stuff.
-     */
-    if (info->dtb_filename) {
-        return;
-    }
 
     assert(fdt);
 
-    node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
-    addr = params->platform_bus_base;
-    size = params->platform_bus_size;
-    irq_start = params->platform_bus_first_irq;
+    node = g_strdup_printf("/platform@%"PRIx64, addr);
 
     /* Create a /platform node that we can put all devices into */
     qemu_fdt_add_subnode(fdt, node);
@@ -499,10 +465,11 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
      */
     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_cells(fdt, node, "ranges", 0, addr >> 32, addr, bus_size);
 
     qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
 
+
     dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
     pbus = PLATFORM_BUS_DEVICE(dev);
 
@@ -518,22 +485,3 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
 
     g_free(node);
 }
-
-static void platform_bus_fdt_notify(Notifier *notifier, void *data)
-{
-    PlatformBusFDTNotifierParams *p = DO_UPCAST(PlatformBusFDTNotifierParams,
-                                                notifier, notifier);
-
-    add_all_platform_bus_fdt_nodes(p->fdt_params);
-    g_free(p->fdt_params);
-    g_free(p);
-}
-
-void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params)
-{
-    PlatformBusFDTNotifierParams *p = g_new(PlatformBusFDTNotifierParams, 1);
-
-    p->fdt_params = fdt_params;
-    p->notifier.notify = platform_bus_fdt_notify;
-    qemu_add_machine_init_done_notifier(&p->notifier);
-}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 112c367..1402149 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -93,8 +93,6 @@
 
 #define PLATFORM_BUS_NUM_IRQS 64
 
-static ARMPlatformBusSystemParams platform_bus_params;
-
 /* RAM limit in GB. Since VIRT_MEM starts at the 1GB mark, this means
  * RAM can go up to the 256GB mark, leaving 256GB of the physical
  * address space unallocated and free for future use between 256G and 512G.
@@ -1063,40 +1061,23 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
     DeviceState *dev;
     SysBusDevice *s;
     int i;
-    ARMPlatformBusFDTParams *fdt_params = g_new(ARMPlatformBusFDTParams, 1);
     MemoryRegion *sysmem = get_system_memory();
 
-    platform_bus_params.platform_bus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
-    platform_bus_params.platform_bus_size = vms->memmap[VIRT_PLATFORM_BUS].size;
-    platform_bus_params.platform_bus_first_irq = vms->irqmap[VIRT_PLATFORM_BUS];
-    platform_bus_params.platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS;
-
-    fdt_params->system_params = &platform_bus_params;
-    fdt_params->binfo = &vms->bootinfo;
-    fdt_params->intc = "/intc";
-    /*
-     * register a machine init done notifier that creates the device tree
-     * nodes of the platform bus and its children dynamic sysbus devices
-     */
-    arm_register_platform_bus_fdt_creator(fdt_params);
-
     dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
     dev->id = TYPE_PLATFORM_BUS_DEVICE;
-    qdev_prop_set_uint32(dev, "num_irqs",
-        platform_bus_params.platform_bus_num_irqs);
-    qdev_prop_set_uint32(dev, "mmio_size",
-        platform_bus_params.platform_bus_size);
+    qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
+    qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
     qdev_init_nofail(dev);
     vms->platform_bus_dev = dev;
-    s = SYS_BUS_DEVICE(dev);
 
-    for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
-        int irqn = platform_bus_params.platform_bus_first_irq + i;
+    s = SYS_BUS_DEVICE(dev);
+    for (i = 0; i < PLATFORM_BUS_NUM_IRQS; i++) {
+        int irqn = vms->irqmap[VIRT_PLATFORM_BUS] + i;
         sysbus_connect_irq(s, i, pic[irqn]);
     }
 
     memory_region_add_subregion(sysmem,
-                                platform_bus_params.platform_bus_base,
+                                vms->memmap[VIRT_PLATFORM_BUS].base,
                                 sysbus_mmio_get_region(s, 0));
 }
 
@@ -1167,6 +1148,26 @@ void virt_machine_done(Notifier *notifier, void *data)
 {
     VirtMachineState *vms = container_of(notifier, VirtMachineState,
                                          machine_done);
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+    struct arm_boot_info *info = &vms->bootinfo;
+    AddressSpace *as = arm_boot_address_space(cpu, info);
+
+    /*
+     * If the user provided a dtb, we assume the dynamic sysbus nodes
+     * already are integrated there. This corresponds to a use case where
+     * the dynamic sysbus nodes are complex and their generation is not yet
+     * supported. In that case the user can take charge of the guest dt
+     * while qemu takes charge of the qom stuff.
+     */
+    if (info->dtb_filename == NULL) {
+        platform_bus_add_all_fdt_nodes(vms->fdt, "/intc",
+                                       vms->memmap[VIRT_PLATFORM_BUS].base,
+                                       vms->memmap[VIRT_PLATFORM_BUS].size,
+                                       vms->irqmap[VIRT_PLATFORM_BUS]);
+    }
+    if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
+        exit(1);
+    }
 
     virt_acpi_setup(vms);
     virt_build_smbios(vms);
@@ -1394,8 +1395,7 @@ static void machvirt_init(MachineState *machine)
     vms->fw_cfg = create_fw_cfg(vms, &address_space_memory);
     rom_set_fw(vms->fw_cfg);
 
-    vms->machine_done.notify = virt_machine_done;
-    qemu_add_machine_init_done_notifier(&vms->machine_done);
+    create_platform_bus(vms, pic);
 
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.kernel_filename = machine->kernel_filename;
@@ -1405,16 +1405,12 @@ static void machvirt_init(MachineState *machine)
     vms->bootinfo.board_id = -1;
     vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
     vms->bootinfo.get_dtb = machvirt_dtb;
+    vms->bootinfo.skip_dtb_autoload = true;
     vms->bootinfo.firmware_loaded = firmware_loaded;
     arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo);
 
-    /*
-     * arm_load_kernel machine init done notifier registration must
-     * happen before the platform_bus_create call. In this latter,
-     * another notifier is registered which adds platform bus nodes.
-     * Notifiers are executed in registration reverse order.
-     */
-    create_platform_bus(vms, pic);
+    vms->machine_done.notify = virt_machine_done;
+    qemu_add_machine_init_done_notifier(&vms->machine_done);
 }
 
 static bool virt_get_secure(Object *obj, Error **errp)
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.13 v2 5/5] arm/boot: split load_dtb() from arm_load_kernel()

[Andrew Jones]

On Wed, Apr 18, 2018 at 04:28:05PM +0200, Igor Mammedov wrote:
> load_dtb() depends on arm_load_kernel() to figure out place
> in RAM where it should be loaded, but it's not required for
> arm_load_kernel() to work. Sometimes it's neccesary for
> devices added with -device/device_add to be enumerated in
> DTB as well, which's lead to [1] and surrounding commits to
> add 2 more machine_done notifiers with non obvious ordering
> to make dynamic sysbus devices initialization happen in
> the right order.
> 
> However instead of moving whole arm_load_kernel() in to
> machine_done, it's sufficient to move only load_dtb() into
> virt_machine_done() notifier and remove ArmLoadKernelNotifier/
> /PlatformBusFDTNotifierParams notifiers, which saves us ~90LOC
> and simplifies code flow quite a bit.
> Later would allow to consolidate DTB generation within one
> function for 'mach-virt' board and make it reentrant so it
> could generate updated DTB in device hotplug secenarios.
> 
> While at it rename load_dtb() to arm_load_dtb() since it's
> public now.
> 
> Add additional field skip_dtb_autoload to struct arm_boot_info
> to allow manual DTB load later in mach-virt and to avoid touching
> all other boards to explicitly call arm_load_dtb().
> 
>  1) (ac9d32e hw/arm/boot: arm_load_kernel implemented as a machine init done notifier)
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - fix rebase conflicts due to dropped
>         [PATCH for-2.13 1/4] arm: reuse  arm_boot_address_space() in armv7m_load_kernel()
>   - add doc comment to new skip_dtb_autoload field
> ---
>  include/hw/arm/arm.h        | 44 +++++++++++++++++++++-------
>  include/hw/arm/sysbus-fdt.h | 37 +++++------------------
>  hw/arm/boot.c               | 71 ++++++++++++---------------------------------
>  hw/arm/sysbus-fdt.c         | 64 ++++------------------------------------
>  hw/arm/virt.c               | 64 +++++++++++++++++++---------------------
>  5 files changed, 95 insertions(+), 185 deletions(-)
> 
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index ce769bd..7039956 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -39,15 +39,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>   */
>  void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size);
>  
> -/*
> - * struct used as a parameter of the arm_load_kernel machine init
> - * done notifier
> - */
> -typedef struct {
> -    Notifier notifier; /* actual notifier */
> -    ARMCPU *cpu; /* handle to the first cpu object */
> -} ArmLoadKernelNotifier;
> -
>  /* arm_boot.c */
>  struct arm_boot_info {
>      uint64_t ram_size;
> @@ -56,6 +47,13 @@ struct arm_boot_info {
>      const char *initrd_filename;
>      const char *dtb_filename;
>      hwaddr loader_start;
> +    hwaddr dtb_start;
> +    hwaddr dtb_limit;
> +    /* If set to True, arm_load_kernel() will not load DTB.
> +     * It allows board to load DTB manually later.
> +     * (default: False)
> +     */
> +    bool skip_dtb_autoload;
>      /* multicore boards that use the default secondary core boot functions
>       * need to put the address of the secondary boot code, the boot reg,
>       * and the GIC address in the next 3 values, respectively. boards that
> @@ -95,7 +93,6 @@ struct arm_boot_info {
>       */
>      void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
>      /* machine init done notifier executing arm_load_dtb */

Need to remove the above now stale comment too.

> -    ArmLoadKernelNotifier load_kernel_notifier;
>      /* Used internally by arm_boot.c */
>      int is_linux;
>      hwaddr initrd_start;
> @@ -143,6 +140,33 @@ struct arm_boot_info {
>   */
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
>  
> +AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> +                                     const struct arm_boot_info *info);
> +
> +/**
> + * arm_load_dtb() - load a device tree binary image into memory
> + * @addr:       the address to load the image at
> + * @binfo:      struct describing the boot environment
> + * @addr_limit: upper limit of the available memory area at @addr
> + * @as:         address space to load image to
> + *
> + * Load a device tree supplied by the machine or by the user  with the
> + * '-dtb' command line option, and put it at offset @addr in target
> + * memory.
> + *
> + * If @addr_limit contains a meaningful value (i.e., it is strictly greater
> + * than @addr), the device tree is only loaded if its size does not exceed
> + * the limit.
> + *
> + * Returns: the size of the device tree image on success,
> + *          0 if the image size exceeds the limit,
> + *          -1 on errors.
> + *
> + * Note: Must not be called unless have_dtb(binfo) is true.
> + */
> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                 hwaddr addr_limit, AddressSpace *as);
> +
>  /* Write a secure board setup routine with a dummy handler for SMCs */
>  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
>                                              const struct arm_boot_info *info,
> diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
> index e15bb81..340c382 100644
> --- a/include/hw/arm/sysbus-fdt.h
> +++ b/include/hw/arm/sysbus-fdt.h
> @@ -24,37 +24,14 @@
>  #ifndef HW_ARM_SYSBUS_FDT_H
>  #define HW_ARM_SYSBUS_FDT_H
>  
> -#include "hw/arm/arm.h"
> -#include "qemu-common.h"
> -#include "hw/sysbus.h"
> -
> -/*
> - * struct that contains dimensioning parameters of the platform bus
> - */
> -typedef struct {
> -    hwaddr platform_bus_base; /* start address of the bus */
> -    hwaddr platform_bus_size; /* size of the bus */
> -    int platform_bus_first_irq; /* first hwirq assigned to the bus */
> -    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
> -} ARMPlatformBusSystemParams;
> -
> -/*
> - * struct that contains all relevant info to build the fdt nodes of
> - * platform bus and attached dynamic sysbus devices
> - * in the future might be augmented with additional info
> - * such as PHY, CLK handles ...
> - */
> -typedef struct {
> -    const ARMPlatformBusSystemParams *system_params;
> -    struct arm_boot_info *binfo;
> -    const char *intc; /* parent interrupt controller name */
> -} ARMPlatformBusFDTParams;
> +#include "exec/hwaddr.h"
>  
>  /**
> - * arm_register_platform_bus_fdt_creator - register a machine init done
> - * notifier that creates the device tree nodes of the platform bus and
> - * associated dynamic sysbus devices
> + * platform_bus_add_all_fdt_nodes - create all the platform bus nodes
> + *
> + * builds the parent platform bus node and all the nodes of dynamic
> + * sysbus devices attached to it.
>   */
> -void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params);
> -
> +void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
> +                                    hwaddr bus_size, int irq_start);
>  #endif
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 9ae6ab2..1f89bc1 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -36,8 +36,8 @@
>  #define ARM64_TEXT_OFFSET_OFFSET    8
>  #define ARM64_MAGIC_OFFSET          56
>  
> -static AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> -                                            const struct arm_boot_info *info)
> +AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> +                                     const struct arm_boot_info *info)
>  {
>      /* Return the address space to use for bootloader reads and writes.
>       * We prefer the secure address space if the CPU has it and we're
> @@ -486,29 +486,8 @@ static void fdt_add_psci_node(void *fdt)
>      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
>  }
>  
> -/**
> - * load_dtb() - load a device tree binary image into memory
> - * @addr:       the address to load the image at
> - * @binfo:      struct describing the boot environment
> - * @addr_limit: upper limit of the available memory area at @addr
> - * @as:         address space to load image to
> - *
> - * Load a device tree supplied by the machine or by the user  with the
> - * '-dtb' command line option, and put it at offset @addr in target
> - * memory.
> - *
> - * If @addr_limit contains a meaningful value (i.e., it is strictly greater
> - * than @addr), the device tree is only loaded if its size does not exceed
> - * the limit.
> - *
> - * Returns: the size of the device tree image on success,
> - *          0 if the image size exceeds the limit,
> - *          -1 on errors.
> - *
> - * Note: Must not be called unless have_dtb(binfo) is true.
> - */
> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> -                    hwaddr addr_limit, AddressSpace *as)
> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                 hwaddr addr_limit, AddressSpace *as)
>  {
>      void *fdt = NULL;
>      int size, rc;
> @@ -935,7 +914,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>      return size;
>  }
>  
> -static void arm_load_kernel_notify(Notifier *notifier, void *data)
> +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
>      CPUState *cs;
>      int kernel_size;
> @@ -945,11 +924,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> -    ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
> -                                         notifier, notifier);
> -    ARMCPU *cpu = n->cpu;
> -    struct arm_boot_info *info =
> -        container_of(n, struct arm_boot_info, load_kernel_notifier);
>      AddressSpace *as = arm_boot_address_space(cpu, info);
>  
>      /* The board code is not supposed to set secure_board_setup unless
> @@ -968,9 +942,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>               * the kernel is supposed to be loaded by the bootloader), copy the
>               * DTB to the base of RAM for the bootloader to pick up.
>               */
> -            if (load_dtb(info->loader_start, info, 0, as) < 0) {
> -                exit(1);
> -            }
> +            info->dtb_start = info->loader_start;

Would be nice to explicitly set dtb_limit = 0 here. Or assert that it's
already zero if the expectation that it's already zero should never be
wrong.

>          }
>  
>          if (info->kernel_filename) {
> @@ -1050,15 +1022,14 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>           */
>          if (elf_low_addr > info->loader_start
>              || elf_high_addr < info->loader_start) {
> -            /* Pass elf_low_addr as address limit to load_dtb if it may be
> +            /* Set elf_low_addr as address limit for arm_load_dtb if it may be
>               * pointing into RAM, otherwise pass '0' (no limit)
>               */
>              if (elf_low_addr < info->loader_start) {
>                  elf_low_addr = 0;
>              }
> -            if (load_dtb(info->loader_start, info, elf_low_addr, as) < 0) {
> -                exit(1);
> -            }
> +            info->dtb_start = info->loader_start;
> +            info->dtb_limit = elf_low_addr;
>          }
>      }
>      entry = elf_entry;
> @@ -1116,7 +1087,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>           */
>          if (have_dtb(info)) {
>              hwaddr align;
> -            hwaddr dtb_start;
>  
>              if (elf_machine == EM_AARCH64) {
>                  /*
> @@ -1136,11 +1106,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>              }
>  
>              /* Place the DTB after the initrd in memory with alignment. */
> -            dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size, align);
> -            if (load_dtb(dtb_start, info, 0, as) < 0) {
> -                exit(1);
> -            }
> -            fixupcontext[FIXUP_ARGPTR] = dtb_start;
> +            info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
> +                                           align);

Again dtd_limit = 0. Could maybe just set it / assert it at the entry of
the function.

> +            fixupcontext[FIXUP_ARGPTR] = info->dtb_start;
>          } else {
>              fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR;
>              if (info->ram_size >= (1ULL << 32)) {
> @@ -1173,15 +1141,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
>          ARM_CPU(cs)->env.boot_info = info;
>      }

I wonder why we need to start at cpu here, but first_cpu below. If
they could both be first_cpu, then we could merge the loop statements
into one loop. Reading enough code to build confidence that it could
be first_cpu is too much to ask for a Friday afternoon though...

> -}
> -
> -void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> -{
> -    CPUState *cs;
> -
> -    info->load_kernel_notifier.cpu = cpu;
> -    info->load_kernel_notifier.notifier.notify = arm_load_kernel_notify;
> -    qemu_add_machine_init_done_notifier(&info->load_kernel_notifier.notifier);
>  
>      /* CPU objects (unlike devices) are not automatically reset on system
>       * reset, so we must always register a handler to do so. If we're
> @@ -1191,6 +1150,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
>          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>      }
> +
> +    if (!info->skip_dtb_autoload) {
> +        if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
> +            exit(1);
> +        }
> +    }
>  }
>  
>  static const TypeInfo arm_linux_boot_if_info = {
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 80ff70e..a4dea93 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -49,15 +49,6 @@ typedef struct PlatformBusFDTData {
>      PlatformBusDevice *pbus;
>  } PlatformBusFDTData;
>  
> -/*
> - * struct used when calling the machine init done notifier
> - * that constructs the fdt nodes of platform bus devices
> - */
> -typedef struct PlatformBusFDTNotifierParams {
> -    Notifier notifier;
> -    ARMPlatformBusFDTParams *fdt_params;
> -} PlatformBusFDTNotifierParams;
> -
>  /* struct that associates a device type name and a node creation function */
>  typedef struct NodeCreationPair {
>      const char *typename;
> @@ -453,42 +444,17 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
>      exit(1);
>  }
>  
> -/**
> - * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
> - *
> - * builds the parent platform bus node and all the nodes of dynamic
> - * sysbus devices attached to it.
> - */
> -static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> +void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
> +                                    hwaddr bus_size, int irq_start)
>  {
>      const char platcomp[] = "qemu,platform\0simple-bus";
> -    PlatformBusDevice *pbus;
>      DeviceState *dev;
> +    PlatformBusDevice *pbus;

Unnecessary change, but whatever

>      gchar *node;
> -    uint64_t addr, size;
> -    int irq_start, dtb_size;
> -    struct arm_boot_info *info = fdt_params->binfo;
> -    const ARMPlatformBusSystemParams *params = fdt_params->system_params;
> -    const char *intc = fdt_params->intc;
> -    void *fdt = info->get_dtb(info, &dtb_size);
> -
> -    /*
> -     * If the user provided a dtb, we assume the dynamic sysbus nodes
> -     * already are integrated there. This corresponds to a use case where
> -     * the dynamic sysbus nodes are complex and their generation is not yet
> -     * supported. In that case the user can take charge of the guest dt
> -     * while qemu takes charge of the qom stuff.
> -     */
> -    if (info->dtb_filename) {
> -        return;
> -    }
>  
>      assert(fdt);
>  
> -    node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
> -    addr = params->platform_bus_base;
> -    size = params->platform_bus_size;
> -    irq_start = params->platform_bus_first_irq;
> +    node = g_strdup_printf("/platform@%"PRIx64, addr);
>  
>      /* Create a /platform node that we can put all devices into */
>      qemu_fdt_add_subnode(fdt, node);
> @@ -499,10 +465,11 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
>       */
>      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_cells(fdt, node, "ranges", 0, addr >> 32, addr, bus_size);
>  
>      qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
>  
> +

Extra blank line added here

>      dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
>      pbus = PLATFORM_BUS_DEVICE(dev);
>  
> @@ -518,22 +485,3 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
>  
>      g_free(node);
>  }
> -
> -static void platform_bus_fdt_notify(Notifier *notifier, void *data)
> -{
> -    PlatformBusFDTNotifierParams *p = DO_UPCAST(PlatformBusFDTNotifierParams,
> -                                                notifier, notifier);
> -
> -    add_all_platform_bus_fdt_nodes(p->fdt_params);
> -    g_free(p->fdt_params);
> -    g_free(p);
> -}
> -
> -void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params)
> -{
> -    PlatformBusFDTNotifierParams *p = g_new(PlatformBusFDTNotifierParams, 1);
> -
> -    p->fdt_params = fdt_params;
> -    p->notifier.notify = platform_bus_fdt_notify;
> -    qemu_add_machine_init_done_notifier(&p->notifier);
> -}
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 112c367..1402149 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -93,8 +93,6 @@
>  
>  #define PLATFORM_BUS_NUM_IRQS 64
>  
> -static ARMPlatformBusSystemParams platform_bus_params;
> -
>  /* RAM limit in GB. Since VIRT_MEM starts at the 1GB mark, this means
>   * RAM can go up to the 256GB mark, leaving 256GB of the physical
>   * address space unallocated and free for future use between 256G and 512G.
> @@ -1063,40 +1061,23 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
>      DeviceState *dev;
>      SysBusDevice *s;
>      int i;
> -    ARMPlatformBusFDTParams *fdt_params = g_new(ARMPlatformBusFDTParams, 1);
>      MemoryRegion *sysmem = get_system_memory();
>  
> -    platform_bus_params.platform_bus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> -    platform_bus_params.platform_bus_size = vms->memmap[VIRT_PLATFORM_BUS].size;
> -    platform_bus_params.platform_bus_first_irq = vms->irqmap[VIRT_PLATFORM_BUS];
> -    platform_bus_params.platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS;
> -
> -    fdt_params->system_params = &platform_bus_params;
> -    fdt_params->binfo = &vms->bootinfo;
> -    fdt_params->intc = "/intc";
> -    /*
> -     * register a machine init done notifier that creates the device tree
> -     * nodes of the platform bus and its children dynamic sysbus devices
> -     */
> -    arm_register_platform_bus_fdt_creator(fdt_params);
> -
>      dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
>      dev->id = TYPE_PLATFORM_BUS_DEVICE;
> -    qdev_prop_set_uint32(dev, "num_irqs",
> -        platform_bus_params.platform_bus_num_irqs);
> -    qdev_prop_set_uint32(dev, "mmio_size",
> -        platform_bus_params.platform_bus_size);
> +    qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> +    qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
>      qdev_init_nofail(dev);
>      vms->platform_bus_dev = dev;
> -    s = SYS_BUS_DEVICE(dev);
>  
> -    for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
> -        int irqn = platform_bus_params.platform_bus_first_irq + i;
> +    s = SYS_BUS_DEVICE(dev);
> +    for (i = 0; i < PLATFORM_BUS_NUM_IRQS; i++) {
> +        int irqn = vms->irqmap[VIRT_PLATFORM_BUS] + i;
>          sysbus_connect_irq(s, i, pic[irqn]);
>      }
>  
>      memory_region_add_subregion(sysmem,
> -                                platform_bus_params.platform_bus_base,
> +                                vms->memmap[VIRT_PLATFORM_BUS].base,
>                                  sysbus_mmio_get_region(s, 0));
>  }
>  
> @@ -1167,6 +1148,26 @@ void virt_machine_done(Notifier *notifier, void *data)
>  {
>      VirtMachineState *vms = container_of(notifier, VirtMachineState,
>                                           machine_done);
> +    ARMCPU *cpu = ARM_CPU(first_cpu);
> +    struct arm_boot_info *info = &vms->bootinfo;
> +    AddressSpace *as = arm_boot_address_space(cpu, info);
> +
> +    /*
> +     * If the user provided a dtb, we assume the dynamic sysbus nodes
> +     * already are integrated there. This corresponds to a use case where
> +     * the dynamic sysbus nodes are complex and their generation is not yet
> +     * supported. In that case the user can take charge of the guest dt
> +     * while qemu takes charge of the qom stuff.
> +     */
> +    if (info->dtb_filename == NULL) {
> +        platform_bus_add_all_fdt_nodes(vms->fdt, "/intc",
> +                                       vms->memmap[VIRT_PLATFORM_BUS].base,
> +                                       vms->memmap[VIRT_PLATFORM_BUS].size,
> +                                       vms->irqmap[VIRT_PLATFORM_BUS]);
> +    }
> +    if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {

I think we can change arm_load_dtb() to just take cpu and info. Then we
don't need to make arm_boot_address_space() global, as we'd just call
it from arm_load_dtb(). Actually even 'cpu' is debatable, because it
should probably always be first_cpu, so we could just use that in
arm_load_dtb() as well.

> +        exit(1);
> +    }
>  
>      virt_acpi_setup(vms);
>      virt_build_smbios(vms);
> @@ -1394,8 +1395,7 @@ static void machvirt_init(MachineState *machine)
>      vms->fw_cfg = create_fw_cfg(vms, &address_space_memory);
>      rom_set_fw(vms->fw_cfg);
>  
> -    vms->machine_done.notify = virt_machine_done;
> -    qemu_add_machine_init_done_notifier(&vms->machine_done);
> +    create_platform_bus(vms, pic);
>  
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.kernel_filename = machine->kernel_filename;
> @@ -1405,16 +1405,12 @@ static void machvirt_init(MachineState *machine)
>      vms->bootinfo.board_id = -1;
>      vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
>      vms->bootinfo.get_dtb = machvirt_dtb;
> +    vms->bootinfo.skip_dtb_autoload = true;
>      vms->bootinfo.firmware_loaded = firmware_loaded;
>      arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo);
>  
> -    /*
> -     * arm_load_kernel machine init done notifier registration must
> -     * happen before the platform_bus_create call. In this latter,
> -     * another notifier is registered which adds platform bus nodes.
> -     * Notifiers are executed in registration reverse order.
> -     */
> -    create_platform_bus(vms, pic);
> +    vms->machine_done.notify = virt_machine_done;
> +    qemu_add_machine_init_done_notifier(&vms->machine_done);
>  }
>  
>  static bool virt_get_secure(Object *obj, Error **errp)
> -- 
> 2.7.4
> 
>

Nice cleanup, particularly regarding the platform bus fdt node parameter
passing. The review would be a bit easier if we did the conversion without
deletion in one patch and deletion in a second patch, but then compiling
would complain about unused code and with warnings treated as errors that
would break bisection, so I guess the reviewers just have to work harder.

Besides the nits and ensuring dtb_limit=0 when it should be,

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH for-2.13 v2 5/5] arm/boot: split load_dtb() from arm_load_kernel()

[Peter Maydell]

On 27 April 2018 at 14:47, Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Apr 18, 2018 at 04:28:05PM +0200, Igor Mammedov wrote:
>> @@ -1173,15 +1141,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>      for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
>>          ARM_CPU(cs)->env.boot_info = info;
>>      }
>
> I wonder why we need to start at cpu here, but first_cpu below. If
> they could both be first_cpu, then we could merge the loop statements
> into one loop. Reading enough code to build confidence that it could
> be first_cpu is too much to ask for a Friday afternoon though...

It should be starting at first_cpu -- starting with 'cpu' is
a bug. However as with the bug fixed in 75ed2c02484101d5, it
isn't currently causing any incorrect behaviour, because every
board we have is passing first_cpu as the boot cpu, either directly
or indirectly.

There is a theoretical use case for only feeding the boot_info
to a subset of CPUs, which is where you have a setup like
the xilinx zynqmp which has 4x A-class cores which run Linux
and 4x R-class cores which run something else; in that setup
you might want to say "the boot_info stuff we have here is
just for the A-class cluster, and the R-class cores should
look after themselves". However, (a) we don't really properly
support heterogenous setups like that -- zynqmp works by
accident rather than design -- and (b) if we do want to
support them we need a sensible API for indicating which
CPUs should or should not be involved in -kernel boot as
primary or secondaries.

So we should fix this loop to start at first_cpu, and worry
about the heterogenous setup usecase if and when it becomes
reality rather than theory.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.13 v2 5/5] arm/boot: split load_dtb() from arm_load_kernel()

[Igor Mammedov]

On Fri, 27 Apr 2018 15:47:28 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Wed, Apr 18, 2018 at 04:28:05PM +0200, Igor Mammedov wrote:
> > load_dtb() depends on arm_load_kernel() to figure out place
> > in RAM where it should be loaded, but it's not required for
> > arm_load_kernel() to work. Sometimes it's neccesary for
> > devices added with -device/device_add to be enumerated in
> > DTB as well, which's lead to [1] and surrounding commits to
> > add 2 more machine_done notifiers with non obvious ordering
> > to make dynamic sysbus devices initialization happen in
> > the right order.
> > 
> > However instead of moving whole arm_load_kernel() in to
> > machine_done, it's sufficient to move only load_dtb() into
> > virt_machine_done() notifier and remove ArmLoadKernelNotifier/
> > /PlatformBusFDTNotifierParams notifiers, which saves us ~90LOC
> > and simplifies code flow quite a bit.
> > Later would allow to consolidate DTB generation within one
> > function for 'mach-virt' board and make it reentrant so it
> > could generate updated DTB in device hotplug secenarios.
> > 
> > While at it rename load_dtb() to arm_load_dtb() since it's
> > public now.
> > 
> > Add additional field skip_dtb_autoload to struct arm_boot_info
> > to allow manual DTB load later in mach-virt and to avoid touching
> > all other boards to explicitly call arm_load_dtb().
> > 
> >  1) (ac9d32e hw/arm/boot: arm_load_kernel implemented as a machine init done notifier)
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   - fix rebase conflicts due to dropped
> >         [PATCH for-2.13 1/4] arm: reuse  arm_boot_address_space() in armv7m_load_kernel()
> >   - add doc comment to new skip_dtb_autoload field
> > ---
> >  include/hw/arm/arm.h        | 44 +++++++++++++++++++++-------
> >  include/hw/arm/sysbus-fdt.h | 37 +++++------------------
> >  hw/arm/boot.c               | 71 ++++++++++++---------------------------------
> >  hw/arm/sysbus-fdt.c         | 64 ++++------------------------------------
> >  hw/arm/virt.c               | 64 +++++++++++++++++++---------------------
> >  5 files changed, 95 insertions(+), 185 deletions(-)
> > 
> > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> > index ce769bd..7039956 100644
> > --- a/include/hw/arm/arm.h
> > +++ b/include/hw/arm/arm.h
> > @@ -39,15 +39,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> >   */
> >  void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size);
> >  
> > -/*
> > - * struct used as a parameter of the arm_load_kernel machine init
> > - * done notifier
> > - */
> > -typedef struct {
> > -    Notifier notifier; /* actual notifier */
> > -    ARMCPU *cpu; /* handle to the first cpu object */
> > -} ArmLoadKernelNotifier;
> > -
> >  /* arm_boot.c */
> >  struct arm_boot_info {
> >      uint64_t ram_size;
> > @@ -56,6 +47,13 @@ struct arm_boot_info {
> >      const char *initrd_filename;
> >      const char *dtb_filename;
> >      hwaddr loader_start;
> > +    hwaddr dtb_start;
> > +    hwaddr dtb_limit;
> > +    /* If set to True, arm_load_kernel() will not load DTB.
> > +     * It allows board to load DTB manually later.
> > +     * (default: False)
> > +     */
> > +    bool skip_dtb_autoload;
> >      /* multicore boards that use the default secondary core boot functions
> >       * need to put the address of the secondary boot code, the boot reg,
> >       * and the GIC address in the next 3 values, respectively. boards that
> > @@ -95,7 +93,6 @@ struct arm_boot_info {
> >       */
> >      void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
> >      /* machine init done notifier executing arm_load_dtb */  
> 
> Need to remove the above now stale comment too.
Fixed

> 
> > -    ArmLoadKernelNotifier load_kernel_notifier;
> >      /* Used internally by arm_boot.c */
> >      int is_linux;
> >      hwaddr initrd_start;
> > @@ -143,6 +140,33 @@ struct arm_boot_info {
> >   */
> >  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
> >  
> > +AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> > +                                     const struct arm_boot_info *info);
> > +
> > +/**
> > + * arm_load_dtb() - load a device tree binary image into memory
> > + * @addr:       the address to load the image at
> > + * @binfo:      struct describing the boot environment
> > + * @addr_limit: upper limit of the available memory area at @addr
> > + * @as:         address space to load image to
> > + *
> > + * Load a device tree supplied by the machine or by the user  with the
> > + * '-dtb' command line option, and put it at offset @addr in target
> > + * memory.
> > + *
> > + * If @addr_limit contains a meaningful value (i.e., it is strictly greater
> > + * than @addr), the device tree is only loaded if its size does not exceed
> > + * the limit.
> > + *
> > + * Returns: the size of the device tree image on success,
> > + *          0 if the image size exceeds the limit,
> > + *          -1 on errors.
> > + *
> > + * Note: Must not be called unless have_dtb(binfo) is true.
> > + */
> > +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> > +                 hwaddr addr_limit, AddressSpace *as);
> > +
> >  /* Write a secure board setup routine with a dummy handler for SMCs */
> >  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
> >                                              const struct arm_boot_info *info,
> > diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
> > index e15bb81..340c382 100644
> > --- a/include/hw/arm/sysbus-fdt.h
> > +++ b/include/hw/arm/sysbus-fdt.h
> > @@ -24,37 +24,14 @@
> >  #ifndef HW_ARM_SYSBUS_FDT_H
> >  #define HW_ARM_SYSBUS_FDT_H
> >  
> > -#include "hw/arm/arm.h"
> > -#include "qemu-common.h"
> > -#include "hw/sysbus.h"
> > -
> > -/*
> > - * struct that contains dimensioning parameters of the platform bus
> > - */
> > -typedef struct {
> > -    hwaddr platform_bus_base; /* start address of the bus */
> > -    hwaddr platform_bus_size; /* size of the bus */
> > -    int platform_bus_first_irq; /* first hwirq assigned to the bus */
> > -    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
> > -} ARMPlatformBusSystemParams;
> > -
> > -/*
> > - * struct that contains all relevant info to build the fdt nodes of
> > - * platform bus and attached dynamic sysbus devices
> > - * in the future might be augmented with additional info
> > - * such as PHY, CLK handles ...
> > - */
> > -typedef struct {
> > -    const ARMPlatformBusSystemParams *system_params;
> > -    struct arm_boot_info *binfo;
> > -    const char *intc; /* parent interrupt controller name */
> > -} ARMPlatformBusFDTParams;
> > +#include "exec/hwaddr.h"
> >  
> >  /**
> > - * arm_register_platform_bus_fdt_creator - register a machine init done
> > - * notifier that creates the device tree nodes of the platform bus and
> > - * associated dynamic sysbus devices
> > + * platform_bus_add_all_fdt_nodes - create all the platform bus nodes
> > + *
> > + * builds the parent platform bus node and all the nodes of dynamic
> > + * sysbus devices attached to it.
> >   */
> > -void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params);
> > -
> > +void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
> > +                                    hwaddr bus_size, int irq_start);
> >  #endif
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 9ae6ab2..1f89bc1 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -36,8 +36,8 @@
> >  #define ARM64_TEXT_OFFSET_OFFSET    8
> >  #define ARM64_MAGIC_OFFSET          56
> >  
> > -static AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> > -                                            const struct arm_boot_info *info)
> > +AddressSpace *arm_boot_address_space(ARMCPU *cpu,
> > +                                     const struct arm_boot_info *info)
> >  {
> >      /* Return the address space to use for bootloader reads and writes.
> >       * We prefer the secure address space if the CPU has it and we're
> > @@ -486,29 +486,8 @@ static void fdt_add_psci_node(void *fdt)
> >      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
> >  }
> >  
> > -/**
> > - * load_dtb() - load a device tree binary image into memory
> > - * @addr:       the address to load the image at
> > - * @binfo:      struct describing the boot environment
> > - * @addr_limit: upper limit of the available memory area at @addr
> > - * @as:         address space to load image to
> > - *
> > - * Load a device tree supplied by the machine or by the user  with the
> > - * '-dtb' command line option, and put it at offset @addr in target
> > - * memory.
> > - *
> > - * If @addr_limit contains a meaningful value (i.e., it is strictly greater
> > - * than @addr), the device tree is only loaded if its size does not exceed
> > - * the limit.
> > - *
> > - * Returns: the size of the device tree image on success,
> > - *          0 if the image size exceeds the limit,
> > - *          -1 on errors.
> > - *
> > - * Note: Must not be called unless have_dtb(binfo) is true.
> > - */
> > -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> > -                    hwaddr addr_limit, AddressSpace *as)
> > +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> > +                 hwaddr addr_limit, AddressSpace *as)
> >  {
> >      void *fdt = NULL;
> >      int size, rc;
> > @@ -935,7 +914,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
> >      return size;
> >  }
> >  
> > -static void arm_load_kernel_notify(Notifier *notifier, void *data)
> > +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> >  {
> >      CPUState *cs;
> >      int kernel_size;
> > @@ -945,11 +924,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >      int elf_machine;
> >      hwaddr entry;
> >      static const ARMInsnFixup *primary_loader;
> > -    ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
> > -                                         notifier, notifier);
> > -    ARMCPU *cpu = n->cpu;
> > -    struct arm_boot_info *info =
> > -        container_of(n, struct arm_boot_info, load_kernel_notifier);
> >      AddressSpace *as = arm_boot_address_space(cpu, info);
> >  
> >      /* The board code is not supposed to set secure_board_setup unless
> > @@ -968,9 +942,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >               * the kernel is supposed to be loaded by the bootloader), copy the
> >               * DTB to the base of RAM for the bootloader to pick up.
> >               */
> > -            if (load_dtb(info->loader_start, info, 0, as) < 0) {
> > -                exit(1);
> > -            }
> > +            info->dtb_start = info->loader_start;  
> 
> Would be nice to explicitly set dtb_limit = 0 here. Or assert that it's
> already zero if the expectation that it's already zero should never be
> wrong.
Fixed

> >          }
> >  
> >          if (info->kernel_filename) {
> > @@ -1050,15 +1022,14 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >           */
> >          if (elf_low_addr > info->loader_start
> >              || elf_high_addr < info->loader_start) {
> > -            /* Pass elf_low_addr as address limit to load_dtb if it may be
> > +            /* Set elf_low_addr as address limit for arm_load_dtb if it may be
> >               * pointing into RAM, otherwise pass '0' (no limit)
> >               */
> >              if (elf_low_addr < info->loader_start) {
> >                  elf_low_addr = 0;
> >              }
> > -            if (load_dtb(info->loader_start, info, elf_low_addr, as) < 0) {
> > -                exit(1);
> > -            }
> > +            info->dtb_start = info->loader_start;
> > +            info->dtb_limit = elf_low_addr;
> >          }
> >      }
> >      entry = elf_entry;
> > @@ -1116,7 +1087,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >           */
> >          if (have_dtb(info)) {
> >              hwaddr align;
> > -            hwaddr dtb_start;
> >  
> >              if (elf_machine == EM_AARCH64) {
> >                  /*
> > @@ -1136,11 +1106,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >              }
> >  
> >              /* Place the DTB after the initrd in memory with alignment. */
> > -            dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size, align);
> > -            if (load_dtb(dtb_start, info, 0, as) < 0) {
> > -                exit(1);
> > -            }
> > -            fixupcontext[FIXUP_ARGPTR] = dtb_start;
> > +            info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
> > +                                           align);  
> 
> Again dtd_limit = 0. Could maybe just set it / assert it at the entry of
> the function.
moved, 0-initialization at the start of function as suggested

> 
> > +            fixupcontext[FIXUP_ARGPTR] = info->dtb_start;
> >          } else {
> >              fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR;
> >              if (info->ram_size >= (1ULL << 32)) {
> > @@ -1173,15 +1141,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >      for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> >          ARM_CPU(cs)->env.boot_info = info;
> >      }  
> 
> I wonder why we need to start at cpu here, but first_cpu below. If
> they could both be first_cpu, then we could merge the loop statements
> into one loop. Reading enough code to build confidence that it could
> be first_cpu is too much to ask for a Friday afternoon though...
Considering Peter is in favor of using  first_cpu here as well,
I'll add extra patch on top to do that

> 
> > -}
> > -
> > -void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> > -{
> > -    CPUState *cs;
> > -
> > -    info->load_kernel_notifier.cpu = cpu;
> > -    info->load_kernel_notifier.notifier.notify = arm_load_kernel_notify;
> > -    qemu_add_machine_init_done_notifier(&info->load_kernel_notifier.notifier);
> >  
> >      /* CPU objects (unlike devices) are not automatically reset on system
> >       * reset, so we must always register a handler to do so. If we're
> > @@ -1191,6 +1150,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> >      for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> >          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> >      }
> > +
> > +    if (!info->skip_dtb_autoload) {
> > +        if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
> > +            exit(1);
> > +        }
> > +    }
> >  }
> >  
> >  static const TypeInfo arm_linux_boot_if_info = {
> > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> > index 80ff70e..a4dea93 100644
> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > @@ -49,15 +49,6 @@ typedef struct PlatformBusFDTData {
> >      PlatformBusDevice *pbus;
> >  } PlatformBusFDTData;
> >  
> > -/*
> > - * struct used when calling the machine init done notifier
> > - * that constructs the fdt nodes of platform bus devices
> > - */
> > -typedef struct PlatformBusFDTNotifierParams {
> > -    Notifier notifier;
> > -    ARMPlatformBusFDTParams *fdt_params;
> > -} PlatformBusFDTNotifierParams;
> > -
> >  /* struct that associates a device type name and a node creation function */
> >  typedef struct NodeCreationPair {
> >      const char *typename;
> > @@ -453,42 +444,17 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
> >      exit(1);
> >  }
> >  
> > -/**
> > - * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
> > - *
> > - * builds the parent platform bus node and all the nodes of dynamic
> > - * sysbus devices attached to it.
> > - */
> > -static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> > +void platform_bus_add_all_fdt_nodes(void *fdt, const char *intc, hwaddr addr,
> > +                                    hwaddr bus_size, int irq_start)
> >  {
> >      const char platcomp[] = "qemu,platform\0simple-bus";
> > -    PlatformBusDevice *pbus;
> >      DeviceState *dev;
> > +    PlatformBusDevice *pbus;  
> 
> Unnecessary change, but whatever
dropped it

> 
> >      gchar *node;
> > -    uint64_t addr, size;
> > -    int irq_start, dtb_size;
> > -    struct arm_boot_info *info = fdt_params->binfo;
> > -    const ARMPlatformBusSystemParams *params = fdt_params->system_params;
> > -    const char *intc = fdt_params->intc;
> > -    void *fdt = info->get_dtb(info, &dtb_size);
> > -
> > -    /*
> > -     * If the user provided a dtb, we assume the dynamic sysbus nodes
> > -     * already are integrated there. This corresponds to a use case where
> > -     * the dynamic sysbus nodes are complex and their generation is not yet
> > -     * supported. In that case the user can take charge of the guest dt
> > -     * while qemu takes charge of the qom stuff.
> > -     */
> > -    if (info->dtb_filename) {
> > -        return;
> > -    }
> >  
> >      assert(fdt);
> >  
> > -    node = g_strdup_printf("/platform@%"PRIx64, params->platform_bus_base);
> > -    addr = params->platform_bus_base;
> > -    size = params->platform_bus_size;
> > -    irq_start = params->platform_bus_first_irq;
> > +    node = g_strdup_printf("/platform@%"PRIx64, addr);
> >  
> >      /* Create a /platform node that we can put all devices into */
> >      qemu_fdt_add_subnode(fdt, node);
> > @@ -499,10 +465,11 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> >       */
> >      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_cells(fdt, node, "ranges", 0, addr >> 32, addr, bus_size);
> >  
> >      qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
> >  
> > +  
> 
> Extra blank line added here
Fixed

> 
> >      dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
> >      pbus = PLATFORM_BUS_DEVICE(dev);
> >  
> > @@ -518,22 +485,3 @@ static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> >  
> >      g_free(node);
> >  }
> > -
> > -static void platform_bus_fdt_notify(Notifier *notifier, void *data)
> > -{
> > -    PlatformBusFDTNotifierParams *p = DO_UPCAST(PlatformBusFDTNotifierParams,
> > -                                                notifier, notifier);
> > -
> > -    add_all_platform_bus_fdt_nodes(p->fdt_params);
> > -    g_free(p->fdt_params);
> > -    g_free(p);
> > -}
> > -
> > -void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params)
> > -{
> > -    PlatformBusFDTNotifierParams *p = g_new(PlatformBusFDTNotifierParams, 1);
> > -
> > -    p->fdt_params = fdt_params;
> > -    p->notifier.notify = platform_bus_fdt_notify;
> > -    qemu_add_machine_init_done_notifier(&p->notifier);
> > -}
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 112c367..1402149 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -93,8 +93,6 @@
> >  
> >  #define PLATFORM_BUS_NUM_IRQS 64
> >  
> > -static ARMPlatformBusSystemParams platform_bus_params;
> > -
> >  /* RAM limit in GB. Since VIRT_MEM starts at the 1GB mark, this means
> >   * RAM can go up to the 256GB mark, leaving 256GB of the physical
> >   * address space unallocated and free for future use between 256G and 512G.
> > @@ -1063,40 +1061,23 @@ static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
> >      DeviceState *dev;
> >      SysBusDevice *s;
> >      int i;
> > -    ARMPlatformBusFDTParams *fdt_params = g_new(ARMPlatformBusFDTParams, 1);
> >      MemoryRegion *sysmem = get_system_memory();
> >  
> > -    platform_bus_params.platform_bus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> > -    platform_bus_params.platform_bus_size = vms->memmap[VIRT_PLATFORM_BUS].size;
> > -    platform_bus_params.platform_bus_first_irq = vms->irqmap[VIRT_PLATFORM_BUS];
> > -    platform_bus_params.platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS;
> > -
> > -    fdt_params->system_params = &platform_bus_params;
> > -    fdt_params->binfo = &vms->bootinfo;
> > -    fdt_params->intc = "/intc";
> > -    /*
> > -     * register a machine init done notifier that creates the device tree
> > -     * nodes of the platform bus and its children dynamic sysbus devices
> > -     */
> > -    arm_register_platform_bus_fdt_creator(fdt_params);
> > -
> >      dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
> >      dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > -    qdev_prop_set_uint32(dev, "num_irqs",
> > -        platform_bus_params.platform_bus_num_irqs);
> > -    qdev_prop_set_uint32(dev, "mmio_size",
> > -        platform_bus_params.platform_bus_size);
> > +    qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> > +    qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
> >      qdev_init_nofail(dev);
> >      vms->platform_bus_dev = dev;
> > -    s = SYS_BUS_DEVICE(dev);
> >  
> > -    for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
> > -        int irqn = platform_bus_params.platform_bus_first_irq + i;
> > +    s = SYS_BUS_DEVICE(dev);
> > +    for (i = 0; i < PLATFORM_BUS_NUM_IRQS; i++) {
> > +        int irqn = vms->irqmap[VIRT_PLATFORM_BUS] + i;
> >          sysbus_connect_irq(s, i, pic[irqn]);
> >      }
> >  
> >      memory_region_add_subregion(sysmem,
> > -                                platform_bus_params.platform_bus_base,
> > +                                vms->memmap[VIRT_PLATFORM_BUS].base,
> >                                  sysbus_mmio_get_region(s, 0));
> >  }
> >  
> > @@ -1167,6 +1148,26 @@ void virt_machine_done(Notifier *notifier, void *data)
> >  {
> >      VirtMachineState *vms = container_of(notifier, VirtMachineState,
> >                                           machine_done);
> > +    ARMCPU *cpu = ARM_CPU(first_cpu);
> > +    struct arm_boot_info *info = &vms->bootinfo;
> > +    AddressSpace *as = arm_boot_address_space(cpu, info);
> > +
> > +    /*
> > +     * If the user provided a dtb, we assume the dynamic sysbus nodes
> > +     * already are integrated there. This corresponds to a use case where
> > +     * the dynamic sysbus nodes are complex and their generation is not yet
> > +     * supported. In that case the user can take charge of the guest dt
> > +     * while qemu takes charge of the qom stuff.
> > +     */
> > +    if (info->dtb_filename == NULL) {
> > +        platform_bus_add_all_fdt_nodes(vms->fdt, "/intc",
> > +                                       vms->memmap[VIRT_PLATFORM_BUS].base,
> > +                                       vms->memmap[VIRT_PLATFORM_BUS].size,
> > +                                       vms->irqmap[VIRT_PLATFORM_BUS]);
> > +    }
> > +    if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {  
> 
> I think we can change arm_load_dtb() to just take cpu and info. Then we
> don't need to make arm_boot_address_space() global, as we'd just call
> it from arm_load_dtb(). Actually even 'cpu' is debatable, because it
> should probably always be first_cpu, so we could just use that in
> arm_load_dtb() as well.
I'll leave it as is for this series as it's beyond of scope of this series.

> > +        exit(1);
> > +    }
> >  
> >      virt_acpi_setup(vms);
> >      virt_build_smbios(vms);
> > @@ -1394,8 +1395,7 @@ static void machvirt_init(MachineState *machine)
> >      vms->fw_cfg = create_fw_cfg(vms, &address_space_memory);
> >      rom_set_fw(vms->fw_cfg);
> >  
> > -    vms->machine_done.notify = virt_machine_done;
> > -    qemu_add_machine_init_done_notifier(&vms->machine_done);
> > +    create_platform_bus(vms, pic);
> >  
> >      vms->bootinfo.ram_size = machine->ram_size;
> >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> > @@ -1405,16 +1405,12 @@ static void machvirt_init(MachineState *machine)
> >      vms->bootinfo.board_id = -1;
> >      vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
> >      vms->bootinfo.get_dtb = machvirt_dtb;
> > +    vms->bootinfo.skip_dtb_autoload = true;
> >      vms->bootinfo.firmware_loaded = firmware_loaded;
> >      arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo);
> >  
> > -    /*
> > -     * arm_load_kernel machine init done notifier registration must
> > -     * happen before the platform_bus_create call. In this latter,
> > -     * another notifier is registered which adds platform bus nodes.
> > -     * Notifiers are executed in registration reverse order.
> > -     */
> > -    create_platform_bus(vms, pic);
> > +    vms->machine_done.notify = virt_machine_done;
> > +    qemu_add_machine_init_done_notifier(&vms->machine_done);
> >  }
> >  
> >  static bool virt_get_secure(Object *obj, Error **errp)
> > -- 
> > 2.7.4
> > 
> >  
> 
> Nice cleanup, particularly regarding the platform bus fdt node parameter
> passing. The review would be a bit easier if we did the conversion without
> deletion in one patch and deletion in a second patch, but then compiling
> would complain about unused code and with warnings treated as errors that
> would break bisection, so I guess the reviewers just have to work harder.
> 
> Besides the nits and ensuring dtb_limit=0 when it should be,
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
Thanks!