Re: [Qemu-devel] [PATCH 5/8] ide/pci: convert to qdev.

[Markus Armbruster]

Gerd Hoffmann <address@hidden> writes:

> With this patch applied ide drives (when attached to a pci adapter) can
> be created via -device, like this:
>
>   -drive if=none,id=mydisk,file=/path/to/disk.img
>   -device ide-drive,drive=mydisk,bus=ide.0,unit=0
>
> Note that creating a master on ide1 doesn't work that way.  That is a
> side effect of qemu creating a cdrom automagically even if you don't
> ask for it.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  Makefile.target |   10 ++-
>  hw/ide/pci.c    |  205 
> ++++++++++++++++++++++++++++++++++---------------------
>  2 files changed, 133 insertions(+), 82 deletions(-)
>
[...]
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 607472b..aa6daf2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
[...]
> @@ -375,19 +391,13 @@ static void cmd646_reset(void *opaque)
>  }
>  
>  /* CMD646 PCI IDE controller */
> -void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> -                         int secondary_ide_enabled)
> +static int pci_cmd646_ide_initfn(PCIDevice *dev)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
> +    uint8_t *pci_conf = d->dev.config;
>      qemu_irq *irq;
>  
> -    d = (PCIIDEState *)pci_register_device(bus, "CMD646 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           -1,
> -                                           NULL, NULL);
>      d->type = IDE_TYPE_CMD646;
> -    pci_conf = d->dev.config;
>      pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CMD);
>      pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_CMD_646);
>  
> @@ -398,31 +408,47 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo 
> **hd_table,
>      pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
>  
>      pci_conf[0x51] = 0x04; // enable IDE0
> -    if (secondary_ide_enabled) {
> +    if (d->secondary) {
>          /* XXX: if not enabled, really disable the seconday IDE controller */
>          pci_conf[0x51] |= 0x08; /* enable IDE1 */
>      }
> +    pci_conf[0x3d] = 0x01; // interrupt on pin 1
>  
>      pci_register_bar((PCIDevice *)d, 0, 0x8,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 1, 0x4,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 2, 0x8,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 3, 0x4,
> -                           PCI_ADDRESS_SPACE_IO, ide_map);
> +                     PCI_ADDRESS_SPACE_IO, ide_map);
>      pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> +                     PCI_ADDRESS_SPACE_IO, bmdma_map);

Please don't mix white-space cleanups with complex changes.

>  
> -    pci_conf[0x3d] = 0x01; // interrupt on pin 1

Any particular reason for moving this assignment up?

> +    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
>  
> -    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], irq[0]);
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], irq[1]);
> +    d->bus[0] = ide_bus_new(&d->dev.qdev);
> +    d->bus[1] = ide_bus_new(&d->dev.qdev);
>  
> -    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
>      qemu_register_reset(cmd646_reset, d);
>      cmd646_reset(d);
> +
> +    irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
> +    ide_init2(d->bus[0], NULL, NULL, irq[0]);
> +    ide_init2(d->bus[1], NULL, NULL, irq[1]);
> +    return 0;
> +}
> +
> +void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> +                         int secondary_ide_enabled)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_noinit(bus, -1, "CMD646 IDE");
> +    qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
> +    qdev_init(&dev->qdev);
> +
> +    pci_ide_create_devs(dev, hd_table);
>  }
>  
>  static void piix3_reset(void *opaque)
> @@ -441,23 +467,10 @@ static void piix3_reset(void *opaque)
>      pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> -void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +static int pci_piix_ide_initfn(PCIIDEState *d)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> -
> -    /* register a function 1 of PIIX3 */
> -    d = (PCIIDEState *)pci_register_device(bus, "PIIX3 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           devfn,
> -                                           NULL, NULL);
> -    d->type = IDE_TYPE_PIIX3;
> +    uint8_t *pci_conf = d->dev.config;
>  
> -    pci_conf = d->dev.config;
> -    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> -    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_1);
>      pci_conf[0x09] = 0x80; // legacy ATA mode
>      pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
>      pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> @@ -466,48 +479,84 @@ void pci_piix3_ide_init(PCIBus *bus, DriveInfo 
> **hd_table, int devfn)
>      piix3_reset(d);
>  
>      pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> -
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
> -    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> -    ide_init_ioport(&d->bus[1], 0x170, 0x376);
> +                     PCI_ADDRESS_SPACE_IO, bmdma_map);
>  
>      register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
> +
> +    d->bus[0] = ide_bus_new(&d->dev.qdev);
> +    d->bus[1] = ide_bus_new(&d->dev.qdev);
> +    ide_init_ioport(d->bus[0], 0x1f0, 0x3f6);
> +    ide_init_ioport(d->bus[1], 0x170, 0x376);
> +
> +    ide_init2(d->bus[0], NULL, NULL, isa_reserve_irq(14));
> +    ide_init2(d->bus[1], NULL, NULL, isa_reserve_irq(15));
> +    return 0;
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +static int pci_piix3_ide_initfn(PCIDevice *dev)
>  {
> -    PCIIDEState *d;
> -    uint8_t *pci_conf;
> -
> -    /* register a function 1 of PIIX4 */
> -    d = (PCIIDEState *)pci_register_device(bus, "PIIX4 IDE",
> -                                           sizeof(PCIIDEState),
> -                                           devfn,
> -                                           NULL, NULL);
> -    d->type = IDE_TYPE_PIIX4;
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
>  
> -    pci_conf = d->dev.config;
> -    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> -    pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB);
> -    pci_conf[0x09] = 0x80; // legacy ATA mode
> -    pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
> -    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
> +    d->type = IDE_TYPE_PIIX3;
> +    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371SB_1);
> +    return pci_piix_ide_initfn(d);
> +}
>  
> -    qemu_register_reset(piix3_reset, d);
> -    piix3_reset(d);
> +static int pci_piix4_ide_initfn(PCIDevice *dev)
> +{
> +    PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
>  
> -    pci_register_bar((PCIDevice *)d, 4, 0x10,
> -                           PCI_ADDRESS_SPACE_IO, bmdma_map);
> +    d->type = IDE_TYPE_PIIX4;
> +    pci_config_set_vendor_id(d->dev.config, PCI_VENDOR_ID_INTEL);
> +    pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82371AB);
> +    return pci_piix_ide_initfn(d);
> +}
>  
> -    ide_init2(&d->bus[0], hd_table[0], hd_table[1], isa_reserve_irq(14));
> -    ide_init2(&d->bus[1], hd_table[2], hd_table[3], isa_reserve_irq(15));
> -    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
> -    ide_init_ioport(&d->bus[1], 0x170, 0x376);

This part of the diff is a bit confusing because besides qdevification
it also factors out common parts of piix3 and piix4 into
pci_piix_ide_initfn().  Good move, but would be easier to review as a
separate commit.

> +/* hd_table must contain 4 block drivers */
> +/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> +void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
>  
> -    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
> +    dev = pci_create_simple(bus, devfn, "PIIX3 IDE");
> +    pci_ide_create_devs(dev, hd_table);
> +}
> +
> +/* hd_table must contain 4 block drivers */
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> +void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_simple(bus, devfn, "PIIX4 IDE");
> +    pci_ide_create_devs(dev, hd_table);
>  }
>  
> +static PCIDeviceInfo piix_ide_info[] = {
> +    {
> +        .qdev.name    = "PIIX3 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_piix3_ide_initfn,
> +    },{
> +        .qdev.name    = "PIIX4 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_piix4_ide_initfn,
> +    },{
> +        .qdev.name    = "CMD646 IDE",
> +        .qdev.size    = sizeof(PCIIDEState),
> +        .init         = pci_cmd646_ide_initfn,
> +        .qdev.props   = (Property[]) {
> +            DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
> +    },{
> +        /* end of list */
> +    }
> +};
> +
> +static void piix_ide_register(void)
> +{
> +    pci_qdev_register_many(piix_ide_info);
> +}
> +device_init(piix_ide_register);