Re: [PATCH 3/6] hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> PCI interrupt wiring and device creation (piix4 only) were performed
> in create() functions which are obsolete. Move these tasks into QOM
> functions to modernize the code.
> 
> In order to avoid duplicate checking for xen_enabled() the piix3 realize
> methods are now split.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix3.c | 67 +++++++++++++++++++++++++++++++++-----------------
>   hw/isa/piix4.c | 20 +++++++++------
>   2 files changed, 57 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index 7d69420967..d15117a7c7 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -24,6 +24,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/range.h"
> +#include "qapi/error.h"
>   #include "hw/southbridge/piix.h"
>   #include "hw/irq.h"
>   #include "hw/isa/isa.h"
> @@ -280,7 +281,7 @@ static const MemoryRegionOps rcr_ops = {
>       .endianness = DEVICE_LITTLE_ENDIAN
>   };
>   
> -static void piix3_realize(PCIDevice *dev, Error **errp)
> +static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>   {
>       PIIX3State *d = PIIX3_PCI_DEVICE(dev);
>   
> @@ -305,7 +306,6 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
>       dc->desc        = "ISA bridge";
>       dc->vmsd        = &vmstate_piix3;
>       dc->hotpluggable   = false;
> -    k->realize      = piix3_realize;
>       k->vendor_id    = PCI_VENDOR_ID_INTEL;
>       /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
>       k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
> @@ -329,11 +329,28 @@ static const TypeInfo piix3_pci_type_info = {
>       },
>   };
>   
> +static void piix3_realize(PCIDevice *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
> +    PCIBus *pci_bus = pci_get_bus(dev);
> +
> +    pci_piix3_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
> +                 piix3, PIIX_NUM_PIRQS);
> +    pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
> +};
> +

Oooof. So the reason this looks a bit odd is because we don't have an equivalent of 
device_class_set_parent_realize() for PCIDevice realize(). Having had a look in pci.c 
this looks like a similar approach for handling errors, execpt that here errp is 
accessed directly.

I think this is probably the best we can do for now though. Have you tried forcing 
pci_piix3_realize() to throw an error during testing to make sure this works as expected?

>   static void piix3_class_init(ObjectClass *klass, void *data)
>   {
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
>       k->config_write = piix3_write_config;
> +    k->realize = piix3_realize;
>   }
>   
>   static const TypeInfo piix3_info = {
> @@ -342,11 +359,33 @@ static const TypeInfo piix3_info = {
>       .class_init    = piix3_class_init,
>   };
>   
> +static void piix3_xen_realize(PCIDevice *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
> +    PCIBus *pci_bus = pci_get_bus(dev);
> +
> +    pci_piix3_realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    /*
> +     * Xen supports additional interrupt routes from the PCI devices to
> +     * the IOAPIC: the four pins of each PCI device on the bus are also
> +     * connected to the IOAPIC directly.
> +     * These additional routes can be discovered through ACPI.
> +     */
> +    pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> +                 piix3, XEN_PIIX_NUM_PIRQS);
> +};
> +
>   static void piix3_xen_class_init(ObjectClass *klass, void *data)
>   {
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
>       k->config_write = piix3_write_config_xen;
> +    k->realize = piix3_xen_realize;
>   };
>   
>   static const TypeInfo piix3_xen_info = {
> @@ -368,27 +407,11 @@ PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus)
>   {
>       PIIX3State *piix3;
>       PCIDevice *pci_dev;
> +    const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
> +                                     : TYPE_PIIX3_DEVICE;
>   
> -    /*
> -     * Xen supports additional interrupt routes from the PCI devices to
> -     * the IOAPIC: the four pins of each PCI device on the bus are also
> -     * connected to the IOAPIC directly.
> -     * These additional routes can be discovered through ACPI.
> -     */
> -    if (xen_enabled()) {
> -        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
> -                                                  TYPE_PIIX3_XEN_DEVICE);
> -        piix3 = PIIX3_PCI_DEVICE(pci_dev);
> -        pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> -                     piix3, XEN_PIIX_NUM_PIRQS);
> -    } else {
> -        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
> -                                                  TYPE_PIIX3_DEVICE);
> -        piix3 = PIIX3_PCI_DEVICE(pci_dev);
> -        pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
> -                     piix3, PIIX_NUM_PIRQS);
> -        pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
> -    }
> +    pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
> +    piix3 = PIIX3_PCI_DEVICE(pci_dev);
>       *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
>   
>       return piix3;
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index a223b69e24..134d23aea7 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -204,6 +204,8 @@ static const MemoryRegionOps piix4_rcr_ops = {
>   static void piix4_realize(PCIDevice *dev, Error **errp)
>   {
>       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> +    PCIDevice *pci;
> +    PCIBus *pci_bus = pci_get_bus(dev);
>       ISABus *isa_bus;
>       qemu_irq *i8259_out_irq;
>   
> @@ -242,6 +244,15 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>           return;
>       }
>       s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
> +
> +    /* IDE */
> +    pci = pci_create_simple(pci_bus, dev->devfn + 1, "piix4-ide");
> +    pci_ide_create_devs(pci);
> +
> +    /* USB */
> +    pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
> +
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>   }
>   
>   static void piix4_init(Object *obj)
> @@ -292,7 +303,6 @@ type_init(piix4_register_types)
>   
>   DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>   {
> -    PIIX4State *s;
>       PCIDevice *pci;
>       DeviceState *dev;
>       int devfn = PCI_DEVFN(10, 0);
> @@ -300,22 +310,16 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>       pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
>                                             TYPE_PIIX4_PCI_DEVICE);
>       dev = DEVICE(pci);
> -    s = PIIX4_PCI_DEVICE(pci);
> +
>       if (isa_bus) {
>           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>       }
>   
> -    pci = pci_create_simple(pci_bus, devfn + 1, "piix4-ide");
> -    pci_ide_create_devs(pci);
> -
> -    pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
>       if (smbus) {
>           *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>                                  qdev_get_gpio_in_named(dev, "isa", 9),
>                                  NULL, 0, NULL);
>       }
>   
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
> -
>       return dev;
>   }

As long as the error handling works as required:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.