On Sat, May 21, 2022 at 10:27 AM Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:
> 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 <shen...@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.
>
Yes, I was surprised by this as well. So I took inspiration from
sdhci_common_realize().
>
> 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?
>
I'll post the results in the cover letter of v2.
>
> > 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-ayl...@ilande.co.uk>
>
>
> ATB,
>
> Mark.
>
