[03/11] hw/isa/piix3: QOM'ify USB controller creation

Commit Message

Bernhard Beschow July 13, 2022, 8:17 a.m. UTC

The USB controller is an integral part of PIIX3 (function 2). So create
it as part of the southbridge.

Note that the USB function is optional in QEMU. This is why it gets
unparented if it is disabled, otherwiese QEMU will abort with:

  src/hw/core/qdev.c:357: qdev_assert_realized_properly_cb: Assertion `dev->realized' failed

Signed-off-by: Bernhard Beschow <[e-mail]>
---
 hw/i386/pc_piix.c             |  6 ++----
 hw/isa/piix3.c                | 26 ++++++++++++++++++++++++++
 include/hw/southbridge/piix.h |  5 +++++
 3 files changed, 33 insertions(+), 4 deletions(-)

Comments

Peter Maydell July 14, 2022, 3:01 p.m. UTC | #1

On Wed, 13 Jul 2022 at 09:49, Bernhard Beschow <[e-mail]> wrote:
>
> The USB controller is an integral part of PIIX3 (function 2). So create
> it as part of the southbridge.
>
> Note that the USB function is optional in QEMU. This is why it gets
> unparented if it is disabled, otherwiese QEMU will abort with:
>
>   src/hw/core/qdev.c:357: qdev_assert_realized_properly_cb: Assertion `dev->realized' failed

I would suggest doing the object_initialize_child() in realize when
we know whether usb is enabled or not, rather than always
initializing it and then unparenting it later. That's the way
I've handled "some property value determines whether or not the
device has a child object" in other places.

thanks
-- PMM

Patch

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f129da29ac..96dc0db729 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -210,6 +210,8 @@  static void pc_init1(MachineState *machine,
         pcms->bus = pci_bus;
 
         pci_dev = pci_new_multifunction(-1, true, type);
+        object_property_set_bool(OBJECT(pci_dev), "has-usb",
+                                 machine_usb(machine), &error_abort);
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
         piix3 = PIIX3_PCI_DEVICE(pci_dev);
         piix3->pic = x86ms->gsi;
@@ -281,10 +283,6 @@  static void pc_init1(MachineState *machine,
     }
 #endif
 
-    if (pcmc->pci_enabled && machine_usb(machine)) {
-        pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
-    }
-
     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
         PCIDevice *piix4_pm;
 
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 44a9998752..dd512cca84 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -27,6 +27,7 @@ 
 #include "qapi/error.h"
 #include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
+#include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
 #include "hw/xen/xen.h"
@@ -296,6 +297,7 @@  static const MemoryRegionOps rcr_ops = {
 static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
 
     isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
@@ -312,6 +314,16 @@  static void pci_piix3_realize(PCIDevice *dev, Error **errp)
     qemu_register_reset(piix3_reset, d);
 
     i8257_dma_init(isa_bus, 0);
+
+    /* USB */
+    if (d->has_usb) {
+        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+            return;
+        }
+    } else {
+        object_unparent(OBJECT(&d->uhci));
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -327,6 +339,18 @@  static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
     }
 }
 
+static void pci_piix3_init(Object *obj)
+{
+    PIIX3State *d = PIIX3_PCI_DEVICE(obj);
+
+    object_initialize_child(obj, "uhci", &d->uhci, "piix3-usb-uhci");
+}
+
+static Property pci_piix3_props[] = {
+    DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pci_piix3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -345,12 +369,14 @@  static void pci_piix3_class_init(ObjectClass *klass, void *data)
      * pc_piix.c's pc_init1()
      */
     dc->user_creatable = false;
+    device_class_set_props(dc, pci_piix3_props);
     adevc->build_dev_aml = build_pci_isa_aml;
 }
 
 static const TypeInfo piix3_pci_type_info = {
     .name = TYPE_PIIX3_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
+    .instance_init  = pci_piix3_init,
     .instance_size = sizeof(PIIX3State),
     .abstract = true,
     .class_init = pci_piix3_class_init,
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 2693778b23..115311d932 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -14,6 +14,7 @@ 
 
 #include "hw/pci/pci.h"
 #include "qom/object.h"
+#include "hw/usb/hcd-uhci.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
@@ -52,11 +53,15 @@  struct PIIXState {
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
+    UHCIState uhci;
+
     /* Reset Control Register contents */
     uint8_t rcr;
 
     /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
     MemoryRegion rcr_mem;
+
+    bool has_usb;
 };
 typedef struct PIIXState PIIX3State;