[PATCH 00/11] QOM'ify PIIX3 southbridge

[PATCH 00/11] QOM'ify PIIX3 southbridge

[Bernhard Beschow]

Similar to PIIX4 this series QOM'ifies internal device creation for PIIX3.
This reduces the delta between the implementations of PIIX3 and PIIX4 and
therefore might allow to merge both implementations in the future.

There were two challenges in this series:

First, QEMU considers the ACPI and USB functions to be optional in PIIX3.
When instantiating those with object_initialize_child(), they need to be
unparented in the realize function to prevent an assertion (see respective
commit messages).

Second, the PIC used to be instantiated outside of the southbridge while
some sub functions require a PIC with populated qemu_irqs. This has been
solved by introducing a proxy PIC which furthermore allows PIIX3 to be
agnostic towards the virtualization technology used (KVM, TCG, Xen).

Testing done:
* make check
* make check-avocado
* Boot live CD:
  * qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso
  * qemu-system-x86_64 -M q35 -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso

Bernhard Beschow (11):
  hw/i386/pc: QOM'ify DMA creation
  hw/i386/pc_piix: Allow for setting properties before realizing PIIX3
    southbridge
  hw/isa/piix3: QOM'ify USB controller creation
  hw/isa/piix3: QOM'ify ACPI controller creation
  hw/i386/pc: QOM'ify RTC creation
  hw/i386/pc: No need for rtc_state to be an out-parameter
  hw/intc/i8259: Introduce i8259 proxy "isa-pic"
  hw/isa/piix3: QOM'ify ISA PIC creation
  hw/isa/piix3: QOM'ify IDE controller creation
  hw/isa/piix3: Wire up ACPI interrupt internally
  hw/isa/piix3: Remove extra ';' outside of functions

 hw/i386/Kconfig               |  1 -
 hw/i386/pc.c                  | 17 ++++---
 hw/i386/pc_piix.c             | 70 ++++++++++++++++-------------
 hw/i386/pc_q35.c              |  3 +-
 hw/intc/i8259.c               | 27 +++++++++++
 hw/isa/Kconfig                |  1 +
 hw/isa/lpc_ich9.c             | 11 +++++
 hw/isa/piix3.c                | 84 ++++++++++++++++++++++++++++++++---
 include/hw/i386/ich9.h        |  2 +
 include/hw/i386/pc.h          |  2 +-
 include/hw/intc/i8259.h       | 14 ++++++
 include/hw/southbridge/piix.h | 16 ++++++-
 12 files changed, 201 insertions(+), 47 deletions(-)

-- 
2.37.1

[PATCH 01/11] hw/i386/pc: QOM'ify DMA creation

[Bernhard Beschow]

Just like in the real hardware, create the DMA in the southbridges.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c      | 3 ---
 hw/i386/pc_piix.c | 2 ++
 hw/isa/lpc_ich9.c | 3 +++
 hw/isa/piix3.c    | 9 +++++++--
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 774cb2bf07..c3602d166d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -47,7 +47,6 @@
 #include "multiboot.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/intc/i8259.h"
-#include "hw/dma/i8257.h"
 #include "hw/timer/i8254.h"
 #include "hw/input/i8042.h"
 #include "hw/irq.h"
@@ -1200,8 +1199,6 @@ void pc_basic_device_init(struct PCMachineState *pcms,
         pcspk_init(pcms->pcspk, isa_bus, pit);
     }
 
-    i8257_dma_init(isa_bus, 0);
-
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
                     pcms->vmport != ON_OFF_AUTO_ON);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a234989ac3..7ad677e967 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -26,6 +26,7 @@
 #include CONFIG_DEVICES
 
 #include "qemu/units.h"
+#include "hw/dma/i8257.h"
 #include "hw/loader.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/pc.h"
@@ -217,6 +218,7 @@ static void pc_init1(MachineState *machine,
         pci_bus = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
                               &error_abort);
+        i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
     }
     isa_bus_irqs(isa_bus, x86ms->gsi);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 4553b5925b..8694e58b21 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -34,6 +34,7 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qemu/range.h"
+#include "hw/dma/i8257.h"
 #include "hw/isa/isa.h"
 #include "migration/vmstate.h"
 #include "hw/irq.h"
@@ -722,6 +723,8 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     qdev_init_gpio_out_named(dev, lpc->gsi, ICH9_GPIO_GSI, GSI_NUM_PINS);
 
     isa_bus_irqs(isa_bus, lpc->gsi);
+
+    i8257_dma_init(isa_bus, 0);
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 48f9ab1096..44a9998752 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/range.h"
 #include "qapi/error.h"
+#include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
@@ -295,9 +296,11 @@ static const MemoryRegionOps rcr_ops = {
 static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    ISABus *isa_bus;
 
-    if (!isa_bus_new(DEVICE(d), get_system_memory(),
-                     pci_address_space_io(dev), errp)) {
+    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
         return;
     }
 
@@ -307,6 +310,8 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
                                         PIIX_RCR_IOPORT, &d->rcr_mem, 1);
 
     qemu_register_reset(piix3_reset, d);
+
+    i8257_dma_init(isa_bus, 0);
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
-- 
2.37.1

[PATCH 02/11] hw/i386/pc_piix: Allow for setting properties before realizing PIIX3 southbridge

[Bernhard Beschow]

The next patches will need to take advantage of it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7ad677e967..f129da29ac 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -209,7 +209,8 @@ static void pc_init1(MachineState *machine,
                               pci_memory, ram_memory);
         pcms->bus = pci_bus;
 
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+        pci_dev = pci_new_multifunction(-1, true, type);
+        pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
         piix3 = PIIX3_PCI_DEVICE(pci_dev);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
-- 
2.37.1

Re: [PATCH 02/11] hw/i386/pc_piix: Allow for setting properties before realizing PIIX3 southbridge

[Peter Maydell]

On Wed, 13 Jul 2022 at 09:36, Bernhard Beschow <shentey@gmail.com> wrote:
>
> The next patches will need to take advantage of it.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Bernhard Beschow]

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 <shentey@gmail.com>
---
 hw/i386/pc_piix.c             |  6 ++----
 hw/isa/piix3.c                | 26 ++++++++++++++++++++++++++
 include/hw/southbridge/piix.h |  5 +++++
 3 files changed, 33 insertions(+), 4 deletions(-)

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;
 
-- 
2.37.1

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

[Peter Maydell]

On Wed, 13 Jul 2022 at 09:49, Bernhard Beschow <shentey@gmail.com> 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 04/11] hw/isa/piix3: QOM'ify ACPI controller creation

[Bernhard Beschow]

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

Note that the ACPI 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 <shentey@gmail.com>
---
 hw/i386/pc_piix.c             | 22 ++++++++++++----------
 hw/isa/piix3.c                | 16 ++++++++++++++++
 include/hw/southbridge/piix.h |  3 +++
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 96dc0db729..364c73b1bc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -44,11 +44,11 @@
 #include "sysemu/kvm.h"
 #include "hw/kvm/clock.h"
 #include "hw/sysbus.h"
+#include "hw/i2c/i2c.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/xen/xen-x86.h"
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
-#include "hw/acpi/piix4.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/xen.h"
@@ -83,6 +83,7 @@ static void pc_init1(MachineState *machine,
     MemoryRegion *system_io = get_system_io();
     PCIBus *pci_bus;
     ISABus *isa_bus;
+    Object *piix4_pm;
     int piix3_devfn = -1;
     qemu_irq smi_irq;
     GSIState *gsi_state;
@@ -212,13 +213,21 @@ static void pc_init1(MachineState *machine,
         pci_dev = pci_new_multifunction(-1, true, type);
         object_property_set_bool(OBJECT(pci_dev), "has-usb",
                                  machine_usb(machine), &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "has-acpi",
+                                 x86_machine_is_acpi_enabled(x86ms),
+                                 &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
+                                 x86_machine_is_smm_enabled(x86ms),
+                                 &error_abort);
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
         piix3 = PIIX3_PCI_DEVICE(pci_dev);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+        piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
     } else {
         pci_bus = NULL;
+        piix4_pm = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
                               &error_abort);
         i8257_dma_init(isa_bus, 0);
@@ -283,15 +292,8 @@ static void pc_init1(MachineState *machine,
     }
 #endif
 
-    if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
-        PCIDevice *piix4_pm;
-
+    if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
-        piix4_pm = pci_new(piix3_devfn + 3, TYPE_PIIX4_PM);
-        qdev_prop_set_uint32(DEVICE(piix4_pm), "smb_io_base", 0xb100);
-        qdev_prop_set_bit(DEVICE(piix4_pm), "smm-enabled",
-                          x86_machine_is_smm_enabled(x86ms));
-        pci_realize_and_unref(piix4_pm, pci_bus, &error_fatal);
 
         qdev_connect_gpio_out(DEVICE(piix4_pm), 0, x86ms->gsi[9]);
         qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
@@ -305,7 +307,7 @@ static void pc_init1(MachineState *machine,
                                  object_property_allow_set_link,
                                  OBJ_PROP_LINK_STRONG);
         object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
-                                 OBJECT(piix4_pm), &error_abort);
+                                 piix4_pm, &error_abort);
     }
 
     if (machine->nvdimms_state->is_enabled) {
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index dd512cca84..5db0bbf7b6 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -324,6 +324,16 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
     } else {
         object_unparent(OBJECT(&d->uhci));
     }
+
+    /* ACPI */
+    if (d->has_acpi) {
+        qdev_prop_set_int32(DEVICE(&d->pm), "addr", dev->devfn + 3);
+        if (!qdev_realize(DEVICE(&d->pm), BUS(pci_bus), errp)) {
+            return;
+        }
+    } else {
+        object_unparent(OBJECT(&d->pm));
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -344,9 +354,15 @@ static void pci_piix3_init(Object *obj)
     PIIX3State *d = PIIX3_PCI_DEVICE(obj);
 
     object_initialize_child(obj, "uhci", &d->uhci, "piix3-usb-uhci");
+
+    object_initialize_child(obj, "pm", &d->pm, TYPE_PIIX4_PM);
+    qdev_prop_set_uint32(DEVICE(&d->pm), "smb_io_base", 0xb100);
+    object_property_add_alias(obj, "smm-enabled",
+                              OBJECT(&d->pm), "smm-enabled");
 }
 
 static Property pci_piix3_props[] = {
+    DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
     DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 115311d932..ee847cb4f2 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/acpi/piix4.h"
 #include "hw/usb/hcd-uhci.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
@@ -54,6 +55,7 @@ struct PIIXState {
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
     UHCIState uhci;
+    PIIX4PMState pm;
 
     /* Reset Control Register contents */
     uint8_t rcr;
@@ -61,6 +63,7 @@ struct PIIXState {
     /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
     MemoryRegion rcr_mem;
 
+    bool has_acpi;
     bool has_usb;
 };
 typedef struct PIIXState PIIX3State;
-- 
2.37.1

[PATCH 05/11] hw/i386/pc: QOM'ify RTC creation

[Bernhard Beschow]

Just like in the real hardware, create the RTC in the southbridges.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c                  | 12 ++++++++++--
 hw/i386/pc_piix.c             |  8 ++++++++
 hw/i386/pc_q35.c              |  1 +
 hw/isa/lpc_ich9.c             |  8 ++++++++
 hw/isa/piix3.c                |  7 +++++++
 include/hw/i386/ich9.h        |  2 ++
 include/hw/southbridge/piix.h |  2 ++
 7 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c3602d166d..eba1c98b5a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1181,9 +1181,17 @@ void pc_basic_device_init(struct PCMachineState *pcms,
         pit_alt_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_PIT_INT);
         rtc_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_RTC_INT);
     }
-    *rtc_state = mc146818_rtc_init(isa_bus, 2000, rtc_irq);
 
-    qemu_register_boot_set(pc_boot_set, *rtc_state);
+    if (rtc_irq) {
+        qdev_connect_gpio_out(DEVICE(*rtc_state), 0, rtc_irq);
+    } else {
+        uint32_t irq = object_property_get_uint(OBJECT(*rtc_state),
+                                                "irq",
+                                                &error_fatal);
+        isa_connect_gpio_out(*rtc_state, 0, irq);
+    }
+
+    qemu_register_boot_set(pc_boot_set, rtc_state);
 
     if (!xen_enabled() &&
         (x86ms->pit == ON_OFF_AUTO_AUTO || x86ms->pit == ON_OFF_AUTO_ON)) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 364c73b1bc..52c550f8b8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -32,6 +32,7 @@
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
 #include "hw/pci-host/i440fx.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "hw/southbridge/piix.h"
 #include "hw/display/ramfb.h"
 #include "hw/firmware/smbios.h"
@@ -224,12 +225,19 @@ static void pc_init1(MachineState *machine,
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+        rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
+                                                             "rtc"));
         piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
     } else {
         pci_bus = NULL;
         piix4_pm = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
                               &error_abort);
+
+        rtc_state = isa_new(TYPE_MC146818_RTC);
+        qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
+        isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
+
         i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index f96cbd04e2..d850313180 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -230,6 +230,7 @@ static void pc_q35_init(MachineState *machine)
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
                                           TYPE_ICH9_LPC_DEVICE);
+    rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 8694e58b21..0051fa66ab 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -660,6 +660,8 @@ static void ich9_lpc_initfn(Object *obj)
     static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
     static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
 
+    object_initialize_child(obj, "rtc", &lpc->rtc, TYPE_MC146818_RTC);
+
     object_property_add_uint8_ptr(obj, ACPI_PM_PROP_SCI_INT,
                                   &lpc->sci_gsi, OBJ_PROP_FLAG_READ);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
@@ -725,6 +727,12 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, lpc->gsi);
 
     i8257_dma_init(isa_bus, 0);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&lpc->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&lpc->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 5db0bbf7b6..afd36178dd 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -315,6 +315,12 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 
     i8257_dma_init(isa_bus, 0);
 
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&d->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
+
     /* USB */
     if (d->has_usb) {
         qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
@@ -353,6 +359,7 @@ static void pci_piix3_init(Object *obj)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(obj);
 
+    object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "uhci", &d->uhci, "piix3-usb-uhci");
 
     object_initialize_child(obj, "pm", &d->pm, TYPE_PIIX4_PM);
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 23ee8e371b..672efc6bce 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -11,6 +11,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "qom/object.h"
 
 void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
@@ -39,6 +40,7 @@ struct ICH9LPCState {
     */
     uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
 
+    RTCState rtc;
     APMState apm;
     ICH9LPCPMRegs pm;
     uint32_t sci_level; /* track sci level */
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index ee847cb4f2..15b05cfc93 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -15,6 +15,7 @@
 #include "hw/pci/pci.h"
 #include "qom/object.h"
 #include "hw/acpi/piix4.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
@@ -54,6 +55,7 @@ struct PIIXState {
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
+    RTCState rtc;
     UHCIState uhci;
     PIIX4PMState pm;
 
-- 
2.37.1

[PATCH 06/11] hw/i386/pc: No need for rtc_state to be an out-parameter

[Bernhard Beschow]

Now that the RTC is created as part of the southbridges it doesn't need
to be an out-parameter any longer.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc.c         | 8 ++++----
 hw/i386/pc_piix.c    | 2 +-
 hw/i386/pc_q35.c     | 2 +-
 include/hw/i386/pc.h | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index eba1c98b5a..886c6b451a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1128,7 +1128,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
 
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice **rtc_state,
+                          ISADevice *rtc_state,
                           bool create_fdctrl,
                           uint32_t hpet_irqs)
 {
@@ -1183,12 +1183,12 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     }
 
     if (rtc_irq) {
-        qdev_connect_gpio_out(DEVICE(*rtc_state), 0, rtc_irq);
+        qdev_connect_gpio_out(DEVICE(rtc_state), 0, rtc_irq);
     } else {
-        uint32_t irq = object_property_get_uint(OBJECT(*rtc_state),
+        uint32_t irq = object_property_get_uint(OBJECT(rtc_state),
                                                 "irq",
                                                 &error_fatal);
-        isa_connect_gpio_out(*rtc_state, 0, irq);
+        isa_connect_gpio_out(rtc_state, 0, irq);
     }
 
     qemu_register_boot_set(pc_boot_set, rtc_state);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 52c550f8b8..0f6cdc5bc4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -263,7 +263,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, true,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
                          0x4);
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d850313180..15b8b814c3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -284,7 +284,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
                          0xff0104);
 
     /* connect pm stuff to lpc */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b7735dccfc..d1fd8969a0 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -164,7 +164,7 @@ uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice **rtc_state,
+                          ISADevice *rtc_state,
                           bool create_fdctrl,
                           uint32_t hpet_irqs);
 void pc_cmos_init(PCMachineState *pcms,
-- 
2.37.1

Re: [PATCH 06/11] hw/i386/pc: No need for rtc_state to be an out-parameter

[Peter Maydell]

On Wed, 13 Jul 2022 at 09:37, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Now that the RTC is created as part of the southbridges it doesn't need
> to be an out-parameter any longer.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 07/11] hw/intc/i8259: Introduce i8259 proxy "isa-pic"

[Bernhard Beschow]

Having an i8259 proxy allows for ISA PICs to be created and wired up in
southbridges. This is especially interesting for PIIX3 for two reasons:
First, the southbridge doesn't need to care about the virtualization
technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
attached) and out-IRQs (which will trigger the IRQs of the respective
virtzalization technology) are separated. Second, since the in-IRQs are
populated with fully initialized qemu_irq's, they can already be wired
up inside PIIX3.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/intc/i8259.c         | 27 +++++++++++++++++++++++++++
 include/hw/intc/i8259.h | 14 ++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index cc4e21ffec..531f6cca53 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -458,9 +458,36 @@ static const TypeInfo i8259_info = {
     .class_size = sizeof(PICClass),
 };
 
+static void isapic_set_irq(void *opaque, int irq, int level)
+{
+    ISAPICState *s = opaque;
+
+    qemu_set_irq(s->out_irqs[irq], level);
+}
+
+static void isapic_init(Object *obj)
+{
+    ISAPICState *s = ISA_PIC(obj);
+
+    qdev_init_gpio_in(DEVICE(s), isapic_set_irq, ISA_NUM_IRQS);
+    qdev_init_gpio_out(DEVICE(s), s->out_irqs, ISA_NUM_IRQS);
+
+    for (int i = 0; i < ISA_NUM_IRQS; ++i) {
+        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
+    }
+}
+
+static const TypeInfo isapic_info = {
+    .name          = TYPE_ISA_PIC,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(ISAPICState),
+    .instance_init = isapic_init,
+};
+
 static void pic_register_types(void)
 {
     type_register_static(&i8259_info);
+    type_register_static(&isapic_info);
 }
 
 type_init(pic_register_types)
diff --git a/include/hw/intc/i8259.h b/include/hw/intc/i8259.h
index e2b1e8c59a..0246ab6ac6 100644
--- a/include/hw/intc/i8259.h
+++ b/include/hw/intc/i8259.h
@@ -1,6 +1,20 @@
 #ifndef HW_I8259_H
 #define HW_I8259_H
 
+#include "qom/object.h"
+#include "hw/isa/isa.h"
+#include "qemu/typedefs.h"
+
+#define TYPE_ISA_PIC "isa-pic"
+OBJECT_DECLARE_SIMPLE_TYPE(ISAPICState, ISA_PIC)
+
+struct ISAPICState {
+    ISADevice parent_obj;
+
+    qemu_irq in_irqs[ISA_NUM_IRQS];
+    qemu_irq out_irqs[ISA_NUM_IRQS];
+};
+
 /* i8259.c */
 
 extern DeviceState *isa_pic;
-- 
2.37.1

[PATCH 08/11] hw/isa/piix3: QOM'ify ISA PIC creation

[Bernhard Beschow]

Use the newly introduced i8259 proxy "isa-pic" which allows for wiring
up devices in the southbridge where the virtualization technology used
(KVM, TCG, Xen) is not yet known.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c             | 15 +++++++++------
 hw/isa/piix3.c                | 10 +++++++++-
 include/hw/southbridge/piix.h |  4 ++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0f6cdc5bc4..4ce215a212 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -198,10 +198,11 @@ static void pc_init1(MachineState *machine,
     gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
 
     if (pcmc->pci_enabled) {
-        PIIX3State *piix3;
+        DeviceState *dev;
         PCIDevice *pci_dev;
         const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
                                          : TYPE_PIIX3_DEVICE;
+        int i;
 
         pci_bus = i440fx_init(host_type,
                               pci_type,
@@ -221,10 +222,12 @@ static void pc_init1(MachineState *machine,
                                  x86_machine_is_smm_enabled(x86ms),
                                  &error_abort);
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
-        piix3->pic = x86ms->gsi;
-        piix3_devfn = piix3->dev.devfn;
-        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
+        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "pic"));
+        for (i = 0; i < ISA_NUM_IRQS; ++i) {
+            qdev_connect_gpio_out(dev, i, x86ms->gsi[i]);
+        }
+        piix3_devfn = pci_dev->devfn;
+        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
         rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
                                                              "rtc"));
         piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
@@ -233,6 +236,7 @@ static void pc_init1(MachineState *machine,
         piix4_pm = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
                               &error_abort);
+        isa_bus_irqs(isa_bus, x86ms->gsi);
 
         rtc_state = isa_new(TYPE_MC146818_RTC);
         qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
@@ -241,7 +245,6 @@ static void pc_init1(MachineState *machine,
         i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
     }
-    isa_bus_irqs(isa_bus, x86ms->gsi);
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index afd36178dd..7bf3488f76 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -41,7 +41,7 @@
 
 static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
-    qemu_set_irq(piix3->pic[pic_irq],
+    qemu_set_irq(piix3->pic.in_irqs[pic_irq],
                  !!(piix3->pic_levels &
                     (((1ULL << PIIX_NUM_PIRQS) - 1) <<
                      (pic_irq * PIIX_NUM_PIRQS))));
@@ -306,6 +306,13 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
+    /* PIC */
+    if (!qdev_realize(DEVICE(&d->pic), BUS(isa_bus), errp)) {
+        return;
+    }
+
+    isa_bus_irqs(isa_bus, d->pic.in_irqs);
+
     memory_region_init_io(&d->rcr_mem, OBJECT(dev), &rcr_ops, d,
                           "piix3-reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
@@ -359,6 +366,7 @@ static void pci_piix3_init(Object *obj)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(obj);
 
+    object_initialize_child(obj, "pic", &d->pic, TYPE_ISA_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "uhci", &d->uhci, "piix3-usb-uhci");
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 15b05cfc93..cfe155ce07 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -15,6 +15,7 @@
 #include "hw/pci/pci.h"
 #include "qom/object.h"
 #include "hw/acpi/piix4.h"
+#include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
 
@@ -50,11 +51,10 @@ struct PIIXState {
 #endif
     uint64_t pic_levels;
 
-    qemu_irq *pic;
-
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
+    ISAPICState pic;
     RTCState rtc;
     UHCIState uhci;
     PIIX4PMState pm;
-- 
2.37.1

[PATCH 09/11] hw/isa/piix3: QOM'ify IDE controller creation

[Bernhard Beschow]

Now that PIIX3 contains the new isa-pic, it is possible to instantiate
PIIX3 IDE in the PIIX3 southbridge. PIIX3 IDE wires up its interrupts to
the ISA bus in its realize method which requires the interrupt
controller to provide fully populated qemu_irqs. This is the case for
isa-pic even though the virtualization technology not known yet.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/Kconfig               |  1 -
 hw/i386/pc_piix.c             | 13 +++++--------
 hw/isa/Kconfig                |  1 +
 hw/isa/piix3.c                |  8 ++++++++
 include/hw/southbridge/piix.h |  2 ++
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..dd247f215c 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -74,7 +74,6 @@ config I440FX
     select ACPI_SMBUS
     select PCI_I440FX
     select PIIX3
-    select IDE_PIIX
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4ce215a212..f843a73d90 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -85,7 +85,6 @@ static void pc_init1(MachineState *machine,
     PCIBus *pci_bus;
     ISABus *isa_bus;
     Object *piix4_pm;
-    int piix3_devfn = -1;
     qemu_irq smi_irq;
     GSIState *gsi_state;
     BusState *idebus[MAX_IDE_BUS];
@@ -226,11 +225,13 @@ static void pc_init1(MachineState *machine,
         for (i = 0; i < ISA_NUM_IRQS; ++i) {
             qdev_connect_gpio_out(dev, i, x86ms->gsi[i]);
         }
-        piix3_devfn = pci_dev->devfn;
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
         rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
                                                              "rtc"));
         piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
+        dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
+        idebus[0] = qdev_get_child_bus(dev, "ide.0");
+        idebus[1] = qdev_get_child_bus(dev, "ide.1");
     } else {
         pci_bus = NULL;
         piix4_pm = NULL;
@@ -244,6 +245,8 @@ static void pc_init1(MachineState *machine,
 
         i8257_dma_init(isa_bus, 0);
         pcms->hpet_enabled = false;
+        idebus[0] = NULL;
+        idebus[1] = NULL;
     }
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
@@ -272,12 +275,6 @@ static void pc_init1(MachineState *machine,
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
     if (pcmc->pci_enabled) {
-        PCIDevice *dev;
-
-        dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
-        pci_ide_create_devs(dev);
-        idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
-        idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
     }
 #ifdef CONFIG_IDE_ISA
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index d42143a991..808b9a30af 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -34,6 +34,7 @@ config PC87312
 config PIIX3
     bool
     select ISA_BUS
+    select IDE_PIIX
 
 config PIIX4
     bool
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 7bf3488f76..fd9c8f853a 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -328,6 +328,13 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
+    /* IDE */
+    qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
+    if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
+        return;
+    }
+    pci_ide_create_devs(PCI_DEVICE(&d->ide));
+
     /* USB */
     if (d->has_usb) {
         qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
@@ -368,6 +375,7 @@ static void pci_piix3_init(Object *obj)
 
     object_initialize_child(obj, "pic", &d->pic, TYPE_ISA_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "ide", &d->ide, "piix3-ide");
     object_initialize_child(obj, "uhci", &d->uhci, "piix3-usb-uhci");
 
     object_initialize_child(obj, "pm", &d->pm, TYPE_PIIX4_PM);
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index cfe155ce07..41983dfa6d 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -15,6 +15,7 @@
 #include "hw/pci/pci.h"
 #include "qom/object.h"
 #include "hw/acpi/piix4.h"
+#include "hw/ide/pci.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
@@ -56,6 +57,7 @@ struct PIIXState {
 
     ISAPICState pic;
     RTCState rtc;
+    PCIIDEState ide;
     UHCIState uhci;
     PIIX4PMState pm;
 
-- 
2.37.1

[PATCH 10/11] hw/isa/piix3: Wire up ACPI interrupt internally

[Bernhard Beschow]

Now that PIIX3 has the PIC integrated, the ACPI controller can be wired
up internally.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 1 -
 hw/isa/piix3.c    | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f843a73d90..19c86b68bd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -303,7 +303,6 @@ static void pc_init1(MachineState *machine,
     if (piix4_pm) {
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
 
-        qdev_connect_gpio_out(DEVICE(piix4_pm), 0, x86ms->gsi[9]);
         qdev_connect_gpio_out_named(DEVICE(piix4_pm), "smi-irq", 0, smi_irq);
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
         /* TODO: Populate SPD eeprom data.  */
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index fd9c8f853a..56a741c192 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -351,6 +351,8 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
         if (!qdev_realize(DEVICE(&d->pm), BUS(pci_bus), errp)) {
             return;
         }
+        qdev_connect_gpio_out(DEVICE(&d->pm), 0,
+                              qdev_get_gpio_in(DEVICE(&d->pic), 9));
     } else {
         object_unparent(OBJECT(&d->pm));
     }
-- 
2.37.1

[PATCH 11/11] hw/isa/piix3: Remove extra ';' outside of functions

[Bernhard Beschow]

Fixes the "extra-semi" clang-tidy check.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix3.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 56a741c192..3cfd6eafcd 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -442,7 +442,7 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
     pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
                  piix3, PIIX_NUM_PIRQS);
     pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
-};
+}
 
 static void piix3_class_init(ObjectClass *klass, void *data)
 {
@@ -477,7 +477,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
      */
     pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
                  piix3, XEN_PIIX_NUM_PIRQS);
-};
+}
 
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
@@ -485,7 +485,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
     k->config_write = piix3_write_config_xen;
     k->realize = piix3_xen_realize;
-};
+}
 
 static const TypeInfo piix3_xen_info = {
     .name          = TYPE_PIIX3_XEN_DEVICE,
-- 
2.37.1

Re: [PATCH 11/11] hw/isa/piix3: Remove extra '; ' outside of functions

[Peter Maydell]

On Wed, 13 Jul 2022 at 09:52, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Fixes the "extra-semi" clang-tidy check.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 00/11] QOM'ify PIIX3 southbridge

[Michael S. Tsirkin]

On Wed, Jul 13, 2022 at 10:17:24AM +0200, Bernhard Beschow wrote:
> Similar to PIIX4 this series QOM'ifies internal device creation for PIIX3.
> This reduces the delta between the implementations of PIIX3 and PIIX4 and
> therefore might allow to merge both implementations in the future.
> 
> There were two challenges in this series:
> 
> First, QEMU considers the ACPI and USB functions to be optional in PIIX3.
> When instantiating those with object_initialize_child(), they need to be
> unparented in the realize function to prevent an assertion (see respective
> commit messages).
> 
> Second, the PIC used to be instantiated outside of the southbridge while
> some sub functions require a PIC with populated qemu_irqs. This has been
> solved by introducing a proxy PIC which furthermore allows PIIX3 to be
> agnostic towards the virtualization technology used (KVM, TCG, Xen).

Thanks!
I think it's best to merge this after the 7.1 release.
I'll tag this but if possible pls also ping me after the release
to make sure I don't forget. Thanks!

> Testing done:
> * make check
> * make check-avocado
> * Boot live CD:
>   * qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso
>   * qemu-system-x86_64 -M q35 -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso
> 
> Bernhard Beschow (11):
>   hw/i386/pc: QOM'ify DMA creation
>   hw/i386/pc_piix: Allow for setting properties before realizing PIIX3
>     southbridge
>   hw/isa/piix3: QOM'ify USB controller creation
>   hw/isa/piix3: QOM'ify ACPI controller creation
>   hw/i386/pc: QOM'ify RTC creation
>   hw/i386/pc: No need for rtc_state to be an out-parameter
>   hw/intc/i8259: Introduce i8259 proxy "isa-pic"
>   hw/isa/piix3: QOM'ify ISA PIC creation
>   hw/isa/piix3: QOM'ify IDE controller creation
>   hw/isa/piix3: Wire up ACPI interrupt internally
>   hw/isa/piix3: Remove extra ';' outside of functions
> 
>  hw/i386/Kconfig               |  1 -
>  hw/i386/pc.c                  | 17 ++++---
>  hw/i386/pc_piix.c             | 70 ++++++++++++++++-------------
>  hw/i386/pc_q35.c              |  3 +-
>  hw/intc/i8259.c               | 27 +++++++++++
>  hw/isa/Kconfig                |  1 +
>  hw/isa/lpc_ich9.c             | 11 +++++
>  hw/isa/piix3.c                | 84 ++++++++++++++++++++++++++++++++---
>  include/hw/i386/ich9.h        |  2 +
>  include/hw/i386/pc.h          |  2 +-
>  include/hw/intc/i8259.h       | 14 ++++++
>  include/hw/southbridge/piix.h | 16 ++++++-
>  12 files changed, 201 insertions(+), 47 deletions(-)
> 
> -- 
> 2.37.1
> 

Re: [PATCH 00/11] QOM'ify PIIX3 southbridge

[BB]

Am 26. Juli 2022 16:53:03 MESZ schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Wed, Jul 13, 2022 at 10:17:24AM +0200, Bernhard Beschow wrote:
>> Similar to PIIX4 this series QOM'ifies internal device creation for PIIX3.
>> This reduces the delta between the implementations of PIIX3 and PIIX4 and
>> therefore might allow to merge both implementations in the future.
>> 
>> There were two challenges in this series:
>> 
>> First, QEMU considers the ACPI and USB functions to be optional in PIIX3.
>> When instantiating those with object_initialize_child(), they need to be
>> unparented in the realize function to prevent an assertion (see respective
>> commit messages).
>> 
>> Second, the PIC used to be instantiated outside of the southbridge while
>> some sub functions require a PIC with populated qemu_irqs. This has been
>> solved by introducing a proxy PIC which furthermore allows PIIX3 to be
>> agnostic towards the virtualization technology used (KVM, TCG, Xen).
>
>Thanks!
>I think it's best to merge this after the 7.1 release.
>I'll tag this but if possible pls also ping me after the release
>to make sure I don't forget. Thanks!

Sure!
I'm extending the scope of this series to go all the way to consolidate the piix 3 + 4 southbridges which is why I didn't post a v2 yet. The extended series will also address Peter's comments.

Thanks,
Bernhard

P.S.:
I've got a working POC where PIIX4 rather than PIIX3 is used in the "pc" machine which also supports KVM accelleration: https://github.com/shentok/qemu/commits/pc-piix4

>
>> Testing done:
>> * make check
>> * make check-avocado
>> * Boot live CD:
>>   * qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso
>>   * qemu-system-x86_64 -M q35 -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso
>> 
>> Bernhard Beschow (11):
>>   hw/i386/pc: QOM'ify DMA creation
>>   hw/i386/pc_piix: Allow for setting properties before realizing PIIX3
>>     southbridge
>>   hw/isa/piix3: QOM'ify USB controller creation
>>   hw/isa/piix3: QOM'ify ACPI controller creation
>>   hw/i386/pc: QOM'ify RTC creation
>>   hw/i386/pc: No need for rtc_state to be an out-parameter
>>   hw/intc/i8259: Introduce i8259 proxy "isa-pic"
>>   hw/isa/piix3: QOM'ify ISA PIC creation
>>   hw/isa/piix3: QOM'ify IDE controller creation
>>   hw/isa/piix3: Wire up ACPI interrupt internally
>>   hw/isa/piix3: Remove extra ';' outside of functions
>> 
>>  hw/i386/Kconfig               |  1 -
>>  hw/i386/pc.c                  | 17 ++++---
>>  hw/i386/pc_piix.c             | 70 ++++++++++++++++-------------
>>  hw/i386/pc_q35.c              |  3 +-
>>  hw/intc/i8259.c               | 27 +++++++++++
>>  hw/isa/Kconfig                |  1 +
>>  hw/isa/lpc_ich9.c             | 11 +++++
>>  hw/isa/piix3.c                | 84 ++++++++++++++++++++++++++++++++---
>>  include/hw/i386/ich9.h        |  2 +
>>  include/hw/i386/pc.h          |  2 +-
>>  include/hw/intc/i8259.h       | 14 ++++++
>>  include/hw/southbridge/piix.h | 16 ++++++-
>>  12 files changed, 201 insertions(+), 47 deletions(-)
>> 
>> -- 
>> 2.37.1
>> 
>