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

[Mark Cave-Ayland]

On 29/05/2022 19:05, Bernhard Beschow wrote:

> 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.

It's a bit trickier in this particular case because here the PIIX4 device acts as 
both a container and PCI device instance - and certainly the distinction can be 
blurred when modelling integrated devices.

The reason for leaning towards referencing the "pm" child object directly is because 
a quick glance at the datasheet suggests that the PM functions and smbus are exposed 
via function 3, so it feels that referencing that PCIDevice instance to find the 
smbus is intuitive.

For me using the "pm" child object is a shorter alternative to using 
pci_find_device() to locate the function 3 PCIDevice function on the bus, which is 
effectively what a guest OS would do. Since the PM device sits directly on the PCI 
bus it would never change depth within the PIIX4 container device, and renaming child 
object properties is an extremely rare event (which would also be easily fixable with 
grep).

> 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?

It's difficult for me to know without having much knowledge of ARM devices, but I can 
see how this might be useful given that SoCs can integrate a lot of different devices 
into a single address space. Another thing to note is that the comment above was 
originally written in 2016, and the qdev/QOM APIs (and indeed our knowledge as to how 
best to use them) have improved considerably since then.


ATB,

Mark.