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

[Bernhard Beschow]

On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 28/05/2022 20:20, 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                | 25 ++++++++++++++-----------
> >   hw/mips/malta.c               |  3 ++-
> >   include/hw/southbridge/piix.h |  2 +-
> >   3 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 96df21a610..217989227d 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -49,6 +49,7 @@ struct PIIX4State {
> >       RTCState rtc;
> >       PCIIDEState ide;
> >       UHCIState uhci;
> > +    PIIX4PMState pm;
> >       /* Reset Control Register */
> >       MemoryRegion rcr_mem;
> >       uint8_t rcr;
> > @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error
> **errp)
> >           return;
> >       }
> >
> > +    /* ACPI controller */
> > +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
> > +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> > +        return;
> > +    }
> > +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
> > +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm),
> "i2c");
>
> Now that the PM device is QOMified you don't actually need this alias
> anymore (see
> below). In general aliasing parts of contained devices onto the container
> isn't
> recommended, since it starts to blur the line between individual devices
> and then you
> find some wiring gets done to the container, some directly to the
> contained device
> and then it starts to get confusing quite quickly.
>
> >       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
> PIIX_NUM_PIRQS);
> >   }
> >
> > @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
> >       object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
> >       object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
> >       object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
> > +
> > +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
> > +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
> > +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
> >   }
> >
> >   static void piix4_class_init(ObjectClass *klass, void *data)
> > @@ -312,7 +325,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;
> > @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus
> **smbus)
> >                                             TYPE_PIIX4_PCI_DEVICE);
> >       dev = DEVICE(pci);
> >
> > -    if (smbus) {
> > -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
> > -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
> > -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
> > -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
> > -        qdev_connect_gpio_out(DEVICE(pci), 0,
> > -                              qdev_get_gpio_in_named(dev, "isa", 9));
> > -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
> > -    }
> > -
> >       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"));
>
> It should now be possible to do something like this:
>
>      pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>      smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));
>
> whereby we grab the reference to the PIIX4_PM device by resolving the "pm"
> child
> object and then use that to obtain the reference to smbus.
>

Imagining the container device to be an abstraction layer for the
contained functionality I actually prefer the alias. Having it one
doesn't need to know that there is an internal device named "pm" and
one doesn't need to care about how many levels deep it is buried
inside the implementation. This allows for further refactoring the
PIIX4 without breaking its contract to its users.

Also, this reflects how the real hardware works: The Malta board can
wire up the PIIX4 southbridge without worrying about its inner
details. One can look into the datasheets, see the exposed interfaces
and pins, and connect them. By QOM'ifying PIIX4 we now have the
opportunity to mirror the real hardware by exposing it to the Malta
board as a black box which exposes dedicated pins and buses.

Looking further into the QEMU code, there is even the following
comment in sabrelite.c:
        /*
         * TODO: Ideally we would expose the chip select and spi bus on the
         * SoC object using alias properties; then we would not need to
         * directly access the underlying spi device object.
         */
To me this comment seems that the authors think in a similar way.

What do you think?

Best regards,
Bernhard





> >       /* 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 0a2ef0c7b6..e1f5d6d5c8 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -70,6 +70,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.
>