qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/4] PPC: Qdev'ify e500 pci


From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH 2/4] PPC: Qdev'ify e500 pci
Date: Tue, 7 Sep 2010 23:33:44 +0200

On 07.09.2010, at 20:21, Blue Swirl wrote:

> On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf <address@hidden> 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 <address@hidden>
>> ---
>>  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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]