Patchwork [2/4] PPC: Qdev'ify e500 pci

login
register
mail settings
Submitter Alexander Graf
Date Sept. 7, 2010, 11:53 a.m.
Message ID <1283860398-11637-3-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/63998/
State New
Headers show

Comments

Alexander Graf - Sept. 7, 2010, 11:53 a.m.
The e500 PCI controller isn't qdev'ified yet. This leads to severe issues
when running with -drive.

To be able to use a virtio disk with an e500 VM, let's convert the PCI
controller over to qdev.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppce500_pci.c |  106 +++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 73 insertions(+), 33 deletions(-)
Blue Swirl - Sept. 7, 2010, 6:21 p.m.
On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf <agraf@suse.de> wrote:
> The e500 PCI controller isn't qdev'ified yet. This leads to severe issues
> when running with -drive.
>
> To be able to use a virtio disk with an e500 VM, let's convert the PCI
> controller over to qdev.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ppce500_pci.c |  106 +++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 73 insertions(+), 33 deletions(-)
>
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 8ac99f2..3fa42d2 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -73,11 +73,11 @@ struct pci_inbound {
>  };
>
>  struct PPCE500PCIState {
> +    PCIHostState pci_state;
>     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>     uint32_t gasket_time;
> -    PCIHostState pci_state;
> -    PCIDevice *pci_dev;
> +    uint64_t base_addr;
>  };
>
>  typedef struct PPCE500PCIState PPCE500PCIState;
> @@ -221,7 +221,7 @@ static void ppce500_pci_save(QEMUFile *f, void *opaque)
>     PPCE500PCIState *controller = opaque;
>     int i;
>
> -    pci_device_save(controller->pci_dev, f);
> +    /* pci_device_save(controller->pci_dev, f); */

Why, is loading/saving broken?

>
>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>         qemu_put_be32s(f, &controller->pob[i].potar);
> @@ -247,7 +247,7 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>     if (version_id != 1)
>         return -EINVAL;
>
> -    pci_device_load(controller->pci_dev, f);
> +    /* pci_device_load(controller->pci_dev, f); */
>
>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>         qemu_get_be32s(f, &controller->pob[i].potar);
> @@ -269,55 +269,95 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>
>  PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>  {
> -    PPCE500PCIState *controller;
> +    DeviceState *dev;
> +    PCIBus *b;
> +    PCIHostState *h;
> +    PPCE500PCIState *s;
>     PCIDevice *d;
> -    int index;
>     static int ppce500_pci_id;
>
> -    controller = qemu_mallocz(sizeof(PPCE500PCIState));
> +    dev = qdev_create(NULL, "e500-pcihost");
> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
> +
> +    qdev_prop_set_uint64(dev, "base_addr", registers);

This property should not be needed. You should simply use
sysbus_mmio_map() here.  See for example grackle_pci.c.

> +    b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
> +                         mpc85xx_pci_map_irq, pci_irqs, PCI_DEVFN(0x11, 0), 4);
> +
> +    s->pci_state.bus = b;
> +    qdev_init_nofail(dev);
> +    d = pci_create_simple(b, 0, "e500-host-bridge");
> +
> +    /* XXX load/save code not tested. */
> +    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
> +                    1, ppce500_pci_save, ppce500_pci_load, s);

It would be nice if you also converted the device to VMState and vmsd.
A reset function would be cool too, if it's needed after Anthony's
reset changes.

>
> -    controller->pci_state.bus = pci_register_bus(NULL, "pci",
> -                                                 mpc85xx_pci_set_irq,
> -                                                 mpc85xx_pci_map_irq,
> -                                                 pci_irqs, PCI_DEVFN(0x11, 0),
> -                                                 4);
> -    d = pci_register_device(controller->pci_state.bus,
> -                            "host bridge", sizeof(PCIDevice),
> -                            0, NULL, NULL);
> +    return b;
> +}
>
> -    pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_FREESCALE);
> -    pci_config_set_device_id(d->config, PCI_DEVICE_ID_MPC8533E);
> -    pci_config_set_class(d->config, PCI_CLASS_PROCESSOR_POWERPC);
> +static int e500_pcihost_initfn(SysBusDevice *dev)
> +{
> +    PCIHostState *h;
> +    PPCE500PCIState *s;
> +    target_phys_addr_t registers;
> +    int index;
>
> -    controller->pci_dev = d;
> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
> +    registers = (target_phys_addr_t)s->base_addr;
>
>     /* CFGADDR */
> -    index = pci_host_conf_register_mmio(&controller->pci_state, 0);
> +    index = pci_host_conf_register_mmio(&s->pci_state, 0);
>     if (index < 0)
> -        goto free;
> +        return -1;
>     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);

Instead of cpu_register_physical_memory(), you should use
sysbus_register_mmio().

>
>     /* CFGDATA */
> -    index = pci_host_data_register_mmio(&controller->pci_state, 0);
> +    index = pci_host_data_register_mmio(&s->pci_state, 0);
>     if (index < 0)
> -        goto free;
> +        return -1;
>     cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
>
>     index = cpu_register_io_memory(e500_pci_reg_read,
> -                                   e500_pci_reg_write, controller);
> +                                   e500_pci_reg_write, s);
>     if (index < 0)
> -        goto free;
> +        return -1;
>     cpu_register_physical_memory(registers + PCIE500_REG_BASE,
>                                    PCIE500_REG_SIZE, index);
> +    return 0;
> +}
>
> -    /* XXX load/save code not tested. */
> -    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
> -                    1, ppce500_pci_save, ppce500_pci_load, controller);
> +static int e500_host_bridge_initfn(PCIDevice *dev)
> +{
> +    pci_config_set_vendor_id(dev->config, PCI_VENDOR_ID_FREESCALE);
> +    pci_config_set_device_id(dev->config, PCI_DEVICE_ID_MPC8533E);
> +    pci_config_set_class(dev->config, PCI_CLASS_PROCESSOR_POWERPC);
> +
> +    return 0;
> +}
> +
> +static PCIDeviceInfo e500_host_bridge_info = {
> +    .qdev.name    = "e500-host-bridge",
> +    .qdev.desc    = "Host bridge",
> +    .qdev.size    = sizeof(PCIDevice),
> +    .qdev.no_user = 1,
> +    .init         = e500_host_bridge_initfn,
> +};
>
> -    return controller->pci_state.bus;
> +static SysBusDeviceInfo e500_pcihost_info = {
> +    .init         = e500_pcihost_initfn,
> +    .qdev.name    = "e500-pcihost",
> +    .qdev.size    = sizeof(PPCE500PCIState),
> +    .qdev.no_user = 1,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT64("base_addr", PPCE500PCIState, base_addr, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
>
> -free:
> -    printf("%s error\n", __func__);
> -    qemu_free(controller);
> -    return NULL;
> +static void e500_pci_register(void)
> +{
> +    sysbus_register_withprop(&e500_pcihost_info);
> +    pci_qdev_register(&e500_host_bridge_info);
>  }
> +device_init(e500_pci_register);
> --
> 1.6.0.2
>
>
>
Alexander Graf - Sept. 7, 2010, 9:33 p.m.
On 07.09.2010, at 20:21, Blue Swirl wrote:

> On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf <agraf@suse.de> wrote:
>> The e500 PCI controller isn't qdev'ified yet. This leads to severe issues
>> when running with -drive.
>> 
>> To be able to use a virtio disk with an e500 VM, let's convert the PCI
>> controller over to qdev.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  hw/ppce500_pci.c |  106 +++++++++++++++++++++++++++++++++++++-----------------
>>  1 files changed, 73 insertions(+), 33 deletions(-)
>> 
>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>> index 8ac99f2..3fa42d2 100644
>> --- a/hw/ppce500_pci.c
>> +++ b/hw/ppce500_pci.c
>> @@ -73,11 +73,11 @@ struct pci_inbound {
>>  };
>> 
>>  struct PPCE500PCIState {
>> +    PCIHostState pci_state;
>>     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>>     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>>     uint32_t gasket_time;
>> -    PCIHostState pci_state;
>> -    PCIDevice *pci_dev;
>> +    uint64_t base_addr;
>>  };
>> 
>>  typedef struct PPCE500PCIState PPCE500PCIState;
>> @@ -221,7 +221,7 @@ static void ppce500_pci_save(QEMUFile *f, void *opaque)
>>     PPCE500PCIState *controller = opaque;
>>     int i;
>> 
>> -    pci_device_save(controller->pci_dev, f);
>> +    /* pci_device_save(controller->pci_dev, f); */
> 
> Why, is loading/saving broken?

It was never tested before and save/restore won't work for this combination anyways, because it requires KVM which doesn't implement full save/restore yet FWIW.

> 
>> 
>>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>>         qemu_put_be32s(f, &controller->pob[i].potar);
>> @@ -247,7 +247,7 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>>     if (version_id != 1)
>>         return -EINVAL;
>> 
>> -    pci_device_load(controller->pci_dev, f);
>> +    /* pci_device_load(controller->pci_dev, f); */
>> 
>>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>>         qemu_get_be32s(f, &controller->pob[i].potar);
>> @@ -269,55 +269,95 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>> 
>>  PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>>  {
>> -    PPCE500PCIState *controller;
>> +    DeviceState *dev;
>> +    PCIBus *b;
>> +    PCIHostState *h;
>> +    PPCE500PCIState *s;
>>     PCIDevice *d;
>> -    int index;
>>     static int ppce500_pci_id;
>> 
>> -    controller = qemu_mallocz(sizeof(PPCE500PCIState));
>> +    dev = qdev_create(NULL, "e500-pcihost");
>> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
>> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
>> +
>> +    qdev_prop_set_uint64(dev, "base_addr", registers);
> 
> This property should not be needed. You should simply use
> sysbus_mmio_map() here.  See for example grackle_pci.c.

The base address can be different for different boards. I thought the idea of the qdev variables was to enable machine description files one day in which case we'll have to pass it?

> 
>> +    b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
>> +                         mpc85xx_pci_map_irq, pci_irqs, PCI_DEVFN(0x11, 0), 4);
>> +
>> +    s->pci_state.bus = b;
>> +    qdev_init_nofail(dev);
>> +    d = pci_create_simple(b, 0, "e500-host-bridge");
>> +
>> +    /* XXX load/save code not tested. */
>> +    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
>> +                    1, ppce500_pci_save, ppce500_pci_load, s);
> 
> It would be nice if you also converted the device to VMState and vmsd.
> A reset function would be cool too, if it's needed after Anthony's
> reset changes.

I agree 100%. Next time I'll touch the code will probably be to convert it to VMState too. For now, qdev got me -drive working, so it was the more pressing issue :).

> 
>> 
>> -    controller->pci_state.bus = pci_register_bus(NULL, "pci",
>> -                                                 mpc85xx_pci_set_irq,
>> -                                                 mpc85xx_pci_map_irq,
>> -                                                 pci_irqs, PCI_DEVFN(0x11, 0),
>> -                                                 4);
>> -    d = pci_register_device(controller->pci_state.bus,
>> -                            "host bridge", sizeof(PCIDevice),
>> -                            0, NULL, NULL);
>> +    return b;
>> +}
>> 
>> -    pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_FREESCALE);
>> -    pci_config_set_device_id(d->config, PCI_DEVICE_ID_MPC8533E);
>> -    pci_config_set_class(d->config, PCI_CLASS_PROCESSOR_POWERPC);
>> +static int e500_pcihost_initfn(SysBusDevice *dev)
>> +{
>> +    PCIHostState *h;
>> +    PPCE500PCIState *s;
>> +    target_phys_addr_t registers;
>> +    int index;
>> 
>> -    controller->pci_dev = d;
>> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
>> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
>> +    registers = (target_phys_addr_t)s->base_addr;
>> 
>>     /* CFGADDR */
>> -    index = pci_host_conf_register_mmio(&controller->pci_state, 0);
>> +    index = pci_host_conf_register_mmio(&s->pci_state, 0);
>>     if (index < 0)
>> -        goto free;
>> +        return -1;
>>     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
> 
> Instead of cpu_register_physical_memory(), you should use
> sysbus_register_mmio().

Oh? Interesting. I'll change that then.


Alex
Blue Swirl - Sept. 8, 2010, 5:38 p.m.
On Tue, Sep 7, 2010 at 9:33 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.09.2010, at 20:21, Blue Swirl wrote:
>
>> On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf <agraf@suse.de> wrote:
>>> The e500 PCI controller isn't qdev'ified yet. This leads to severe issues
>>> when running with -drive.
>>>
>>> To be able to use a virtio disk with an e500 VM, let's convert the PCI
>>> controller over to qdev.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  hw/ppce500_pci.c |  106 +++++++++++++++++++++++++++++++++++++-----------------
>>>  1 files changed, 73 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>>> index 8ac99f2..3fa42d2 100644
>>> --- a/hw/ppce500_pci.c
>>> +++ b/hw/ppce500_pci.c
>>> @@ -73,11 +73,11 @@ struct pci_inbound {
>>>  };
>>>
>>>  struct PPCE500PCIState {
>>> +    PCIHostState pci_state;
>>>     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>>>     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>>>     uint32_t gasket_time;
>>> -    PCIHostState pci_state;
>>> -    PCIDevice *pci_dev;
>>> +    uint64_t base_addr;
>>>  };
>>>
>>>  typedef struct PPCE500PCIState PPCE500PCIState;
>>> @@ -221,7 +221,7 @@ static void ppce500_pci_save(QEMUFile *f, void *opaque)
>>>     PPCE500PCIState *controller = opaque;
>>>     int i;
>>>
>>> -    pci_device_save(controller->pci_dev, f);
>>> +    /* pci_device_save(controller->pci_dev, f); */
>>
>> Why, is loading/saving broken?
>
> It was never tested before and save/restore won't work for this combination anyways, because it requires KVM which doesn't implement full save/restore yet FWIW.
>
>>
>>>
>>>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>>>         qemu_put_be32s(f, &controller->pob[i].potar);
>>> @@ -247,7 +247,7 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>>>     if (version_id != 1)
>>>         return -EINVAL;
>>>
>>> -    pci_device_load(controller->pci_dev, f);
>>> +    /* pci_device_load(controller->pci_dev, f); */
>>>
>>>     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
>>>         qemu_get_be32s(f, &controller->pob[i].potar);
>>> @@ -269,55 +269,95 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
>>>
>>>  PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>>>  {
>>> -    PPCE500PCIState *controller;
>>> +    DeviceState *dev;
>>> +    PCIBus *b;
>>> +    PCIHostState *h;
>>> +    PPCE500PCIState *s;
>>>     PCIDevice *d;
>>> -    int index;
>>>     static int ppce500_pci_id;
>>>
>>> -    controller = qemu_mallocz(sizeof(PPCE500PCIState));
>>> +    dev = qdev_create(NULL, "e500-pcihost");
>>> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
>>> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
>>> +
>>> +    qdev_prop_set_uint64(dev, "base_addr", registers);
>>
>> This property should not be needed. You should simply use
>> sysbus_mmio_map() here.  See for example grackle_pci.c.
>
> The base address can be different for different boards. I thought the idea of the qdev variables was to enable machine description files one day in which case we'll have to pass it?

Yes, but the addresses will be specified by sysbus_mmio_map(), there
is very rarely a need to create a property for the base address.

>>> +    b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
>>> +                         mpc85xx_pci_map_irq, pci_irqs, PCI_DEVFN(0x11, 0), 4);
>>> +
>>> +    s->pci_state.bus = b;
>>> +    qdev_init_nofail(dev);
>>> +    d = pci_create_simple(b, 0, "e500-host-bridge");
>>> +
>>> +    /* XXX load/save code not tested. */
>>> +    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
>>> +                    1, ppce500_pci_save, ppce500_pci_load, s);
>>
>> It would be nice if you also converted the device to VMState and vmsd.
>> A reset function would be cool too, if it's needed after Anthony's
>> reset changes.
>
> I agree 100%. Next time I'll touch the code will probably be to convert it to VMState too. For now, qdev got me -drive working, so it was the more pressing issue :).
>
>>
>>>
>>> -    controller->pci_state.bus = pci_register_bus(NULL, "pci",
>>> -                                                 mpc85xx_pci_set_irq,
>>> -                                                 mpc85xx_pci_map_irq,
>>> -                                                 pci_irqs, PCI_DEVFN(0x11, 0),
>>> -                                                 4);
>>> -    d = pci_register_device(controller->pci_state.bus,
>>> -                            "host bridge", sizeof(PCIDevice),
>>> -                            0, NULL, NULL);
>>> +    return b;
>>> +}
>>>
>>> -    pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_FREESCALE);
>>> -    pci_config_set_device_id(d->config, PCI_DEVICE_ID_MPC8533E);
>>> -    pci_config_set_class(d->config, PCI_CLASS_PROCESSOR_POWERPC);
>>> +static int e500_pcihost_initfn(SysBusDevice *dev)
>>> +{
>>> +    PCIHostState *h;
>>> +    PPCE500PCIState *s;
>>> +    target_phys_addr_t registers;
>>> +    int index;
>>>
>>> -    controller->pci_dev = d;
>>> +    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
>>> +    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
>>> +    registers = (target_phys_addr_t)s->base_addr;
>>>
>>>     /* CFGADDR */
>>> -    index = pci_host_conf_register_mmio(&controller->pci_state, 0);
>>> +    index = pci_host_conf_register_mmio(&s->pci_state, 0);
>>>     if (index < 0)
>>> -        goto free;
>>> +        return -1;
>>>     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
>>
>> Instead of cpu_register_physical_memory(), you should use
>> sysbus_register_mmio().
>
> Oh? Interesting. I'll change that then.
>
>
> Alex
>
>

Patch

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 8ac99f2..3fa42d2 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -73,11 +73,11 @@  struct pci_inbound {
 };
 
 struct PPCE500PCIState {
+    PCIHostState pci_state;
     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
     uint32_t gasket_time;
-    PCIHostState pci_state;
-    PCIDevice *pci_dev;
+    uint64_t base_addr;
 };
 
 typedef struct PPCE500PCIState PPCE500PCIState;
@@ -221,7 +221,7 @@  static void ppce500_pci_save(QEMUFile *f, void *opaque)
     PPCE500PCIState *controller = opaque;
     int i;
 
-    pci_device_save(controller->pci_dev, f);
+    /* pci_device_save(controller->pci_dev, f); */
 
     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
         qemu_put_be32s(f, &controller->pob[i].potar);
@@ -247,7 +247,7 @@  static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    pci_device_load(controller->pci_dev, f);
+    /* pci_device_load(controller->pci_dev, f); */
 
     for (i = 0; i < PPCE500_PCI_NR_POBS; i++) {
         qemu_get_be32s(f, &controller->pob[i].potar);
@@ -269,55 +269,95 @@  static int ppce500_pci_load(QEMUFile *f, void *opaque, int version_id)
 
 PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
 {
-    PPCE500PCIState *controller;
+    DeviceState *dev;
+    PCIBus *b;
+    PCIHostState *h;
+    PPCE500PCIState *s;
     PCIDevice *d;
-    int index;
     static int ppce500_pci_id;
 
-    controller = qemu_mallocz(sizeof(PPCE500PCIState));
+    dev = qdev_create(NULL, "e500-pcihost");
+    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
+    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
+
+    qdev_prop_set_uint64(dev, "base_addr", registers);
+    b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
+                         mpc85xx_pci_map_irq, pci_irqs, PCI_DEVFN(0x11, 0), 4);
+
+    s->pci_state.bus = b;
+    qdev_init_nofail(dev);
+    d = pci_create_simple(b, 0, "e500-host-bridge");
+
+    /* XXX load/save code not tested. */
+    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
+                    1, ppce500_pci_save, ppce500_pci_load, s);
 
-    controller->pci_state.bus = pci_register_bus(NULL, "pci",
-                                                 mpc85xx_pci_set_irq,
-                                                 mpc85xx_pci_map_irq,
-                                                 pci_irqs, PCI_DEVFN(0x11, 0),
-                                                 4);
-    d = pci_register_device(controller->pci_state.bus,
-                            "host bridge", sizeof(PCIDevice),
-                            0, NULL, NULL);
+    return b;
+}
 
-    pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_FREESCALE);
-    pci_config_set_device_id(d->config, PCI_DEVICE_ID_MPC8533E);
-    pci_config_set_class(d->config, PCI_CLASS_PROCESSOR_POWERPC);
+static int e500_pcihost_initfn(SysBusDevice *dev)
+{
+    PCIHostState *h;
+    PPCE500PCIState *s;
+    target_phys_addr_t registers;
+    int index;
 
-    controller->pci_dev = d;
+    h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
+    s = DO_UPCAST(PPCE500PCIState, pci_state, h);
+    registers = (target_phys_addr_t)s->base_addr;
 
     /* CFGADDR */
-    index = pci_host_conf_register_mmio(&controller->pci_state, 0);
+    index = pci_host_conf_register_mmio(&s->pci_state, 0);
     if (index < 0)
-        goto free;
+        return -1;
     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
 
     /* CFGDATA */
-    index = pci_host_data_register_mmio(&controller->pci_state, 0);
+    index = pci_host_data_register_mmio(&s->pci_state, 0);
     if (index < 0)
-        goto free;
+        return -1;
     cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
 
     index = cpu_register_io_memory(e500_pci_reg_read,
-                                   e500_pci_reg_write, controller);
+                                   e500_pci_reg_write, s);
     if (index < 0)
-        goto free;
+        return -1;
     cpu_register_physical_memory(registers + PCIE500_REG_BASE,
                                    PCIE500_REG_SIZE, index);
+    return 0;
+}
 
-    /* XXX load/save code not tested. */
-    register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
-                    1, ppce500_pci_save, ppce500_pci_load, controller);
+static int e500_host_bridge_initfn(PCIDevice *dev)
+{
+    pci_config_set_vendor_id(dev->config, PCI_VENDOR_ID_FREESCALE);
+    pci_config_set_device_id(dev->config, PCI_DEVICE_ID_MPC8533E);
+    pci_config_set_class(dev->config, PCI_CLASS_PROCESSOR_POWERPC);
+
+    return 0;
+}
+
+static PCIDeviceInfo e500_host_bridge_info = {
+    .qdev.name    = "e500-host-bridge",
+    .qdev.desc    = "Host bridge",
+    .qdev.size    = sizeof(PCIDevice),
+    .qdev.no_user = 1,
+    .init         = e500_host_bridge_initfn,
+};
 
-    return controller->pci_state.bus;
+static SysBusDeviceInfo e500_pcihost_info = {
+    .init         = e500_pcihost_initfn,
+    .qdev.name    = "e500-pcihost",
+    .qdev.size    = sizeof(PPCE500PCIState),
+    .qdev.no_user = 1,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT64("base_addr", PPCE500PCIState, base_addr, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
 
-free:
-    printf("%s error\n", __func__);
-    qemu_free(controller);
-    return NULL;
+static void e500_pci_register(void)
+{
+    sysbus_register_withprop(&e500_pcihost_info);
+    pci_qdev_register(&e500_host_bridge_info);
 }
+device_init(e500_pci_register);