Re: [Qemu-devel] [PATCH 2/4] qdev: add children before qdev_init

[Anthony Liguori <aliguori@us.ibm.com>]

On 03/27/2012 11:38 AM, Paolo Bonzini wrote:
> We want the composition tree to to be in order by the time we call
> qdev_init, so that a single set of the toplevel realize property can
> propagate all the way down the composition tree.
>
> This is not the case so far.  Unfortunately, this is incompatible
> with calling qdev_init in the constructor wrappers for devices,
> so for now we need to unattach some devices that are created through
> those wrappers.  This will be fixed by removing qdev_init and instead
> setting the toplevel realize property after machine init.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   hw/pc_piix.c      |   18 +-----------------
>   hw/piix_pci.c     |    3 +--
>   hw/ppc_prep.c     |    2 +-
>   hw/qdev-monitor.c |    8 ++++----
>   4 files changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3f99f9a..747c40f 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -146,7 +146,6 @@ static void pc_init1(MemoryRegion *system_memory,
>       MemoryRegion *ram_memory;
>       MemoryRegion *pci_memory;
>       MemoryRegion *rom_memory;
> -    DeviceState *dev;
>
>       pc_cpus_init(cpu_model);
>
> @@ -224,11 +223,7 @@ static void pc_init1(MemoryRegion *system_memory,
>
>       pc_register_ferr_irq(gsi[13]);
>
> -    dev = pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
> -    if (dev) {
> -        object_property_add_child(object_get_root(), "vga", OBJECT(dev), NULL);
> -    }
> -
> +    pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>       if (xen_enabled()) {
>           pci_create_simple(pci_bus, -1, "xen-platform");
>       }
> @@ -255,17 +250,6 @@ static void pc_init1(MemoryRegion *system_memory,
>           }
>           idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
>           idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
> -
> -        /* FIXME there's some major spaghetti here.  Somehow we create the
> -         * devices on the PIIX before we actually create it.  We create the
> -         * PIIX3 deep in the recess of the i440fx creation too and then lose
> -         * the DeviceState.
> -         *
> -         * For now, let's "fix" this by making judicious use of paths.  This
> -         * is not generally the right way to do this.
> -         */
> -        object_property_add_child(object_resolve_path("/i440fx/piix3", NULL),
> -                                  "rtc", (Object *)rtc_state, NULL);
>       } else {
>           for(i = 0; i<  MAX_IDE_BUS; i++) {
>               ISADevice *dev;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index e0268fe..9017565 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -276,8 +276,8 @@ static PCIBus *i440fx_common_init(const char *device_name,
>       b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
>                       address_space_io, 0);
>       s->bus = b;
> -    qdev_init_nofail(dev);
>       object_property_add_child(object_get_root(), "i440fx", OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
>
>       d = pci_create_simple(b, 0, device_name);
>       *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
> @@ -316,7 +316,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
>           pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
>                   PIIX_NUM_PIRQS);
>       }
> -    object_property_add_child(OBJECT(dev), "piix3", OBJECT(piix3), NULL);
>       piix3->pic = pic;
>       *isa_bus = DO_UPCAST(ISABus, qbus,
>                            qdev_get_child_bus(&piix3->dev.qdev, "isa.0"));
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index 06d589d..86c9336 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -615,8 +615,8 @@ static void ppc_prep_init (ram_addr_t ram_size,
>       sys = sysbus_from_qdev(dev);
>       pcihost = DO_UPCAST(PCIHostState, busdev, sys);
>       pcihost->address_space = get_system_memory();
> -    qdev_init_nofail(dev);
>       object_property_add_child(object_get_root(), "raven", OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
>       pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>       if (pci_bus == NULL) {
>           fprintf(stderr, "Couldn't create PCI host controller.\n");
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 2e82962..031cb83 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -458,10 +458,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>           qdev_free(qdev);
>           return NULL;
>       }
> -    if (qdev_init(qdev)<  0) {
> -        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> -        return NULL;
> -    }
>       if (qdev->id) {
>           object_property_add_child(qdev_get_peripheral(), qdev->id,
>                                     OBJECT(qdev), NULL);
> @@ -472,6 +468,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>                                     OBJECT(qdev), NULL);
>           g_free(name);
>       }
> +    if (qdev_init(qdev)<  0) {
> +        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> +        return NULL;
> +    }
>       qdev->opts = opts;
>       return qdev;
>   }