Re: [PATCH v2 5/6] hw/isa/piix4: QOM'ify PIIX4 PM creation

[Mark Cave-Ayland]

On 25/05/2022 19:09, Mark Cave-Ayland wrote:

> On 22/05/2022 22:24, Bernhard Beschow wrote:
>
> > Just like the real hardware, create the PIIX4 ACPI controller as part of
> > the PIIX4 southbridge. This also mirrors how the IDE and USB functions
> > are already created.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >   hw/isa/piix4.c                | 14 +++++++-------
> >   hw/mips/malta.c               |  3 ++-
> >   include/hw/southbridge/piix.h |  2 +-
> >   3 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 4968c69da9..1645f63450 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -206,6 +206,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
> >       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> >       PCIDevice *pci;
> >       PCIBus *pci_bus = pci_get_bus(dev);
> > +    I2CBus *smbus;
> >       ISABus *isa_bus;
> >       qemu_irq *i8259_out_irq;
> > @@ -252,6 +253,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
> >       /* USB */
> >       pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
> > +    /* ACPI controller */
> > +    smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100, s->isa[9],
> > +                          NULL, 0, NULL);
> > +    object_property_add_const_link(OBJECT(s), "smbus", OBJECT(smbus));
> > +
>
> Interesting hack here to expose the smbus so it is available to qdev_get_child_bus(), 
> but really this is still really working around the fact that piix4_pm_init() itself 
> should be removed first. Once that is done, you can then use a standard QOM pattern 
> to initialise the "internal" PCI devices via object_initialize_child() and realize 
> them in piix4_realize() instead of using pci_create_simple().
>
> Is that something you could take a look at? If not, I may be able to put something 
> together towards the end of the week. Other than that I think the rest of the series 
> looks good.

Hi Bernhard,

I've just sent through the series for eliminating the piix4_pm_init() which should 
allow you to improve the QOMifcation done as part of this series.

A bit of background as to why this is necessary: when building a container device 
such as the PIIX southbridge, there should still be 2 distinct init and realize 
phases. In effect this needs to be done depth-first so when you init the PIIX4 
southbridge, the instance init function should also init the contained PCI devices 
such as IDE and USB. Similarly when the PIIX4 device is realized, its realize 
function should then realize the contained PCI devices using qdev_realize().

This is one of the main reasons why legacy global device init functions aren't 
recommended, since they both init *and* realize the device before returning it which 
immediately breaks this contract.

The normal way to initialise a contained device is to use object_initialize_child() 
in the container's instance init function and to store the the child device instance 
within the container. But what this also means is that you shouldn't use any _new() 
functions like pci_new() or pci_create_simple() to instantiated contained devices in 
a container realize function either. So the next question: how is this done?

Fortunately there is an existing example of this: take a look at how this is 
currently done in hw/pci-host/q35.c's q35_host_initfn() and q35_host_realize() for 
the MCH_PCI_DEVICE device.

I hope this gives you all the information you need to produce a v3 of the series: if 
you have any further questions, do let me know and I will do my best to answer them.

> >       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 
> > PIIX_NUM_PIRQS);
> >   }
> > @@ -301,7 +307,7 @@ static void piix4_register_types(void)
> >   type_init(piix4_register_types)
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> > +DeviceState *piix4_create(PCIBus *pci_bus)
> >   {
> >       PCIDevice *pci;
> >       DeviceState *dev;
> > @@ -311,11 +317,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> >                                             TYPE_PIIX4_PCI_DEVICE);
> >       dev = DEVICE(pci);
> > -    if (smbus) {
> > -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> > -                               qdev_get_gpio_in_named(dev, "isa", 9),
> > -                               NULL, 0, NULL);
> > -    }
> > -
> >       return dev;
> >   }
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index e446b25ad0..b0fc84ccbb 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
> >       empty_slot_init("GT64120", 0, 0x20000000);
> >       /* Southbridge */
> > -    dev = piix4_create(pci_bus, &smbus);
> > +    dev = piix4_create(pci_bus);
> >       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> > +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
> >       /* Interrupt controller */
> >       qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> > diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> > index 0bec7f8ca3..2c21359efa 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -76,6 +76,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> >   PIIX3State *piix3_create(PCIBus *pci_bus);
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> > +DeviceState *piix4_create(PCIBus *pci_bus);
> >   #endif


ATB,

Mark.