[for-2.13,2/4] platform-bus-device: use device plug callback instead of machine_done notifier

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

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
>

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)

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

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

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

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.