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

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

[Igor Mammedov]

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 (4):
  arm: reuse arm_boot_address_space() in armv7m_load_kernel()
  platform-bus-device: use device plug callback instead of machine_done
    notifier
  arm: always start from first_cpu when registering loader cpu reset
    callback
  arm/boot: split load_dtb() from arm_load_kernel()

 hw/ppc/e500.h               |   3 ++
 include/hw/arm/arm.h        |  39 ++++++++++++-----
 include/hw/arm/sysbus-fdt.h |  37 ++++------------
 include/hw/arm/virt.h       |   3 ++
 include/hw/platform-bus.h   |   4 +-
 hw/arm/armv7m.c             |  10 +----
 hw/arm/boot.c               |  85 +++++++++++--------------------------
 hw/arm/sysbus-fdt.c         |  67 +++--------------------------
 hw/arm/virt.c               | 100 +++++++++++++++++++++++++++++---------------
 hw/core/platform-bus.c      |  29 +++----------
 hw/ppc/e500.c               |  37 +++++++++-------
 hw/ppc/e500plat.c           |  60 +++++++++++++++++++++++++-
 12 files changed, 227 insertions(+), 247 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.13 1/4] arm: reuse arm_boot_address_space() in armv7m_load_kernel()

[Igor Mammedov]

reduce code duplication by resusing arm_boot_address_space()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/arm/arm.h |  2 ++
 hw/arm/armv7m.c      | 10 +---------
 hw/arm/boot.c        | 16 ++++++++--------
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index ce769bd..188d18b 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -143,6 +143,8 @@ struct arm_boot_info {
  */
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
 
+AddressSpace *arm_boot_address_space(ARMCPU *cpu, bool secure_boot);
+
 /* 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/hw/arm/armv7m.c b/hw/arm/armv7m.c
index f123cc7..d372d9c 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -289,8 +289,6 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
     uint64_t lowaddr;
     int big_endian;
     AddressSpace *as;
-    int asidx;
-    CPUState *cs = CPU(cpu);
 
 #ifdef TARGET_WORDS_BIGENDIAN
     big_endian = 1;
@@ -303,13 +301,7 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
         exit(1);
     }
 
-    if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) {
-        asidx = ARMASIdx_S;
-    } else {
-        asidx = ARMASIdx_NS;
-    }
-    as = cpu_get_address_space(cs, asidx);
-
+    as = arm_boot_address_space(cpu, true);
     if (kernel_filename) {
         image_size = load_elf_as(kernel_filename, NULL, NULL, &entry, &lowaddr,
                                  NULL, big_endian, EM_ARM, 1, 0, as);
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 26184bc..2f464ca 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -36,8 +36,7 @@
 #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, bool secure_boot)
 {
     /* 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
@@ -46,7 +45,7 @@ static AddressSpace *arm_boot_address_space(ARMCPU *cpu,
     int asidx;
     CPUState *cs = CPU(cpu);
 
-    if (arm_feature(&cpu->env, ARM_FEATURE_EL3) && info->secure_boot) {
+    if (arm_feature(&cpu->env, ARM_FEATURE_EL3) && secure_boot) {
         asidx = ARMASIdx_S;
     } else {
         asidx = ARMASIdx_NS;
@@ -193,7 +192,7 @@ static void default_write_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
     uint32_t fixupcontext[FIXUP_MAX];
-    AddressSpace *as = arm_boot_address_space(cpu, info);
+    AddressSpace *as = arm_boot_address_space(cpu, info->secure_boot);
 
     fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr;
     fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr;
@@ -211,7 +210,7 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                             const struct arm_boot_info *info,
                                             hwaddr mvbar_addr)
 {
-    AddressSpace *as = arm_boot_address_space(cpu, info);
+    AddressSpace *as = arm_boot_address_space(cpu, info->secure_boot);
     int n;
     uint32_t mvbar_blob[] = {
         /* mvbar_addr: secure monitor vectors
@@ -262,7 +261,7 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
 static void default_reset_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
-    AddressSpace *as = arm_boot_address_space(cpu, info);
+    AddressSpace *as = arm_boot_address_space(cpu, info->secure_boot);
     CPUState *cs = CPU(cpu);
 
     address_space_stl_notdirty(as, info->smp_bootreg_addr,
@@ -753,7 +752,8 @@ static void do_cpu_reset(void *opaque)
             }
 
             if (cs == first_cpu) {
-                AddressSpace *as = arm_boot_address_space(cpu, info);
+                AddressSpace *as =
+                    arm_boot_address_space(cpu, info->secure_boot);
 
                 cpu_set_pc(cs, info->loader_start);
 
@@ -950,7 +950,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     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);
+    AddressSpace *as = arm_boot_address_space(cpu, info->secure_boot);
 
     /* The board code is not supposed to set secure_board_setup unless
      * running its code in secure mode is actually possible, and KVM
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.13 1/4] arm: reuse arm_boot_address_space() in armv7m_load_kernel()

[Peter Maydell]

On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> wrote:
> reduce code duplication by resusing arm_boot_address_space()
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/arm/arm.h |  2 ++
>  hw/arm/armv7m.c      | 10 +---------
>  hw/arm/boot.c        | 16 ++++++++--------
>  3 files changed, 11 insertions(+), 17 deletions(-)

This minor duplication was deliberate, because hw/arm/armv7m.c
is for M profile cores only, and hw/arm/boot.c is for
A/R profile cores only. I don't think that saving a
single if/else in armv7m.c really justifies blurring
that dividing line.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.13 1/4] arm: reuse arm_boot_address_space() in armv7m_load_kernel()

[Igor Mammedov]

On Thu, 12 Apr 2018 19:22:13 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> wrote:
> > reduce code duplication by resusing arm_boot_address_space()
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/arm/arm.h |  2 ++
> >  hw/arm/armv7m.c      | 10 +---------
> >  hw/arm/boot.c        | 16 ++++++++--------
> >  3 files changed, 11 insertions(+), 17 deletions(-)  
> 
> This minor duplication was deliberate, because hw/arm/armv7m.c
> is for M profile cores only, and hw/arm/boot.c is for
> A/R profile cores only. I don't think that saving a
> single if/else in armv7m.c really justifies blurring
> that dividing line.
I'll drop patch on respin

> thanks
> -- PMM

[Qemu-devel] [PATCH for-2.13 2/4] 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
---
 hw/ppc/e500.h             |  3 +++
 include/hw/arm/virt.h     |  3 +++
 include/hw/platform-bus.h |  4 ++--
 hw/arm/sysbus-fdt.c       |  3 ---
 hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
 hw/core/platform-bus.c    | 29 ++++-------------------
 hw/ppc/e500.c             | 37 +++++++++++++++++------------
 hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
 8 files changed, 129 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 70ba1d8..d0f8ddd 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -2,6 +2,7 @@
 #define PPCE500_H
 
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 
 typedef struct PPCE500Params {
     int pci_first_slot;
@@ -26,6 +27,8 @@ typedef struct PPCE500Params {
 
 void ppce500_init(MachineState *machine, PPCE500Params *params);
 
+void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
+
 hwaddr booke206_page_size_to_tlb(uint64_t size);
 
 #endif
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ba0c1a4..5535760 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -86,11 +86,14 @@ typedef struct {
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
+    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
+                                           DeviceState *dev);
 } VirtMachineClass;
 
 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..2e10d8b 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,37 @@ 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_hotpug_handler(MachineState *machine,
+                                                       DeviceState *dev)
+{
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return vmc->get_hotplug_handler ?
+        vmc->get_hotplug_handler(machine, dev) : NULL;
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
+    vmc->get_hotplug_handler = mc->get_hotplug_handler;
+    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
+    hc->plug = virt_machine_device_plug_cb;
 }
 
 static const TypeInfo virt_machine_info = {
@@ -1566,6 +1598,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 9a85a41..e2829db 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -64,6 +64,8 @@
 #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
 #define MPC8XXX_GPIO_IRQ           47
 
+static SysBusDevice *pbus_dev;
+
 struct boot_info
 {
     uint32_t dt_base;
@@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
     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 = pbus,
+    };
 
-        /* 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);
 }
 
+void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
+{
+    if (pbus_dev) {
+        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
+    }
+}
+
 static int ppce500_load_device_tree(MachineState *machine,
                                     PPCE500Params *params,
                                     hwaddr addr,
@@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
         qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
         qdev_init_nofail(dev);
-        s = SYS_BUS_DEVICE(dev);
+        pbus_dev = SYS_BUS_DEVICE(dev);
 
         for (i = 0; i < params->platform_bus_num_irqs; i++) {
             int irqn = params->platform_bus_first_irq + i;
-            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
+            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
         }
 
         memory_region_add_subregion(address_space_mem,
                                     params->platform_bus_base,
-                                    sysbus_mmio_get_region(s, 0));
+                                    sysbus_mmio_get_region(pbus_dev, 0));
     }
 
     /*
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 81d03e1..2f014cc 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
     ppce500_init(machine, &params);
 }
 
-static void e500plat_machine_init(MachineClass *mc)
+typedef struct {
+    MachineClass parent;
+    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
+                                           DeviceState *dev);
+} E500PlatMachineClass;
+
+#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
+#define E500PLAT_MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
+#define E500PLAT_MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
+
+static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+                                            DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
+    }
+}
+
+static
+HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
+                                                    DeviceState *dev)
+{
+    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+
+    return emc->get_hotplug_handler ?
+        emc->get_hotplug_handler(machine, dev) : NULL;
+}
+
+static void e500plat_machine_class_init(ObjectClass *oc, void *data)
+{
+    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    emc->get_hotplug_handler = mc->get_hotplug_handler;
+    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
+    hc->plug = e500plat_machine_device_plug_cb;
+
     mc->desc = "generic paravirt e500 platform";
     mc->init = e500plat_init;
     mc->max_cpus = 32;
@@ -69,4 +110,19 @@ static void e500plat_machine_init(MachineClass *mc)
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
 }
 
-DEFINE_MACHINE("ppce500", e500plat_machine_init)
+static const TypeInfo e500plat_info = {
+    .name          = TYPE_E500PLAT_MACHINE,
+    .parent        = TYPE_MACHINE,
+    .class_size    = sizeof(E500PlatMachineClass),
+    .class_init    = e500plat_machine_class_init,
+    .interfaces    = (InterfaceInfo[]) {
+         { TYPE_HOTPLUG_HANDLER },
+         { }
+    }
+};
+
+static void e500plat_register_types(void)
+{
+    type_register_static(&e500plat_info);
+}
+type_init(e500plat_register_types)
-- 
2.7.4

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

[Auger Eric]

Hi Igor,

On 12/04/18 18:40, 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
> ---
>  hw/ppc/e500.h             |  3 +++
>  include/hw/arm/virt.h     |  3 +++
>  include/hw/platform-bus.h |  4 ++--
>  hw/arm/sysbus-fdt.c       |  3 ---
>  hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
>  hw/core/platform-bus.c    | 29 ++++-------------------
>  hw/ppc/e500.c             | 37 +++++++++++++++++------------
>  hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>  8 files changed, 129 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8..d0f8ddd 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -2,6 +2,7 @@
>  #define PPCE500_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  
>  typedef struct PPCE500Params {
>      int pci_first_slot;
> @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> +
>  hwaddr booke206_page_size_to_tlb(uint64_t size);
>  
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ba0c1a4..5535760 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -86,11 +86,14 @@ typedef struct {
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
> +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +                                           DeviceState *dev);
>  } VirtMachineClass;
>  
>  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..2e10d8b 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,37 @@ 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) {
do we need this check? Isn't this function supposed to be called only
after the machine creation, time at which the platform_bus_dev is created.
> +            platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
> +                                     SYS_BUS_DEVICE(dev));
> +        }
> +    }
> +}
> +
> +static HotplugHandler *virt_machine_get_hotpug_handler(MachineState *machine,
> +                                                       DeviceState *dev)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return vmc->get_hotplug_handler ?
> +        vmc->get_hotplug_handler(machine, dev) : NULL;
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
> +    vmc->get_hotplug_handler = mc->get_hotplug_handler;
> +    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
> +    hc->plug = virt_machine_device_plug_cb;
>  }
>  
>  static const TypeInfo virt_machine_info = {
> @@ -1566,6 +1598,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);
I have a doubt about this being moved at realize time and above comment.
Seems to say the userspace can define the gsi on the command line. I am
not used to this property, tried sysbus-irq[n] but the property does not
seem to be recognized.
> -
> -    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 9a85a41..e2829db 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -64,6 +64,8 @@
>  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
>  #define MPC8XXX_GPIO_IRQ           47
>  
> +static SysBusDevice *pbus_dev;
> +
>  struct boot_info
>  {
>      uint32_t dt_base;
> @@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
>      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 = pbus,
> +    };
>  
> -        /* 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);
>  }
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
> +{
> +    if (pbus_dev) {
> +        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
> +    }
> +}
> +
>  static int ppce500_load_device_tree(MachineState *machine,
>                                      PPCE500Params *params,
>                                      hwaddr addr,
> @@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
>          qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
>          qdev_init_nofail(dev);
> -        s = SYS_BUS_DEVICE(dev);
> +        pbus_dev = SYS_BUS_DEVICE(dev);
>  
>          for (i = 0; i < params->platform_bus_num_irqs; i++) {
>              int irqn = params->platform_bus_first_irq + i;
> -            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> +            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
>          }
>  
>          memory_region_add_subregion(address_space_mem,
>                                      params->platform_bus_base,
> -                                    sysbus_mmio_get_region(s, 0));
> +                                    sysbus_mmio_get_region(pbus_dev, 0));
>      }
>  
>      /*
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 81d03e1..2f014cc 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
>      ppce500_init(machine, &params);
>  }
>  
> -static void e500plat_machine_init(MachineClass *mc)
> +typedef struct {
> +    MachineClass parent;
> +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +                                           DeviceState *dev);
> +} E500PlatMachineClass;
> +
> +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> +#define E500PLAT_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
> +#define E500PLAT_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
> +
> +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                            DeviceState *dev, Error **errp)
>  {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
> +    }
> +}
> +
> +static
> +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> +                                                    DeviceState *dev)
> +{
> +    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return emc->get_hotplug_handler ?
> +        emc->get_hotplug_handler(machine, dev) : NULL;
> +}
> +
> +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    emc->get_hotplug_handler = mc->get_hotplug_handler;
> +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
> +    hc->plug = e500plat_machine_device_plug_cb;
> +
>      mc->desc = "generic paravirt e500 platform";
>      mc->init = e500plat_init;
>      mc->max_cpus = 32;
> @@ -69,4 +110,19 @@ static void e500plat_machine_init(MachineClass *mc)
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>  }
>  
> -DEFINE_MACHINE("ppce500", e500plat_machine_init)
> +static const TypeInfo e500plat_info = {
> +    .name          = TYPE_E500PLAT_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .class_size    = sizeof(E500PlatMachineClass),
> +    .class_init    = e500plat_machine_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +         { TYPE_HOTPLUG_HANDLER },
> +         { }
> +    }
> +};
> +
> +static void e500plat_register_types(void)
> +{
> +    type_register_static(&e500plat_info);
> +}
> +type_init(e500plat_register_types)

Otherwise I gave a try on Seattle with the Calxeda xgmac vfio-device and
I do not observe any regression with my sample command line.

Thanks

Eric
> 

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

[Igor Mammedov]

On Fri, 13 Apr 2018 20:00:18 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 12/04/18 18:40, 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
> > ---
> >  hw/ppc/e500.h             |  3 +++
> >  include/hw/arm/virt.h     |  3 +++
> >  include/hw/platform-bus.h |  4 ++--
> >  hw/arm/sysbus-fdt.c       |  3 ---
> >  hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
> >  hw/core/platform-bus.c    | 29 ++++-------------------
> >  hw/ppc/e500.c             | 37 +++++++++++++++++------------
> >  hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
> >  8 files changed, 129 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > index 70ba1d8..d0f8ddd 100644
> > --- a/hw/ppc/e500.h
> > +++ b/hw/ppc/e500.h
> > @@ -2,6 +2,7 @@
> >  #define PPCE500_H
> >  
> >  #include "hw/boards.h"
> > +#include "hw/sysbus.h"
> >  
> >  typedef struct PPCE500Params {
> >      int pci_first_slot;
> > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> >  
> >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > +
> >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> >  
> >  #endif
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index ba0c1a4..5535760 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -86,11 +86,14 @@ typedef struct {
> >      bool no_pmu;
> >      bool claim_edge_triggered_timers;
> >      bool smbios_old_sys_ver;
> > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +                                           DeviceState *dev);
> >  } VirtMachineClass;
> >  
> >  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..2e10d8b 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,37 @@ 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) {  
> do we need this check? Isn't this function supposed to be called only
> after the machine creation, time at which the platform_bus_dev is created.
it's necessary as device plug callback is called for every device
including platform_bus so we need to wait till vms->platform_bus_dev is set
before using it with platform_bus_link_device()

> > +            platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
> > +                                     SYS_BUS_DEVICE(dev));
> > +        }
> > +    }
> > +}
> > +
> > +static HotplugHandler *virt_machine_get_hotpug_handler(MachineState *machine,
> > +                                                       DeviceState *dev)
> > +{
> > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        return HOTPLUG_HANDLER(machine);
> > +    }
> > +
> > +    return vmc->get_hotplug_handler ?
> > +        vmc->get_hotplug_handler(machine, dev) : NULL;
> > +}
> > +
> >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > +    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
> > +    vmc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
> > +    hc->plug = virt_machine_device_plug_cb;
> >  }
> >  
> >  static const TypeInfo virt_machine_info = {
> > @@ -1566,6 +1598,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);  
> I have a doubt about this being moved at realize time and above comment.
> Seems to say the userspace can define the gsi on the command line. I am
> not used to this property, tried sysbus-irq[n] but the property does not
> seem to be recognized.
The only reason to call it at realize time is to get irq map of sysbus
devices that existed before plaform_bus device has been created.

For any devices that's created after that it's not needed,
device_plug callback will take care of it by calling
platform_bus_link_device() every time a sysbus device is
successfully realized. (i.e. every time -device option is processed)


> > -
> > -    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 9a85a41..e2829db 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -64,6 +64,8 @@
> >  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
> >  #define MPC8XXX_GPIO_IRQ           47
> >  
> > +static SysBusDevice *pbus_dev;
> > +
> >  struct boot_info
> >  {
> >      uint32_t dt_base;
> > @@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
> >      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 = pbus,
> > +    };
> >  
> > -        /* 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);
> >  }
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
> > +{
> > +    if (pbus_dev) {
> > +        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
> > +    }
> > +}
> > +
> >  static int ppce500_load_device_tree(MachineState *machine,
> >                                      PPCE500Params *params,
> >                                      hwaddr addr,
> > @@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> >          qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
> >          qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
> >          qdev_init_nofail(dev);
> > -        s = SYS_BUS_DEVICE(dev);
> > +        pbus_dev = SYS_BUS_DEVICE(dev);
> >  
> >          for (i = 0; i < params->platform_bus_num_irqs; i++) {
> >              int irqn = params->platform_bus_first_irq + i;
> > -            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> > +            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
> >          }
> >  
> >          memory_region_add_subregion(address_space_mem,
> >                                      params->platform_bus_base,
> > -                                    sysbus_mmio_get_region(s, 0));
> > +                                    sysbus_mmio_get_region(pbus_dev, 0));
> >      }
> >  
> >      /*
> > diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> > index 81d03e1..2f014cc 100644
> > --- a/hw/ppc/e500plat.c
> > +++ b/hw/ppc/e500plat.c
> > @@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
> >      ppce500_init(machine, &params);
> >  }
> >  
> > -static void e500plat_machine_init(MachineClass *mc)
> > +typedef struct {
> > +    MachineClass parent;
> > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +                                           DeviceState *dev);
> > +} E500PlatMachineClass;
> > +
> > +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> > +#define E500PLAT_MACHINE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
> > +#define E500PLAT_MACHINE_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
> > +
> > +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> > +                                            DeviceState *dev, Error **errp)
> >  {
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
> > +    }
> > +}
> > +
> > +static
> > +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> > +                                                    DeviceState *dev)
> > +{
> > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        return HOTPLUG_HANDLER(machine);
> > +    }
> > +
> > +    return emc->get_hotplug_handler ?
> > +        emc->get_hotplug_handler(machine, dev) : NULL;
> > +}
> > +
> > +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
> > +{
> > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    emc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;
> > +    hc->plug = e500plat_machine_device_plug_cb;
> > +
> >      mc->desc = "generic paravirt e500 platform";
> >      mc->init = e500plat_init;
> >      mc->max_cpus = 32;
> > @@ -69,4 +110,19 @@ static void e500plat_machine_init(MachineClass *mc)
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
> >  }
> >  
> > -DEFINE_MACHINE("ppce500", e500plat_machine_init)
> > +static const TypeInfo e500plat_info = {
> > +    .name          = TYPE_E500PLAT_MACHINE,
> > +    .parent        = TYPE_MACHINE,
> > +    .class_size    = sizeof(E500PlatMachineClass),
> > +    .class_init    = e500plat_machine_class_init,
> > +    .interfaces    = (InterfaceInfo[]) {
> > +         { TYPE_HOTPLUG_HANDLER },
> > +         { }
> > +    }
> > +};
> > +
> > +static void e500plat_register_types(void)
> > +{
> > +    type_register_static(&e500plat_info);
> > +}
> > +type_init(e500plat_register_types)  
> 
> Otherwise I gave a try on Seattle with the Calxeda xgmac vfio-device and
> I do not observe any regression with my sample command line.
> 
> Thanks
> 
> Eric
> >   

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

[David Gibson]

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

On Thu, Apr 12, 2018 at 06:40:19PM +0200, 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
> ---
>  hw/ppc/e500.h             |  3 +++
>  include/hw/arm/virt.h     |  3 +++
>  include/hw/platform-bus.h |  4 ++--
>  hw/arm/sysbus-fdt.c       |  3 ---
>  hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
>  hw/core/platform-bus.c    | 29 ++++-------------------
>  hw/ppc/e500.c             | 37 +++++++++++++++++------------
>  hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>  8 files changed, 129 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8..d0f8ddd 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -2,6 +2,7 @@
>  #define PPCE500_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  
>  typedef struct PPCE500Params {
>      int pci_first_slot;
> @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> +
>  hwaddr booke206_page_size_to_tlb(uint64_t size);
>  
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ba0c1a4..5535760 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -86,11 +86,14 @@ typedef struct {
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
> +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +                                           DeviceState *dev);
>  } VirtMachineClass;
>  
>  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..2e10d8b 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,37 @@ 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_hotpug_handler(MachineState *machine,
> +                                                       DeviceState *dev)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return vmc->get_hotplug_handler ?
> +        vmc->get_hotplug_handler(machine, dev) : NULL;
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
> +    vmc->get_hotplug_handler = mc->get_hotplug_handler;
> +    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
> +    hc->plug = virt_machine_device_plug_cb;
>  }
>  
>  static const TypeInfo virt_machine_info = {
> @@ -1566,6 +1598,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 9a85a41..e2829db 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -64,6 +64,8 @@
>  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
>  #define MPC8XXX_GPIO_IRQ           47
>  
> +static SysBusDevice *pbus_dev;

I'm not thrilled about adding a new global for information that really
belongs in the machine state.  Although I do recognize that adding an
actual MachineState subtype that didn't previously exist is a bit of a pain.

>  struct boot_info
>  {
>      uint32_t dt_base;
> @@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
>      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 = pbus,
> +    };
>  
> -        /* 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);
>  }
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
> +{
> +    if (pbus_dev) {
> +        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
> +    }
> +}
> +
>  static int ppce500_load_device_tree(MachineState *machine,
>                                      PPCE500Params *params,
>                                      hwaddr addr,
> @@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
>          qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
>          qdev_init_nofail(dev);
> -        s = SYS_BUS_DEVICE(dev);
> +        pbus_dev = SYS_BUS_DEVICE(dev);
>  
>          for (i = 0; i < params->platform_bus_num_irqs; i++) {
>              int irqn = params->platform_bus_first_irq + i;
> -            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> +            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
>          }
>  
>          memory_region_add_subregion(address_space_mem,
>                                      params->platform_bus_base,
> -                                    sysbus_mmio_get_region(s, 0));
> +                                    sysbus_mmio_get_region(pbus_dev, 0));
>      }
>  
>      /*
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 81d03e1..2f014cc 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
>      ppce500_init(machine, &params);
>  }
>  
> -static void e500plat_machine_init(MachineClass *mc)
> +typedef struct {
> +    MachineClass parent;
> +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +                                           DeviceState *dev);
> +} E500PlatMachineClass;
> +
> +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> +#define E500PLAT_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
> +#define E500PLAT_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
> +
> +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                            DeviceState *dev, Error **errp)
>  {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
> +    }
> +}
> +
> +static
> +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> +                                                    DeviceState *dev)
> +{
> +    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        return HOTPLUG_HANDLER(machine);
> +    }
> +
> +    return emc->get_hotplug_handler ?
> +        emc->get_hotplug_handler(machine, dev) : NULL;
> +}
> +
> +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    emc->get_hotplug_handler = mc->get_hotplug_handler;
> +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;

I'm pretty sure the parent type will never have a handler, so I'm not
sure this is really necessary.

> +    hc->plug = e500plat_machine_device_plug_cb;
> +
>      mc->desc = "generic paravirt e500 platform";
>      mc->init = e500plat_init;
>      mc->max_cpus = 32;
> @@ -69,4 +110,19 @@ static void e500plat_machine_init(MachineClass *mc)
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>  }
>  
> -DEFINE_MACHINE("ppce500", e500plat_machine_init)
> +static const TypeInfo e500plat_info = {
> +    .name          = TYPE_E500PLAT_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .class_size    = sizeof(E500PlatMachineClass),
> +    .class_init    = e500plat_machine_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +         { TYPE_HOTPLUG_HANDLER },
> +         { }
> +    }
> +};
> +
> +static void e500plat_register_types(void)
> +{
> +    type_register_static(&e500plat_info);
> +}
> +type_init(e500plat_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 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

[Igor Mammedov]

On Mon, 16 Apr 2018 12:43:55 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Apr 12, 2018 at 06:40:19PM +0200, 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
> > ---
> >  hw/ppc/e500.h             |  3 +++
> >  include/hw/arm/virt.h     |  3 +++
> >  include/hw/platform-bus.h |  4 ++--
> >  hw/arm/sysbus-fdt.c       |  3 ---
> >  hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
> >  hw/core/platform-bus.c    | 29 ++++-------------------
> >  hw/ppc/e500.c             | 37 +++++++++++++++++------------
> >  hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
> >  8 files changed, 129 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > index 70ba1d8..d0f8ddd 100644
> > --- a/hw/ppc/e500.h
> > +++ b/hw/ppc/e500.h
> > @@ -2,6 +2,7 @@
> >  #define PPCE500_H
> >  
> >  #include "hw/boards.h"
> > +#include "hw/sysbus.h"
> >  
> >  typedef struct PPCE500Params {
> >      int pci_first_slot;
> > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> >  
> >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > +
> >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> >  
> >  #endif
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index ba0c1a4..5535760 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -86,11 +86,14 @@ typedef struct {
> >      bool no_pmu;
> >      bool claim_edge_triggered_timers;
> >      bool smbios_old_sys_ver;
> > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +                                           DeviceState *dev);
> >  } VirtMachineClass;
> >  
> >  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..2e10d8b 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,37 @@ 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_hotpug_handler(MachineState *machine,
> > +                                                       DeviceState *dev)
> > +{
> > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        return HOTPLUG_HANDLER(machine);
> > +    }
> > +
> > +    return vmc->get_hotplug_handler ?
> > +        vmc->get_hotplug_handler(machine, dev) : NULL;
> > +}
> > +
> >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > +    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
> > +    vmc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
> > +    hc->plug = virt_machine_device_plug_cb;
> >  }
> >  
> >  static const TypeInfo virt_machine_info = {
> > @@ -1566,6 +1598,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 9a85a41..e2829db 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -64,6 +64,8 @@
> >  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
> >  #define MPC8XXX_GPIO_IRQ           47
> >  
> > +static SysBusDevice *pbus_dev;  
> 
> I'm not thrilled about adding a new global for information that really
> belongs in the machine state.  Although I do recognize that adding an
> actual MachineState subtype that didn't previously exist is a bit of a pain.
yep, adding MachineState seemed too much for the task that's why I've used global.
I can switch to actual MachineState here if you'd prefer it.


> >  struct boot_info
> >  {
> >      uint32_t dt_base;
> > @@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
> >      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 = pbus,
> > +    };
> >  
> > -        /* 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);
> >  }
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
> > +{
> > +    if (pbus_dev) {
> > +        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
> > +    }
> > +}
> > +
> >  static int ppce500_load_device_tree(MachineState *machine,
> >                                      PPCE500Params *params,
> >                                      hwaddr addr,
> > @@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> >          qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
> >          qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
> >          qdev_init_nofail(dev);
> > -        s = SYS_BUS_DEVICE(dev);
> > +        pbus_dev = SYS_BUS_DEVICE(dev);
> >  
> >          for (i = 0; i < params->platform_bus_num_irqs; i++) {
> >              int irqn = params->platform_bus_first_irq + i;
> > -            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> > +            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
> >          }
> >  
> >          memory_region_add_subregion(address_space_mem,
> >                                      params->platform_bus_base,
> > -                                    sysbus_mmio_get_region(s, 0));
> > +                                    sysbus_mmio_get_region(pbus_dev, 0));
> >      }
> >  
> >      /*
> > diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> > index 81d03e1..2f014cc 100644
> > --- a/hw/ppc/e500plat.c
> > +++ b/hw/ppc/e500plat.c
> > @@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
> >      ppce500_init(machine, &params);
> >  }
> >  
> > -static void e500plat_machine_init(MachineClass *mc)
> > +typedef struct {
> > +    MachineClass parent;
> > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +                                           DeviceState *dev);
> > +} E500PlatMachineClass;
> > +
> > +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> > +#define E500PLAT_MACHINE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
> > +#define E500PLAT_MACHINE_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
> > +
> > +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> > +                                            DeviceState *dev, Error **errp)
> >  {
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
> > +    }
> > +}
> > +
> > +static
> > +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> > +                                                    DeviceState *dev)
> > +{
> > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > +        return HOTPLUG_HANDLER(machine);
> > +    }
> > +
> > +    return emc->get_hotplug_handler ?
> > +        emc->get_hotplug_handler(machine, dev) : NULL;
> > +}
> > +
> > +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
> > +{
> > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    emc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;  
> 
> I'm pretty sure the parent type will never have a handler, so I'm not
> sure this is really necessary.
It seems that only PC machine handles chain correctly while other
targets /spapr,s390x/ don't.

Perhaps we should just drop caching original
MachineClass::get_hotplug_handler (which is NULL) everywhere
so it will be consistent across boards.
If we ever need chaining here, we could add it then and do
it consistently for every board that uses it.
 
> > +    hc->plug = e500plat_machine_device_plug_cb;
> > +
> >      mc->desc = "generic paravirt e500 platform";
> >      mc->init = e500plat_init;
> >      mc->max_cpus = 32;
> > @@ -69,4 +110,19 @@ static void e500plat_machine_init(MachineClass *mc)
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
> >  }
> >  
> > -DEFINE_MACHINE("ppce500", e500plat_machine_init)
> > +static const TypeInfo e500plat_info = {
> > +    .name          = TYPE_E500PLAT_MACHINE,
> > +    .parent        = TYPE_MACHINE,
> > +    .class_size    = sizeof(E500PlatMachineClass),
> > +    .class_init    = e500plat_machine_class_init,
> > +    .interfaces    = (InterfaceInfo[]) {
> > +         { TYPE_HOTPLUG_HANDLER },
> > +         { }
> > +    }
> > +};
> > +
> > +static void e500plat_register_types(void)
> > +{
> > +    type_register_static(&e500plat_info);
> > +}
> > +type_init(e500plat_register_types)  
> 

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

[David Gibson]

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

On Mon, Apr 16, 2018 at 10:19:14AM +0200, Igor Mammedov wrote:
> On Mon, 16 Apr 2018 12:43:55 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Apr 12, 2018 at 06:40:19PM +0200, 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
> > > ---
> > >  hw/ppc/e500.h             |  3 +++
> > >  include/hw/arm/virt.h     |  3 +++
> > >  include/hw/platform-bus.h |  4 ++--
> > >  hw/arm/sysbus-fdt.c       |  3 ---
> > >  hw/arm/virt.c             | 36 ++++++++++++++++++++++++++++
> > >  hw/core/platform-bus.c    | 29 ++++-------------------
> > >  hw/ppc/e500.c             | 37 +++++++++++++++++------------
> > >  hw/ppc/e500plat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++--
> > >  8 files changed, 129 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > > index 70ba1d8..d0f8ddd 100644
> > > --- a/hw/ppc/e500.h
> > > +++ b/hw/ppc/e500.h
> > > @@ -2,6 +2,7 @@
> > >  #define PPCE500_H
> > >  
> > >  #include "hw/boards.h"
> > > +#include "hw/sysbus.h"
> > >  
> > >  typedef struct PPCE500Params {
> > >      int pci_first_slot;
> > > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> > >  
> > >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> > >  
> > > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > > +
> > >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> > >  
> > >  #endif
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index ba0c1a4..5535760 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -86,11 +86,14 @@ typedef struct {
> > >      bool no_pmu;
> > >      bool claim_edge_triggered_timers;
> > >      bool smbios_old_sys_ver;
> > > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > +                                           DeviceState *dev);
> > >  } VirtMachineClass;
> > >  
> > >  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..2e10d8b 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,37 @@ 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_hotpug_handler(MachineState *machine,
> > > +                                                       DeviceState *dev)
> > > +{
> > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> > > +
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > > +        return HOTPLUG_HANDLER(machine);
> > > +    }
> > > +
> > > +    return vmc->get_hotplug_handler ?
> > > +        vmc->get_hotplug_handler(machine, dev) : NULL;
> > > +}
> > > +
> > >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      MachineClass *mc = MACHINE_CLASS(oc);
> > > +    VirtMachineClass *vmc = VIRT_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 +1586,9 @@ 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;
> > > +    vmc->get_hotplug_handler = mc->get_hotplug_handler;
> > > +    mc->get_hotplug_handler = virt_machine_get_hotpug_handler;
> > > +    hc->plug = virt_machine_device_plug_cb;
> > >  }
> > >  
> > >  static const TypeInfo virt_machine_info = {
> > > @@ -1566,6 +1598,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 9a85a41..e2829db 100644
> > > --- a/hw/ppc/e500.c
> > > +++ b/hw/ppc/e500.c
> > > @@ -64,6 +64,8 @@
> > >  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
> > >  #define MPC8XXX_GPIO_IRQ           47
> > >  
> > > +static SysBusDevice *pbus_dev;  
> > 
> > I'm not thrilled about adding a new global for information that really
> > belongs in the machine state.  Although I do recognize that adding an
> > actual MachineState subtype that didn't previously exist is a bit of a pain.
> yep, adding MachineState seemed too much for the task that's why I've used global.
> I can switch to actual MachineState here if you'd prefer it.

I would prefer that, please.  The code's already pretty ugly, let's
not make it uglier.

> 
> > >  struct boot_info
> > >  {
> > >      uint32_t dt_base;
> > > @@ -248,23 +250,28 @@ static void platform_bus_create_devtree(PPCE500Params *params, void *fdt,
> > >      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 = pbus,
> > > +    };
> > >  
> > > -        /* 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);
> > >  }
> > >  
> > > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev)
> > > +{
> > > +    if (pbus_dev) {
> > > +        platform_bus_link_device(PLATFORM_BUS_DEVICE(pbus_dev), sbdev);
> > > +    }
> > > +}
> > > +
> > >  static int ppce500_load_device_tree(MachineState *machine,
> > >                                      PPCE500Params *params,
> > >                                      hwaddr addr,
> > > @@ -945,16 +952,16 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> > >          qdev_prop_set_uint32(dev, "num_irqs", params->platform_bus_num_irqs);
> > >          qdev_prop_set_uint32(dev, "mmio_size", params->platform_bus_size);
> > >          qdev_init_nofail(dev);
> > > -        s = SYS_BUS_DEVICE(dev);
> > > +        pbus_dev = SYS_BUS_DEVICE(dev);
> > >  
> > >          for (i = 0; i < params->platform_bus_num_irqs; i++) {
> > >              int irqn = params->platform_bus_first_irq + i;
> > > -            sysbus_connect_irq(s, i, qdev_get_gpio_in(mpicdev, irqn));
> > > +            sysbus_connect_irq(pbus_dev, i, qdev_get_gpio_in(mpicdev, irqn));
> > >          }
> > >  
> > >          memory_region_add_subregion(address_space_mem,
> > >                                      params->platform_bus_base,
> > > -                                    sysbus_mmio_get_region(s, 0));
> > > +                                    sysbus_mmio_get_region(pbus_dev, 0));
> > >      }
> > >  
> > >      /*
> > > diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> > > index 81d03e1..2f014cc 100644
> > > --- a/hw/ppc/e500plat.c
> > > +++ b/hw/ppc/e500plat.c
> > > @@ -60,8 +60,49 @@ static void e500plat_init(MachineState *machine)
> > >      ppce500_init(machine, &params);
> > >  }
> > >  
> > > -static void e500plat_machine_init(MachineClass *mc)
> > > +typedef struct {
> > > +    MachineClass parent;
> > > +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > +                                           DeviceState *dev);
> > > +} E500PlatMachineClass;
> > > +
> > > +#define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
> > > +#define E500PLAT_MACHINE_GET_CLASS(obj) \
> > > +    OBJECT_GET_CLASS(E500PlatMachineClass, obj, TYPE_E500PLAT_MACHINE)
> > > +#define E500PLAT_MACHINE_CLASS(klass) \
> > > +    OBJECT_CLASS_CHECK(E500PlatMachineClass, klass, TYPE_E500PLAT_MACHINE)
> > > +
> > > +static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> > > +                                            DeviceState *dev, Error **errp)
> > >  {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > > +        ppce500_plug_dynamic_sysbus_device(SYS_BUS_DEVICE(dev));
> > > +    }
> > > +}
> > > +
> > > +static
> > > +HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
> > > +                                                    DeviceState *dev)
> > > +{
> > > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_GET_CLASS(machine);
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> > > +        return HOTPLUG_HANDLER(machine);
> > > +    }
> > > +
> > > +    return emc->get_hotplug_handler ?
> > > +        emc->get_hotplug_handler(machine, dev) : NULL;
> > > +}
> > > +
> > > +static void e500plat_machine_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    E500PlatMachineClass *emc = E500PLAT_MACHINE_CLASS(oc);
> > > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > > +    MachineClass *mc = MACHINE_CLASS(oc);
> > > +
> > > +    emc->get_hotplug_handler = mc->get_hotplug_handler;
> > > +    mc->get_hotplug_handler = e500plat_machine_get_hotpug_handler;  
> > 
> > I'm pretty sure the parent type will never have a handler, so I'm not
> > sure this is really necessary.
> It seems that only PC machine handles chain correctly while other
> targets /spapr,s390x/ don't.
> 
> Perhaps we should just drop caching original
> MachineClass::get_hotplug_handler (which is NULL) everywhere
> so it will be consistent across boards.
> If we ever need chaining here, we could add it then and do
> it consistently for every board that uses it.

Sounds reasonable to me.

-- 
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] 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>
---
tested only with make check.

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] ppc: e500: switch E500 based machines to full machine definition

[David Gibson]

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

On Tue, Apr 17, 2018 at 06:34:05PM +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>

Applied to ppc-for-2.13, thanks.

> ---
> tested only with make check.
> 
> 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 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

[Peter Maydell]

On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>

> +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_hotpug_handler(MachineState *machine,
> +                                                       DeviceState *dev)

Nit: typo in function name, should be "hotplug".

Will let others review the meat of the patch.

thanks
-- PMM

[Qemu-devel] [PATCH for-2.13 3/4] 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>
---
 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 2f464ca..2591fee 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 3/4] arm: always start from first_cpu when registering loader cpu reset callback

[Peter Maydell]

On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>
> ---
>  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 2f464ca..2591fee 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));
>      }
>  }

Definitely a bug fix, so:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I think though that in at least some cases we'll still mishandle
being passed anything other than first_cpu as the CPU pointer,
because in do_cpu_reset() we do some checks for "do this if
cs == first_cpu", on the assumption that first_cpu is the
primary CPU that we're booting. We should instead I suppose
be checking against the CPU pointer we're given as the
arm_load_kernel() argument (which I think do_cpu_reset() can
get at via info->load_kernel_notifier.cpu).

We should probably analyse which boards actually pass anything
other than first_cpu -- I suspect it will end up just being
the xilinx board which has both A and R profile cores...

thanks
-- PMM

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

[Igor Mammedov]

On Thu, 12 Apr 2018 19:29:28 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>
> > ---
> >  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 2f464ca..2591fee 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));
> >      }
> >  }  
> 
> Definitely a bug fix, so:
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I think though that in at least some cases we'll still mishandle
> being passed anything other than first_cpu as the CPU pointer,
> because in do_cpu_reset() we do some checks for "do this if
> cs == first_cpu", on the assumption that first_cpu is the
> primary CPU that we're booting. We should instead I suppose
> be checking against the CPU pointer we're given as the
> arm_load_kernel() argument (which I think do_cpu_reset() can
> get at via info->load_kernel_notifier.cpu).
Agreed,
only that load_kernel_notifier is being removed in 4/4,
but nothing prevents us from adding pointer to arm_boot_info directly.

> We should probably analyse which boards actually pass anything
> other than first_cpu -- I suspect it will end up just being
> the xilinx board which has both A and R profile cores...
Probably,
I've managed to stop myself digging deeper into reset handling for now,
and deal with firmware blobs regeneration first.
I still might have to look back at do_cpu_reset() later
if it proves to be in a way of cpu hotplug but not just yet.


> thanks
> -- PMM

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

[Peter Maydell]

On 12 April 2018 at 19:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>
>> ---
>>  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 2f464ca..2591fee 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));
>>      }
>>  }
>
> Definitely a bug fix, so:
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I think though that in at least some cases we'll still mishandle
> being passed anything other than first_cpu as the CPU pointer,
> because in do_cpu_reset() we do some checks for "do this if
> cs == first_cpu", on the assumption that first_cpu is the
> primary CPU that we're booting. We should instead I suppose
> be checking against the CPU pointer we're given as the
> arm_load_kernel() argument (which I think do_cpu_reset() can
> get at via info->load_kernel_notifier.cpu).
>
> We should probably analyse which boards actually pass anything
> other than first_cpu -- I suspect it will end up just being
> the xilinx board which has both A and R profile cores...

I did the check, and in fact all of our boards always pass the
first CPU as the boot CPU. Xilinx comes the closest, in that
the SoC object has a boot-cpu property to pick a boot CPU, but
since none of our boards set that it defaults to the first CPU.

Since this patch isn't very related to the rest of the series
I've just applied it to target-arm.next ready for 2.13 (with
a note in the commit message that in practice it isn't a behaviour
change). Let me know if that's awkward for you and you'd prefer
to keep it in this series, in which case I'll drop it.

thanks
-- PMM

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

[Igor Mammedov]

On Mon, 16 Apr 2018 18:17:51 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 12 April 2018 at 19:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>
> >> ---
> >>  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 2f464ca..2591fee 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));
> >>      }
> >>  }  
> >
> > Definitely a bug fix, so:
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > I think though that in at least some cases we'll still mishandle
> > being passed anything other than first_cpu as the CPU pointer,
> > because in do_cpu_reset() we do some checks for "do this if
> > cs == first_cpu", on the assumption that first_cpu is the
> > primary CPU that we're booting. We should instead I suppose
> > be checking against the CPU pointer we're given as the
> > arm_load_kernel() argument (which I think do_cpu_reset() can
> > get at via info->load_kernel_notifier.cpu).
> >
> > We should probably analyse which boards actually pass anything
> > other than first_cpu -- I suspect it will end up just being
> > the xilinx board which has both A and R profile cores...  
> 
> I did the check, and in fact all of our boards always pass the
> first CPU as the boot CPU. Xilinx comes the closest, in that
> the SoC object has a boot-cpu property to pick a boot CPU, but
> since none of our boards set that it defaults to the first CPU.
> 
> Since this patch isn't very related to the rest of the series
> I've just applied it to target-arm.next ready for 2.13 (with
> a note in the commit message that in practice it isn't a behaviour
> change). Let me know if that's awkward for you and you'd prefer
> to keep it in this series, in which case I'll drop it.
That's fine, I'll just rebase v2 on top of it.

> 
> thanks
> -- PMM

[Qemu-devel] [PATCH for-2.13 4/4] 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>
---
 include/hw/arm/arm.h        | 37 ++++++++++++++++++-------
 include/hw/arm/sysbus-fdt.h | 37 +++++--------------------
 hw/arm/boot.c               | 67 +++++++++++----------------------------------
 hw/arm/sysbus-fdt.c         | 64 ++++---------------------------------------
 hw/arm/virt.c               | 64 ++++++++++++++++++++-----------------------
 5 files changed, 86 insertions(+), 183 deletions(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 188d18b..312e533 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,9 @@ struct arm_boot_info {
     const char *initrd_filename;
     const char *dtb_filename;
     hwaddr loader_start;
+    hwaddr dtb_start;
+    hwaddr dtb_limit;
+    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 +89,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;
@@ -145,6 +138,30 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
 
 AddressSpace *arm_boot_address_space(ARMCPU *cpu, bool secure_boot);
 
+/**
+ * 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 2591fee..0cd2fb3 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -485,29 +485,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->secure_boot);
 
     /* 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 2e10d8b..4139e56 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->secure_boot);
+
+    /*
+     * 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 4/4] arm/boot: split load_dtb() from arm_load_kernel()

[Peter Maydell]

On 12 April 2018 at 17:40, Igor Mammedov <imammedo@redhat.com> 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>
> ---
>  include/hw/arm/arm.h        | 37 ++++++++++++++++++-------
>  include/hw/arm/sysbus-fdt.h | 37 +++++--------------------
>  hw/arm/boot.c               | 67 +++++++++++----------------------------------
>  hw/arm/sysbus-fdt.c         | 64 ++++---------------------------------------
>  hw/arm/virt.c               | 64 ++++++++++++++++++++-----------------------
>  5 files changed, 86 insertions(+), 183 deletions(-)
>
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 188d18b..312e533 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,9 @@ struct arm_boot_info {
>      const char *initrd_filename;
>      const char *dtb_filename;
>      hwaddr loader_start;
> +    hwaddr dtb_start;
> +    hwaddr dtb_limit;
> +    bool skip_dtb_autoload;

skip_dtb_autoload is a flag that the board code needs to set, so can
we have a comment documenting what it does, please?

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

Otherwise this looks good, especially the diffstat :-)
I'm assuming Eric will check the platform-bus/sysbus-fdt parts
of this patch.

thanks
-- PMM