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 <[email protected]> > --- > CC: [email protected] > CC: [email protected] > CC: [email protected] > --- > 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, ¶ms);
> }
>
> -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
signature.asc
Description: PGP signature
