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

Mon, 30 May 2022 13:15:49 -0700 

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


On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland <
mark.cave-ayl...@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 <shen...@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.

Reply via email to