On 07.09.2010, at 20:21, Blue Swirl wrote:
> On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf <ag...@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 <ag...@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
