[PATCH v4 00/30] This series consolidates the implementations of the PIIX3 and PIIX4 south

[PATCH v4 00/30] This series consolidates the implementations of the PIIX3 and PIIX4 south

[Bernhard Beschow]

code as possible and to bring both device models to feature parity such that
perhaps PIIX4 can become a drop-in-replacement for PIIX3 in the pc machine. This
could resolve the "Frankenstein" PIIX4-PM problem in PIIX3 discussed on this
list before.

The series is structured as follows: First, PIIX3 is changed to instantiate
internal devices itself, like PIIX4 does already. Second, PIIX3 gets prepared
for the merge with PIIX4 which includes some fixes, cleanups, and renamings.
Third, the same is done for PIIX4. In step four the implementations are merged.
Since some consolidations could be done easier with merged implementations, the
consolidation continues in step five which concludes the series.

One particular challenge in this series was that the PIC of PIIX3 used to be
instantiated outside of the south bridge 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). Due to consolidation PIIX4 gained the proxy PIC as well.

Another challenge was dealing with optional devices where Peter already gave
advice in [1] which this series implements.

Last but not least there might be some opportunity to consolidate VM state
handling, probably by reusing the one from PIIX3. Since I'm not very familiar
with the requirements I didn't touch it so far.

v4:
- Rebase onto "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges"
  since it is already queued via mips-next. This eliminates patches
  'hw/isa/piix3: Prefix pci_slot_get_pirq() with "piix3_"' and 'hw/isa/piix4:
  Prefix pci_slot_get_pirq() with "piix4_"'.
- Squash 'hw/isa/piix: Drop the "3" from the PIIX base class' into
  'hw/isa/piix3: Rename typedef PIIX3State to PIIXState'. I originally only
  split these patches since I wasn't sure whether renaming a type was allowed.
- Add new patch 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is
  created' for forther cleanup of INTx-to-LNKx route decoupling.

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`
* 'qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"`

Based-on: <20221120150550.63059-1-shentey@gmail.com>
          "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges"

v3:
- Introduce one TYPE_ICH9_USB_UHCI(fn) rather than several TYPE_ICH9_USB_UHCIx
  (Philippe)
- Make proxy PIC generic (Philippe)
- Track Malta's PIIX dependencies through KConfig
- Rebase onto Philippe's 'hw/isa/piix4: Remove MIPS Malta specific bits' series [3]
- Also rebase onto latest master to resolve merge conflicts. This required
  copying Philippe's series as first three patches - please ignore.

v2:
- Introduce TYPE_ defines for IDE and USB device models (Mark)
- Omit unexporting of PIIXState (Mark)
- Improve commit message of patch 5 to mention reset triggering through PCI
  configuration space (Mark)
- Move reviewed patches w/o dependencies to the bottom of the series for early
  upstreaming

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg02348.html
[2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03310.html
[3] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg05367.html

Bernhard Beschow (27):
  hw/mips/Kconfig: Track Malta's PIIX dependencies via Kconfig
  hw/usb/hcd-uhci: Introduce TYPE_ defines for device models
  hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is
    created
  hw/i386/pc_piix: Allow for setting properties before realizing PIIX3
    south bridge
  hw/i386/pc: Create RTC controllers in south bridges
  hw/i386/pc: No need for rtc_state to be an out-parameter
  hw/isa/piix3: Create USB controller in host device
  hw/isa/piix3: Create power management controller in host device
  hw/core: Introduce proxy-pic
  hw/isa/piix3: Create Proxy PIC in host device
  hw/isa/piix3: Create IDE controller in host device
  hw/isa/piix3: Wire up ACPI interrupt internally
  hw/isa/piix3: Resolve redundant PIIX_NUM_PIC_IRQS
  hw/isa/piix3: Rename pci_piix3_props for sharing with PIIX4
  hw/isa/piix3: Rename piix3_reset() for sharing with PIIX4
  hw/isa/piix3: Drop the "3" from PIIX base class
  hw/isa/piix4: Make PIIX4's ACPI and USB functions optional
  hw/isa/piix4: Remove unused inbound ISA interrupt lines
  hw/isa/piix4: Use Proxy PIC device
  hw/isa/piix4: Reuse struct PIIXState from PIIX3
  hw/isa/piix4: Rename reset control operations to match PIIX3
  hw/isa/piix3: Merge hw/isa/piix4.c
  hw/isa/piix: Harmonize names of reset control memory regions
  hw/isa/piix: Reuse PIIX3 base class' realize method in PIIX4
  hw/isa/piix: Rename functions to be shared for interrupt triggering
  hw/isa/piix: Consolidate IRQ triggering
  hw/isa/piix: Share PIIX3's base class with PIIX4

Philippe Mathieu-Daudé (3):
  hw/mips/malta: Introduce PIIX4_PCI_DEVFN definition
  hw/mips/malta: Set PIIX4 IRQ routes in embedded bootloader
  hw/isa/piix4: Correct IRQRC[A:D] reset values

 MAINTAINERS                             |   8 +-
 configs/devices/mips-softmmu/common.mak |   2 -
 hw/core/Kconfig                         |   3 +
 hw/core/meson.build                     |   1 +
 hw/core/proxy-pic.c                     |  70 ++++++
 hw/i386/Kconfig                         |   4 +-
 hw/i386/pc.c                            |  16 +-
 hw/i386/pc_piix.c                       |  77 +++---
 hw/i386/pc_q35.c                        |  16 +-
 hw/isa/Kconfig                          |  10 +-
 hw/isa/lpc_ich9.c                       |   8 +
 hw/isa/meson.build                      |   3 +-
 hw/isa/{piix3.c => piix.c}              | 274 ++++++++++++++++-----
 hw/isa/piix4.c                          | 302 ------------------------
 hw/mips/Kconfig                         |   2 +
 hw/mips/malta.c                         |  38 ++-
 hw/usb/hcd-uhci.c                       |  16 +-
 hw/usb/hcd-uhci.h                       |   4 +
 include/hw/core/proxy-pic.h             |  54 +++++
 include/hw/i386/ich9.h                  |   2 +
 include/hw/i386/pc.h                    |   2 +-
 include/hw/southbridge/piix.h           |  30 ++-
 22 files changed, 496 insertions(+), 446 deletions(-)
 create mode 100644 hw/core/proxy-pic.c
 rename hw/isa/{piix3.c => piix.c} (57%)
 delete mode 100644 hw/isa/piix4.c
 create mode 100644 include/hw/core/proxy-pic.h

-- 
2.39.0

[PATCH v4 01/30] hw/mips/malta: Introduce PIIX4_PCI_DEVFN definition

[Bernhard Beschow]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

The PIIX4 PCI-ISA bridge function is always located at 10:0.
Since we want to re-use its address, add the PIIX4_PCI_DEVFN
definition.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221027204720.33611-2-philmd@linaro.org>
---
 hw/mips/malta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index e435f80973..2e175741ff 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -72,6 +72,8 @@
 
 #define FLASH_SIZE          0x400000
 
+#define PIIX4_PCI_DEVFN     PCI_DEVFN(10, 0)
+
 typedef struct {
     MemoryRegion iomem;
     MemoryRegion iomem_lo; /* 0 - 0x900 */
@@ -1427,7 +1429,7 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    piix4 = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
+    piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
                                             TYPE_PIIX4_PCI_DEVICE);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
 
-- 
2.39.0

[PATCH v4 02/30] hw/mips/malta: Set PIIX4 IRQ routes in embedded bootloader

[Bernhard Beschow]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

Linux kernel expects the northbridge & southbridge chipsets
configured by the BIOS firmware. We emulate that by writing
a tiny bootloader code in write_bootloader().

Upon introduction in commit 5c2b87e34d ("PIIX4 support"),
the PIIX4 configuration space included values specific to
the Malta board.

Set the Malta-specific IRQ routing values in the embedded
bootloader, so the next commit can remove the Malta specific
bits from the PIIX4 PCI-ISA bridge and make it generic
(matching the real hardware).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221027204720.33611-3-philmd@linaro.org>
---
 hw/mips/malta.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 2e175741ff..ef3e10dc4d 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -804,6 +804,8 @@ static void write_bootloader_nanomips(uint8_t *base, uint64_t run_addr,
     stw_p(p++, 0x8422); stw_p(p++, 0x9088);
                                 /* sw t0, 0x88(t1)              */
 
+    /* TODO set PIIX IRQC[A:D] routing values! */
+
     stw_p(p++, 0xe320 | NM_HI1(kernel_entry));
 
     stw_p(p++, NM_HI2(kernel_entry));
@@ -841,6 +843,9 @@ static void write_bootloader_nanomips(uint8_t *base, uint64_t run_addr,
 static void write_bootloader(uint8_t *base, uint64_t run_addr,
                              uint64_t kernel_entry)
 {
+    const char pci_pins_cfg[PCI_NUM_PINS] = {
+        10, 10, 11, 11 /* PIIX IRQRC[A:D] */
+    };
     uint32_t *p;
 
     /* Small bootloader */
@@ -915,6 +920,20 @@ static void write_bootloader(uint8_t *base, uint64_t run_addr,
 
 #undef cpu_to_gt32
 
+    /*
+     * The PIIX ISA bridge is on PCI bus 0 dev 10 func 0.
+     * Load the PIIX IRQC[A:D] routing config address, then
+     * write routing configuration to the config data register.
+     */
+    bl_gen_write_u32(&p, /* GT_PCI0_CFGADDR */
+                     cpu_mips_phys_to_kseg1(NULL, 0x1be00000 + 0xcf8),
+                     tswap32((1 << 31) /* ConfigEn */
+                             | PCI_BUILD_BDF(0, PIIX4_PCI_DEVFN) << 8
+                             | PIIX_PIRQCA));
+    bl_gen_write_u32(&p, /* GT_PCI0_CFGDATA */
+                     cpu_mips_phys_to_kseg1(NULL, 0x1be00000 + 0xcfc),
+                     tswap32(ldl_be_p(pci_pins_cfg)));
+
     bl_gen_jump_kernel(&p,
                        true, ENVP_VADDR - 64,
                        /*
-- 
2.39.0

[PATCH v4 03/30] hw/isa/piix4: Correct IRQRC[A:D] reset values

[Bernhard Beschow]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

IRQRC[A:D] registers reset value is 0x80. We were forcing
the MIPS Malta machine routing to be able to boot a Linux
kernel without any bootloader.
We now have these registers initialized in the Malta machine
write_bootloader(), so we can use the correct reset values.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221027204720.33611-4-philmd@linaro.org>
---
 hw/isa/piix4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index eca96fb8f0..6e9434129d 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -91,10 +91,10 @@ static void piix4_isa_reset(DeviceState *dev)
     pci_conf[0x4c] = 0x4d;
     pci_conf[0x4e] = 0x03;
     pci_conf[0x4f] = 0x00;
-    pci_conf[0x60] = 0x0a; // PCI A -> IRQ 10
-    pci_conf[0x61] = 0x0a; // PCI B -> IRQ 10
-    pci_conf[0x62] = 0x0b; // PCI C -> IRQ 11
-    pci_conf[0x63] = 0x0b; // PCI D -> IRQ 11
+    pci_conf[0x60] = 0x80;
+    pci_conf[0x61] = 0x80;
+    pci_conf[0x62] = 0x80;
+    pci_conf[0x63] = 0x80;
     pci_conf[0x69] = 0x02;
     pci_conf[0x70] = 0x80;
     pci_conf[0x76] = 0x0c;
-- 
2.39.0

[PATCH v4 04/30] hw/mips/Kconfig: Track Malta's PIIX dependencies via Kconfig

[Bernhard Beschow]

Tracking dependencies via Kconfig seems much cleaner.

Note that PIIX4 already depends on ACPI_PIIX4.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 configs/devices/mips-softmmu/common.mak | 2 --
 hw/mips/Kconfig                         | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/configs/devices/mips-softmmu/common.mak b/configs/devices/mips-softmmu/common.mak
index 416161f833..7813fd1b41 100644
--- a/configs/devices/mips-softmmu/common.mak
+++ b/configs/devices/mips-softmmu/common.mak
@@ -18,10 +18,8 @@ CONFIG_PCSPK=y
 CONFIG_PCKBD=y
 CONFIG_FDC=y
 CONFIG_ACPI=y
-CONFIG_ACPI_PIIX4=y
 CONFIG_APM=y
 CONFIG_I8257=y
-CONFIG_PIIX4=y
 CONFIG_IDE_ISA=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_I8259=y
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index 725525358d..4e7042f03d 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -1,6 +1,7 @@
 config MALTA
     bool
     select ISA_SUPERIO
+    select PIIX4
 
 config MIPSSIM
     bool
-- 
2.39.0

[PATCH v4 05/30] hw/usb/hcd-uhci: Introduce TYPE_ defines for device models

[Bernhard Beschow]

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-10-shentey@gmail.com>
---
 hw/i386/pc_piix.c |  3 ++-
 hw/i386/pc_q35.c  | 13 +++++++------
 hw/isa/piix4.c    |  2 +-
 hw/usb/hcd-uhci.c | 16 ++++++++--------
 hw/usb/hcd-uhci.h |  4 ++++
 5 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ecae85a31e..e4bb8994da 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -51,6 +51,7 @@
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/piix4.h"
+#include "hw/usb/hcd-uhci.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/xen.h"
@@ -306,7 +307,7 @@ 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");
+        pci_create_simple(pci_bus, piix3_devfn + 2, TYPE_PIIX3_USB_UHCI);
     }
 
     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 39f035903c..ed541102f4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -48,6 +48,7 @@
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
+#include "hw/usb/hcd-uhci.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
@@ -65,15 +66,15 @@ struct ehci_companions {
 };
 
 static const struct ehci_companions ich9_1d[] = {
-    { .name = "ich9-usb-uhci1", .func = 0, .port = 0 },
-    { .name = "ich9-usb-uhci2", .func = 1, .port = 2 },
-    { .name = "ich9-usb-uhci3", .func = 2, .port = 4 },
+    { .name = TYPE_ICH9_USB_UHCI(1), .func = 0, .port = 0 },
+    { .name = TYPE_ICH9_USB_UHCI(2), .func = 1, .port = 2 },
+    { .name = TYPE_ICH9_USB_UHCI(3), .func = 2, .port = 4 },
 };
 
 static const struct ehci_companions ich9_1a[] = {
-    { .name = "ich9-usb-uhci4", .func = 0, .port = 0 },
-    { .name = "ich9-usb-uhci5", .func = 1, .port = 2 },
-    { .name = "ich9-usb-uhci6", .func = 2, .port = 4 },
+    { .name = TYPE_ICH9_USB_UHCI(4), .func = 0, .port = 0 },
+    { .name = TYPE_ICH9_USB_UHCI(5), .func = 1, .port = 2 },
+    { .name = TYPE_ICH9_USB_UHCI(6), .func = 2, .port = 4 },
 };
 
 static int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 6e9434129d..de60ceef73 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -255,7 +255,7 @@ static void piix4_init(Object *obj)
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
-    object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
+    object_initialize_child(obj, "uhci", &s->uhci, TYPE_PIIX4_USB_UHCI);
 
     object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
     qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index d1b5657d72..30ae0104bb 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1292,56 +1292,56 @@ void uhci_data_class_init(ObjectClass *klass, void *data)
 
 static UHCIInfo uhci_info[] = {
     {
-        .name       = "piix3-usb-uhci",
+        .name      = TYPE_PIIX3_USB_UHCI,
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82371SB_2,
         .revision  = 0x01,
         .irq_pin   = 3,
         .unplug    = true,
     },{
-        .name      = "piix4-usb-uhci",
+        .name      = TYPE_PIIX4_USB_UHCI,
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82371AB_2,
         .revision  = 0x01,
         .irq_pin   = 3,
         .unplug    = true,
     },{
-        .name      = "ich9-usb-uhci1", /* 00:1d.0 */
+        .name      = TYPE_ICH9_USB_UHCI(1), /* 00:1d.0 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI1,
         .revision  = 0x03,
         .irq_pin   = 0,
         .unplug    = false,
     },{
-        .name      = "ich9-usb-uhci2", /* 00:1d.1 */
+        .name      = TYPE_ICH9_USB_UHCI(2), /* 00:1d.1 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI2,
         .revision  = 0x03,
         .irq_pin   = 1,
         .unplug    = false,
     },{
-        .name      = "ich9-usb-uhci3", /* 00:1d.2 */
+        .name      = TYPE_ICH9_USB_UHCI(3), /* 00:1d.2 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI3,
         .revision  = 0x03,
         .irq_pin   = 2,
         .unplug    = false,
     },{
-        .name      = "ich9-usb-uhci4", /* 00:1a.0 */
+        .name      = TYPE_ICH9_USB_UHCI(4), /* 00:1a.0 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI4,
         .revision  = 0x03,
         .irq_pin   = 0,
         .unplug    = false,
     },{
-        .name      = "ich9-usb-uhci5", /* 00:1a.1 */
+        .name      = TYPE_ICH9_USB_UHCI(5), /* 00:1a.1 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI5,
         .revision  = 0x03,
         .irq_pin   = 1,
         .unplug    = false,
     },{
-        .name      = "ich9-usb-uhci6", /* 00:1a.2 */
+        .name      = TYPE_ICH9_USB_UHCI(6), /* 00:1a.2 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .device_id = PCI_DEVICE_ID_INTEL_82801I_UHCI6,
         .revision  = 0x03,
diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
index c85ab7868e..83e6f548b1 100644
--- a/hw/usb/hcd-uhci.h
+++ b/hw/usb/hcd-uhci.h
@@ -91,4 +91,8 @@ typedef struct UHCIInfo {
 void uhci_data_class_init(ObjectClass *klass, void *data);
 void usb_uhci_common_realize(PCIDevice *dev, Error **errp);
 
+#define TYPE_PIIX3_USB_UHCI "piix3-usb-uhci"
+#define TYPE_PIIX4_USB_UHCI "piix4-usb-uhci"
+#define TYPE_ICH9_USB_UHCI(fn) "ich9-usb-uhci" #fn
+
 #endif
-- 
2.39.0

[PATCH v4 06/30] hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is created

[Bernhard Beschow]

Observe that the pci_map_irq_fn's don't depend on the south bridge
instance. So associate them immediately when the PCI bus is created to
keep things logically together.

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e4bb8994da..bfa7cb513b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -229,6 +229,9 @@ static void pc_init1(MachineState *machine,
                               x86ms->below_4g_mem_size,
                               x86ms->above_4g_mem_size,
                               pci_memory, ram_memory);
+        pci_bus_map_irqs(pci_bus,
+                         xen_enabled() ? xen_pci_slot_get_pirq
+                                       : pci_slot_get_pirq);
         pcms->bus = pci_bus;
 
         pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
@@ -236,10 +239,6 @@ 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"));
-
-        pci_bus_map_irqs(pci_bus,
-                         xen_enabled() ? xen_pci_slot_get_pirq
-                                       : pci_slot_get_pirq);
     } else {
         pci_bus = NULL;
         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
-- 
2.39.0

[PATCH v4 07/30] hw/i386/pc_piix: Allow for setting properties before realizing PIIX3 south bridge

[Bernhard Beschow]

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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-3-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 bfa7cb513b..0689b7d3f7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -234,7 +234,8 @@ static void pc_init1(MachineState *machine,
                                        : pci_slot_get_pirq);
         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.39.0

[PATCH v4 08/30] hw/i386/pc: Create RTC controllers in south bridges

[Bernhard Beschow]

Just like in the real hardware (and in PIIX4), create the RTC
controllers in the south bridges.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-11-shentey@gmail.com>
---
 hw/i386/pc.c                  | 12 +++++++++++-
 hw/i386/pc_piix.c             |  8 ++++++++
 hw/i386/pc_q35.c              |  1 +
 hw/isa/Kconfig                |  2 ++
 hw/isa/lpc_ich9.c             |  8 ++++++++
 hw/isa/piix3.c                | 15 +++++++++++++++
 include/hw/i386/ich9.h        |  2 ++
 include/hw/southbridge/piix.h |  3 +++
 8 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fa69b6f43e..d154eedcb3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1299,7 +1299,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);
+
+    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);
+    }
+    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
+                              "date");
 
     qemu_register_boot_set(pc_boot_set, *rtc_state);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0689b7d3f7..d4a9c79713 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"
@@ -240,10 +241,17 @@ 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"));
     } else {
         pci_bus = 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 ed541102f4..92817a9ebd 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -239,6 +239,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/Kconfig b/hw/isa/Kconfig
index 18b5c6bf3f..af5ec9cd61 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -35,6 +35,7 @@ config PIIX3
     bool
     select I8257
     select ISA_BUS
+    select MC146818RTC
 
 config PIIX4
     bool
@@ -79,3 +80,4 @@ config LPC_ICH9
     select ISA_BUS
     select ACPI_SMBUS
     select ACPI_X86_ICH
+    select MC146818RTC
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 6c44cc9767..eb230a1a23 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 283b971ec4..e8ddb6a602 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -28,6 +28,7 @@
 #include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
 #include "hw/irq.h"
+#include "hw/qdev-properties.h"
 #include "hw/isa/isa.h"
 #include "hw/xen/xen.h"
 #include "sysemu/runstate.h"
@@ -301,6 +302,12 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
                                         PIIX_RCR_IOPORT, &d->rcr_mem, 1);
 
     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;
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -327,6 +334,13 @@ 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, "rtc", &d->rtc, TYPE_MC146818_RTC);
+}
+
 static void pci_piix3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -353,6 +367,7 @@ static const TypeInfo piix3_pci_type_info = {
     .name = TYPE_PIIX3_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIX3State),
+    .instance_init = pci_piix3_init,
     .abstract = true,
     .class_init = pci_piix3_class_init,
     .interfaces = (InterfaceInfo[]) {
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 2693778b23..b1fa08dd2b 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/rtc/mc146818rtc.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
@@ -52,6 +53,8 @@ struct PIIXState {
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
+    RTCState rtc;
+
     /* Reset Control Register contents */
     uint8_t rcr;
 
-- 
2.39.0

[PATCH v4 09/30] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-12-shentey@gmail.com>
---
 hw/i386/pc.c         | 12 ++++++------
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     |  2 +-
 include/hw/i386/pc.h |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d154eedcb3..6990687211 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1246,7 +1246,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)
 {
@@ -1301,17 +1301,17 @@ 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);
     }
-    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
+    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(rtc_state),
                               "date");
 
-    qemu_register_boot_set(pc_boot_set, *rtc_state);
+    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 d4a9c79713..5e6dba3558 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -277,7 +277,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 92817a9ebd..ef3e1c72f0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -293,7 +293,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 c95333514e..0cf3ccdf0d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -169,7 +169,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.39.0

[PATCH v4 10/30] hw/isa/piix3: Create USB controller in host device

[Bernhard Beschow]

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

Note that the USB function is optional in QEMU. This is why it gets
object_initialize_child()'ed in realize rather than in instance_init.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-13-shentey@gmail.com>
---
 hw/i386/pc_piix.c             |  7 ++-----
 hw/isa/Kconfig                |  1 +
 hw/isa/piix3.c                | 17 +++++++++++++++++
 include/hw/southbridge/piix.h |  4 ++++
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5e6dba3558..18523e8a80 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -52,7 +52,6 @@
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/piix4.h"
-#include "hw/usb/hcd-uhci.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/xen.h"
@@ -236,6 +235,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;
@@ -314,10 +315,6 @@ static void pc_init1(MachineState *machine,
     }
 #endif
 
-    if (pcmc->pci_enabled && machine_usb(machine)) {
-        pci_create_simple(pci_bus, piix3_devfn + 2, TYPE_PIIX3_USB_UHCI);
-    }
-
     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
         PCIDevice *piix4_pm;
 
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index af5ec9cd61..97b8ea7c06 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -36,6 +36,7 @@ config PIIX3
     select I8257
     select ISA_BUS
     select MC146818RTC
+    select USB_UHCI
 
 config PIIX4
     bool
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index e8ddb6a602..45c20dea17 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -288,6 +288,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), pci_address_space(dev),
@@ -308,6 +309,16 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
     if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
         return;
     }
+
+    /* USB */
+    if (d->has_usb) {
+        object_initialize_child(OBJECT(dev), "uhci", &d->uhci,
+                                TYPE_PIIX3_USB_UHCI);
+        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+            return;
+        }
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -341,6 +352,11 @@ static void pci_piix3_init(Object *obj)
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
 }
 
+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);
@@ -360,6 +376,7 @@ 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;
 }
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index b1fa08dd2b..5367917182 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/rtc/mc146818rtc.h"
+#include "hw/usb/hcd-uhci.h"
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
@@ -54,12 +55,15 @@ struct PIIXState {
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
     RTCState rtc;
+    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.39.0

[PATCH v4 11/30] hw/isa/piix3: Create power management controller in host device

[Bernhard Beschow]

The power management controller is an integral part of PIIX3 (function
3). So create it as part of the south bridge.

Note that the ACPI function is optional in QEMU. This is why it gets
object_initialize_child()'ed in realize rather than in instance_init.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-14-shentey@gmail.com>
---
 hw/i386/pc_piix.c             | 24 ++++++++++++++----------
 hw/isa/Kconfig                |  1 +
 hw/isa/piix3.c                | 14 ++++++++++++++
 include/hw/southbridge/piix.h |  6 ++++++
 4 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 18523e8a80..10f2db6f2d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -46,12 +46,12 @@
 #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 "hw/xen/xen.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"
@@ -97,6 +97,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;
@@ -237,15 +238,25 @@ 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);
+        qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
+        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"));
         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);
 
@@ -315,15 +326,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);
@@ -337,7 +341,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/Kconfig b/hw/isa/Kconfig
index 97b8ea7c06..6c154d88c7 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -33,6 +33,7 @@ config PC87312
 
 config PIIX3
     bool
+    select ACPI_PIIX4
     select I8257
     select ISA_BUS
     select MC146818RTC
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 45c20dea17..ed7d58bc98 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -319,6 +319,17 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
             return;
         }
     }
+
+    /* Power Management */
+    if (d->has_acpi) {
+        object_initialize_child(OBJECT(d), "pm", &d->pm, TYPE_PIIX4_PM);
+        qdev_prop_set_int32(DEVICE(&d->pm), "addr", dev->devfn + 3);
+        qdev_prop_set_uint32(DEVICE(&d->pm), "smb_io_base", d->smb_io_base);
+        qdev_prop_set_bit(DEVICE(&d->pm), "smm-enabled", d->smm_enabled);
+        if (!qdev_realize(DEVICE(&d->pm), BUS(pci_bus), errp)) {
+            return;
+        }
+    }
 }
 
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -353,7 +364,10 @@ static void pci_piix3_init(Object *obj)
 }
 
 static Property pci_piix3_props[] = {
+    DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
+    DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
     DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
+    DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 5367917182..1c291cc954 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/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
 
@@ -56,6 +57,9 @@ struct PIIXState {
 
     RTCState rtc;
     UHCIState uhci;
+    PIIX4PMState pm;
+
+    uint32_t smb_io_base;
 
     /* Reset Control Register contents */
     uint8_t rcr;
@@ -63,7 +67,9 @@ struct PIIXState {
     /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
     MemoryRegion rcr_mem;
 
+    bool has_acpi;
     bool has_usb;
+    bool smm_enabled;
 };
 typedef struct PIIXState PIIX3State;
 
-- 
2.39.0

[PATCH v4 12/30] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Having a proxy PIC allows for ISA PICs to be created and wired up in
southbridges. This is especially useful 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-15-shentey@gmail.com>
---
 MAINTAINERS                 |  2 ++
 hw/core/Kconfig             |  3 ++
 hw/core/meson.build         |  1 +
 hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
 include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
 5 files changed, 130 insertions(+)
 create mode 100644 hw/core/proxy-pic.c
 create mode 100644 include/hw/core/proxy-pic.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 716d5a24ad..f862bfc7d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1675,6 +1675,7 @@ S: Supported
 F: hw/char/debugcon.c
 F: hw/char/parallel*
 F: hw/char/serial*
+F: hw/core/proxy-pic.c
 F: hw/dma/i8257*
 F: hw/i2c/pm_smbus.c
 F: hw/input/pckbd.c
@@ -1691,6 +1692,7 @@ F: hw/watchdog/wdt_ib700.c
 F: hw/watchdog/wdt_i6300esb.c
 F: include/hw/display/vga.h
 F: include/hw/char/parallel.h
+F: include/hw/core/proxy-pic.h
 F: include/hw/dma/i8257.h
 F: include/hw/i2c/pm_smbus.h
 F: include/hw/input/i8042.h
diff --git a/hw/core/Kconfig b/hw/core/Kconfig
index 9397503656..a7224f4ca0 100644
--- a/hw/core/Kconfig
+++ b/hw/core/Kconfig
@@ -22,6 +22,9 @@ config OR_IRQ
 config PLATFORM_BUS
     bool
 
+config PROXY_PIC
+    bool
+
 config REGISTER
     bool
 
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 7a4d02b6c0..e86aef6ec3 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
 softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
 softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
 softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
+softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
 softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
 softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
new file mode 100644
index 0000000000..3251727d19
--- /dev/null
+++ b/hw/core/proxy-pic.c
@@ -0,0 +1,70 @@
+/*
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/proxy-pic.h"
+
+static void proxy_pic_set_irq(void *opaque, int irq, int level)
+{
+    ProxyPICState *s = opaque;
+
+    qemu_set_irq(s->out_irqs[irq], level);
+}
+
+static void proxy_pic_realize(DeviceState *dev, Error **errp)
+{
+    ProxyPICState *s = PROXY_PIC(dev);
+
+    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
+    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
+
+    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
+        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
+    }
+}
+
+static void proxy_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    /* No state to reset or migrate */
+    dc->realize = proxy_pic_realize;
+
+    /* Reason: Needs to be wired up to work */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo proxy_pic_info = {
+    .name          = TYPE_PROXY_PIC,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(ProxyPICState),
+    .class_init = proxy_pic_class_init,
+};
+
+static void split_irq_register_types(void)
+{
+    type_register_static(&proxy_pic_info);
+}
+
+type_init(split_irq_register_types)
diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
new file mode 100644
index 0000000000..0eb40c478a
--- /dev/null
+++ b/include/hw/core/proxy-pic.h
@@ -0,0 +1,54 @@
+/*
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_PROXY_PIC_H
+#define HW_PROXY_PIC_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+#include "hw/irq.h"
+
+#define TYPE_PROXY_PIC "proxy-pic"
+OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
+
+#define MAX_PROXY_PIC_LINES 16
+
+/**
+ * This is a simple device which has 16 pairs of GPIO input and output lines.
+ * Any change on an input line is forwarded to the respective output.
+ *
+ * QEMU interface:
+ *  + 16 unnamed GPIO inputs: the input lines
+ *  + 16 unnamed GPIO outputs: the output lines
+ */
+struct ProxyPICState {
+    /*< private >*/
+    struct DeviceState parent_obj;
+    /*< public >*/
+
+    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
+    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
+};
+
+#endif /* HW_PROXY_PIC_H */
-- 
2.39.0

[PATCH v4 13/30] hw/isa/piix3: Create Proxy PIC in host device

[Bernhard Beschow]

Use the newly introduced TYPE_PROXY_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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-16-shentey@gmail.com>
---
 hw/i386/Kconfig               |  1 +
 hw/i386/pc_piix.c             | 15 +++++++++------
 hw/isa/Kconfig                |  1 +
 hw/isa/piix3.c                | 10 +++++++++-
 include/hw/southbridge/piix.h |  4 ++--
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..79f5925dbe 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -72,6 +72,7 @@ config I440FX
     select PC_PCI
     select PC_ACPI
     select ACPI_SMBUS
+    select I8259
     select PCI_I440FX
     select PIIX3
     select IDE_PIIX
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 10f2db6f2d..e33406a2e3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -219,10 +219,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(pci_type,
                               i440fx_host,
@@ -247,10 +248,12 @@ static void pc_init1(MachineState *machine,
                                  &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");
@@ -259,6 +262,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);
@@ -267,7 +271,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/Kconfig b/hw/isa/Kconfig
index 6c154d88c7..b4ad1fb66e 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -37,6 +37,7 @@ config PIIX3
     select I8257
     select ISA_BUS
     select MC146818RTC
+    select PROXY_PIC
     select USB_UHCI
 
 config PIIX4
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index ed7d58bc98..9e9155cbda 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -39,7 +39,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))));
@@ -297,6 +297,13 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
+    /* PIC */
+    if (!qdev_realize(DEVICE(&d->pic), NULL, 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),
@@ -360,6 +367,7 @@ static void pci_piix3_init(Object *obj)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(obj);
 
+    object_initialize_child(obj, "pic", &d->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
 }
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 1c291cc954..7b1b4625a3 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/core/proxy-pic.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];
 
+    ProxyPICState pic;
     RTCState rtc;
     UHCIState uhci;
     PIIX4PMState pm;
-- 
2.39.0

[PATCH v4 14/30] hw/isa/piix3: Create IDE controller in host device

[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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-17-shentey@gmail.com>
---
 hw/i386/Kconfig               |  1 -
 hw/i386/pc_piix.c             | 15 ++++++---------
 hw/isa/Kconfig                |  1 +
 hw/isa/piix3.c                |  8 ++++++++
 include/hw/southbridge/piix.h |  2 ++
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 79f5925dbe..39a35467ca 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -75,7 +75,6 @@ config I440FX
     select I8259
     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 e33406a2e3..8c3d3698eb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -41,7 +41,6 @@
 #include "hw/usb.h"
 #include "net/net.h"
 #include "hw/ide/pci.h"
-#include "hw/ide/piix.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
 #include "hw/kvm/clock.h"
@@ -98,7 +97,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];
@@ -252,11 +250,14 @@ 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"));
+        pci_ide_create_devs(PCI_DEVICE(dev));
+        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;
@@ -270,6 +271,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) {
@@ -298,12 +301,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, TYPE_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 b4ad1fb66e..8bf6462798 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -35,6 +35,7 @@ config PIIX3
     bool
     select ACPI_PIIX4
     select I8257
+    select IDE_PIIX
     select ISA_BUS
     select MC146818RTC
     select PROXY_PIC
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 9e9155cbda..d6d36db01e 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -29,6 +29,7 @@
 #include "hw/southbridge/piix.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "hw/ide/piix.h"
 #include "hw/isa/isa.h"
 #include "hw/xen/xen.h"
 #include "sysemu/runstate.h"
@@ -317,6 +318,12 @@ 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;
+    }
+
     /* USB */
     if (d->has_usb) {
         object_initialize_child(OBJECT(dev), "uhci", &d->uhci,
@@ -369,6 +376,7 @@ static void pci_piix3_init(Object *obj)
 
     object_initialize_child(obj, "pic", &d->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "ide", &d->ide, TYPE_PIIX3_IDE);
 }
 
 static Property pci_piix3_props[] = {
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 7b1b4625a3..c4e6e9f827 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -16,6 +16,7 @@
 #include "qom/object.h"
 #include "hw/acpi/piix4.h"
 #include "hw/core/proxy-pic.h"
+#include "hw/ide/pci.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
 
@@ -56,6 +57,7 @@ struct PIIXState {
 
     ProxyPICState pic;
     RTCState rtc;
+    PCIIDEState ide;
     UHCIState uhci;
     PIIX4PMState pm;
 
-- 
2.39.0

[PATCH v4 15/30] 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-18-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 8c3d3698eb..3ff84209fe 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -329,7 +329,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 d6d36db01e..c33a3faa2f 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -343,6 +343,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));
     }
 }
 
-- 
2.39.0

[PATCH v4 16/30] hw/isa/piix3: Resolve redundant PIIX_NUM_PIC_IRQS

[Bernhard Beschow]

PIIX_NUM_PIC_IRQS is assumed to be the same as ISA_NUM_IRQS, otherwise
inconsistencies can occur.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-21-shentey@gmail.com>
---
 hw/isa/piix3.c                | 8 ++++----
 include/hw/southbridge/piix.h | 5 ++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index c33a3faa2f..a1fd1e0d3e 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -52,7 +52,7 @@ static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level)
     uint64_t mask;
 
     pic_irq = piix3->dev.config[PIIX_PIRQCA + pirq];
-    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+    if (pic_irq >= ISA_NUM_IRQS) {
         return;
     }
 
@@ -66,7 +66,7 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
     int pic_irq;
 
     pic_irq = piix3->dev.config[PIIX_PIRQCA + pirq];
-    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+    if (pic_irq >= ISA_NUM_IRQS) {
         return;
     }
 
@@ -87,7 +87,7 @@ static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
     int irq = piix3->dev.config[PIIX_PIRQCA + pin];
     PCIINTxRoute route;
 
-    if (irq < PIIX_NUM_PIC_IRQS) {
+    if (irq < ISA_NUM_IRQS) {
         route.mode = PCI_INTX_ENABLED;
         route.irq = irq;
     } else {
@@ -119,7 +119,7 @@ static void piix3_write_config(PCIDevice *dev,
 
         pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix3->dev));
         piix3_update_irq_levels(piix3);
-        for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
+        for (pic_irq = 0; pic_irq < ISA_NUM_IRQS; pic_irq++) {
             piix3_set_irq_pic(piix3, pic_irq);
         }
     }
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index c4e6e9f827..39c31da9ad 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -32,7 +32,6 @@
  */
 #define PIIX_RCR_IOPORT 0xcf9
 
-#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
 #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
 
 struct PIIXState {
@@ -44,10 +43,10 @@ struct PIIXState {
      * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
      *
      * PIRQ is mapped to PIC pins, we track it by
-     * PIIX_NUM_PIRQS * PIIX_NUM_PIC_IRQS = 64 bits with
+     * PIIX_NUM_PIRQS * ISA_NUM_IRQS = 64 bits with
      * pic_irq * PIIX_NUM_PIRQS + pirq
      */
-#if PIIX_NUM_PIC_IRQS * PIIX_NUM_PIRQS > 64
+#if ISA_NUM_IRQS * PIIX_NUM_PIRQS > 64
 #error "unable to encode pic state in 64bit in pic_levels."
 #endif
     uint64_t pic_levels;
-- 
2.39.0

[PATCH v4 17/30] hw/isa/piix3: Rename pci_piix3_props for sharing with PIIX4

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-22-shentey@gmail.com>
---
 hw/isa/piix3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index a1fd1e0d3e..63f41741e0 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -381,7 +381,7 @@ static void pci_piix3_init(Object *obj)
     object_initialize_child(obj, "ide", &d->ide, TYPE_PIIX3_IDE);
 }
 
-static Property pci_piix3_props[] = {
+static Property pci_piix_props[] = {
     DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
     DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
     DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
@@ -408,7 +408,7 @@ 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);
+    device_class_set_props(dc, pci_piix_props);
     adevc->build_dev_aml = build_pci_isa_aml;
 }
 
-- 
2.39.0

[PATCH v4 18/30] hw/isa/piix3: Rename piix3_reset() for sharing with PIIX4

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-23-shentey@gmail.com>
---
 hw/isa/piix3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 63f41741e0..0848ae1c47 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -145,7 +145,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
     piix3_write_config(dev, address, val, len);
 }
 
-static void piix3_reset(DeviceState *dev)
+static void piix_reset(DeviceState *dev)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
     uint8_t *pci_conf = d->dev.config;
@@ -395,7 +395,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
 
-    dc->reset       = piix3_reset;
+    dc->reset       = piix_reset;
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
     dc->hotpluggable   = false;
-- 
2.39.0

[PATCH v4 19/30] hw/isa/piix3: Drop the "3" from PIIX base class

[Bernhard Beschow]

This commit marks the finalization of the PIIX3 preparations
to be merged with PIIX4. In particular, PIIXState is prepared
to be reused in piix4.c.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-25-shentey@gmail.com>
---
 hw/isa/piix3.c                | 60 +++++++++++++++++------------------
 include/hw/southbridge/piix.h |  6 ++--
 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 0848ae1c47..970fad6549 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -38,7 +38,7 @@
 
 #define XEN_PIIX_NUM_PIRQS      128ULL
 
-static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
+static void piix3_set_irq_pic(PIIXState *piix3, int pic_irq)
 {
     qemu_set_irq(piix3->pic.in_irqs[pic_irq],
                  !!(piix3->pic_levels &
@@ -46,7 +46,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
                      (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level)
+static void piix3_set_irq_level_internal(PIIXState *piix3, int pirq, int level)
 {
     int pic_irq;
     uint64_t mask;
@@ -61,7 +61,7 @@ static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level)
     piix3->pic_levels |= mask * !!level;
 }
 
-static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
+static void piix3_set_irq_level(PIIXState *piix3, int pirq, int level)
 {
     int pic_irq;
 
@@ -77,13 +77,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
 
 static void piix3_set_irq(void *opaque, int pirq, int level)
 {
-    PIIX3State *piix3 = opaque;
+    PIIXState *piix3 = opaque;
     piix3_set_irq_level(piix3, pirq, level);
 }
 
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 {
-    PIIX3State *piix3 = opaque;
+    PIIXState *piix3 = opaque;
     int irq = piix3->dev.config[PIIX_PIRQCA + pin];
     PCIINTxRoute route;
 
@@ -98,7 +98,7 @@ static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 }
 
 /* irq routing is changed. so rebuild bitmap */
-static void piix3_update_irq_levels(PIIX3State *piix3)
+static void piix3_update_irq_levels(PIIXState *piix3)
 {
     PCIBus *bus = pci_get_bus(&piix3->dev);
     int pirq;
@@ -114,7 +114,7 @@ static void piix3_write_config(PCIDevice *dev,
 {
     pci_default_write_config(dev, address, val, len);
     if (ranges_overlap(address, len, PIIX_PIRQCA, 4)) {
-        PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+        PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
         int pic_irq;
 
         pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix3->dev));
@@ -147,7 +147,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
 
 static void piix_reset(DeviceState *dev)
 {
-    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    PIIXState *d = PIIX_PCI_DEVICE(dev);
     uint8_t *pci_conf = d->dev.config;
 
     pci_conf[0x04] = 0x07; /* master, memory and I/O */
@@ -188,7 +188,7 @@ static void piix_reset(DeviceState *dev)
 
 static int piix3_post_load(void *opaque, int version_id)
 {
-    PIIX3State *piix3 = opaque;
+    PIIXState *piix3 = opaque;
     int pirq;
 
     /*
@@ -211,7 +211,7 @@ static int piix3_post_load(void *opaque, int version_id)
 static int piix3_pre_save(void *opaque)
 {
     int i;
-    PIIX3State *piix3 = opaque;
+    PIIXState *piix3 = opaque;
 
     for (i = 0; i < ARRAY_SIZE(piix3->pci_irq_levels_vmstate); i++) {
         piix3->pci_irq_levels_vmstate[i] =
@@ -223,7 +223,7 @@ static int piix3_pre_save(void *opaque)
 
 static bool piix3_rcr_needed(void *opaque)
 {
-    PIIX3State *piix3 = opaque;
+    PIIXState *piix3 = opaque;
 
     return (piix3->rcr != 0);
 }
@@ -234,7 +234,7 @@ static const VMStateDescription vmstate_piix3_rcr = {
     .minimum_version_id = 1,
     .needed = piix3_rcr_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT8(rcr, PIIX3State),
+        VMSTATE_UINT8(rcr, PIIXState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -246,8 +246,8 @@ static const VMStateDescription vmstate_piix3 = {
     .post_load = piix3_post_load,
     .pre_save = piix3_pre_save,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, PIIX3State),
-        VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIX3State,
+        VMSTATE_PCI_DEVICE(dev, PIIXState),
+        VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIXState,
                               PIIX_NUM_PIRQS, 3),
         VMSTATE_END_OF_LIST()
     },
@@ -260,7 +260,7 @@ static const VMStateDescription vmstate_piix3 = {
 
 static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
 {
-    PIIX3State *d = opaque;
+    PIIXState *d = opaque;
 
     if (val & 4) {
         qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
@@ -271,7 +271,7 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
 
 static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned len)
 {
-    PIIX3State *d = opaque;
+    PIIXState *d = opaque;
 
     return d->rcr;
 }
@@ -288,7 +288,7 @@ static const MemoryRegionOps rcr_ops = {
 
 static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 {
-    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    PIIXState *d = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
 
@@ -374,7 +374,7 @@ static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
 
 static void pci_piix3_init(Object *obj)
 {
-    PIIX3State *d = PIIX3_PCI_DEVICE(obj);
+    PIIXState *d = PIIX_PCI_DEVICE(obj);
 
     object_initialize_child(obj, "pic", &d->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
@@ -382,10 +382,10 @@ static void pci_piix3_init(Object *obj)
 }
 
 static Property pci_piix_props[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
-    DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
-    DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
-    DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
+    DEFINE_PROP_UINT32("smb_io_base", PIIXState, smb_io_base, 0),
+    DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
+    DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
+    DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -412,10 +412,10 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
     adevc->build_dev_aml = build_pci_isa_aml;
 }
 
-static const TypeInfo piix3_pci_type_info = {
-    .name = TYPE_PIIX3_PCI_DEVICE,
+static const TypeInfo piix_pci_type_info = {
+    .name = TYPE_PIIX_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIX3State),
+    .instance_size = sizeof(PIIXState),
     .instance_init = pci_piix3_init,
     .abstract = true,
     .class_init = pci_piix3_class_init,
@@ -429,7 +429,7 @@ static const TypeInfo piix3_pci_type_info = {
 static void piix3_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
-    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
     pci_piix3_realize(dev, errp);
@@ -451,14 +451,14 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo piix3_info = {
     .name          = TYPE_PIIX3_DEVICE,
-    .parent        = TYPE_PIIX3_PCI_DEVICE,
+    .parent        = TYPE_PIIX_PCI_DEVICE,
     .class_init    = piix3_class_init,
 };
 
 static void piix3_xen_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
-    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
     pci_piix3_realize(dev, errp);
@@ -485,13 +485,13 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo piix3_xen_info = {
     .name          = TYPE_PIIX3_XEN_DEVICE,
-    .parent        = TYPE_PIIX3_PCI_DEVICE,
+    .parent        = TYPE_PIIX_PCI_DEVICE,
     .class_init    = piix3_xen_class_init,
 };
 
 static void piix3_register_types(void)
 {
-    type_register_static(&piix3_pci_type_info);
+    type_register_static(&piix_pci_type_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
 }
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 39c31da9ad..65ad8569da 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -72,11 +72,9 @@ struct PIIXState {
     bool has_usb;
     bool smm_enabled;
 };
-typedef struct PIIXState PIIX3State;
 
-#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
-DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
-                         TYPE_PIIX3_PCI_DEVICE)
+#define TYPE_PIIX_PCI_DEVICE "pci-piix"
+OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE)
 
 #define TYPE_PIIX3_DEVICE "PIIX3"
 #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
-- 
2.39.0

[PATCH v4 20/30] hw/isa/piix4: Make PIIX4's ACPI and USB functions optional

[Bernhard Beschow]

This aligns PIIX4 with PIIX3.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-30-shentey@gmail.com>
---
 hw/isa/piix4.c  | 44 ++++++++++++++++++++++++++++++++------------
 hw/mips/malta.c |  6 ++++--
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index de60ceef73..de4133f573 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -51,9 +51,16 @@ struct PIIX4State {
     PCIIDEState ide;
     UHCIState uhci;
     PIIX4PMState pm;
+
+    uint32_t smb_io_base;
+
     /* Reset Control Register */
     MemoryRegion rcr_mem;
     uint8_t rcr;
+
+    bool has_acpi;
+    bool has_usb;
+    bool smm_enabled;
 };
 
 OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
@@ -234,17 +241,26 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     }
 
     /* USB */
-    qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
-    if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
-        return;
+    if (s->has_usb) {
+        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
+                                TYPE_PIIX4_USB_UHCI);
+        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), 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;
+    if (s->has_acpi) {
+        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
+        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
+        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
+        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
+        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
+            return;
+        }
+        qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
     }
-    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
 
     pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
 }
@@ -255,13 +271,16 @@ static void piix4_init(Object *obj)
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
-    object_initialize_child(obj, "uhci", &s->uhci, TYPE_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 Property piix4_props[] = {
+    DEFINE_PROP_UINT32("smb_io_base", PIIX4State, smb_io_base, 0),
+    DEFINE_PROP_BOOL("has-acpi", PIIX4State, has_acpi, true),
+    DEFINE_PROP_BOOL("has-usb", PIIX4State, has_usb, true),
+    DEFINE_PROP_BOOL("smm-enabled", PIIX4State, smm_enabled, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void piix4_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -280,6 +299,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
      */
     dc->user_creatable = false;
     dc->hotpluggable = false;
+    device_class_set_props(dc, piix4_props);
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ef3e10dc4d..a930a91f00 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1448,8 +1448,10 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
-                                            TYPE_PIIX4_PCI_DEVICE);
+    piix4 = pci_new_multifunction(PIIX4_PCI_DEVFN, true,
+                                  TYPE_PIIX4_PCI_DEVICE);
+    qdev_prop_set_uint32(DEVICE(piix4), "smb_io_base", 0x1100);
+    pci_realize_and_unref(piix4, pci_bus, &error_fatal);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
 
     dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "ide"));
-- 
2.39.0

[PATCH v4 21/30] hw/isa/piix4: Remove unused inbound ISA interrupt lines

[Bernhard Beschow]

The Malta board, which is the only user of PIIX4, doesn't connect to the
exported interrupt lines. PIIX3 doesn't expose such intterupt lines
either, so remove them for PIIX4 for simplicity and consistency.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-32-shentey@gmail.com>
---
 hw/isa/piix4.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index de4133f573..9edaa5de3e 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -155,12 +155,6 @@ static void piix4_request_i8259_irq(void *opaque, int irq, int level)
     qemu_set_irq(s->cpu_intr, level);
 }
 
-static void piix4_set_i8259_irq(void *opaque, int irq, int level)
-{
-    PIIX4State *s = opaque;
-    qemu_set_irq(s->isa[irq], level);
-}
-
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {
@@ -204,8 +198,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
-    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
-                            "isa", ISA_NUM_IRQS);
     qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
                              "intr", 1);
 
-- 
2.39.0

[PATCH v4 22/30] hw/isa/piix4: Use Proxy PIC device

[Bernhard Beschow]

Aligns the code with PIIX3 such that PIIXState can be used in PIIX4,
too.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-33-shentey@gmail.com>
---
 hw/isa/Kconfig  |  2 +-
 hw/isa/piix4.c  | 30 +++++++++++-------------------
 hw/mips/Kconfig |  1 +
 hw/mips/malta.c | 11 +++++++++--
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 8bf6462798..4dfa3310d9 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -48,10 +48,10 @@ config PIIX4
     select ACPI_PIIX4
     select I8254
     select I8257
-    select I8259
     select IDE_PIIX
     select ISA_BUS
     select MC146818RTC
+    select PROXY_PIC
     select USB_UHCI
 
 config VT82C686
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 9edaa5de3e..a68e45cd53 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -30,7 +30,7 @@
 #include "hw/pci/pci.h"
 #include "hw/ide/piix.h"
 #include "hw/isa/isa.h"
-#include "hw/intc/i8259.h"
+#include "hw/core/proxy-pic.h"
 #include "hw/dma/i8257.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
@@ -44,9 +44,8 @@
 
 struct PIIX4State {
     PCIDevice dev;
-    qemu_irq cpu_intr;
-    qemu_irq *isa;
 
+    ProxyPICState pic;
     RTCState rtc;
     PCIIDEState ide;
     UHCIState uhci;
@@ -82,7 +81,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
                 pic_level |= pci_bus_get_irq_level(bus, i);
             }
         }
-        qemu_set_irq(s->isa[pic_irq], pic_level);
+        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
     }
 }
 
@@ -149,12 +148,6 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
-static void piix4_request_i8259_irq(void *opaque, int irq, int level)
-{
-    PIIX4State *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {
@@ -190,7 +183,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
-    qemu_irq *i8259_out_irq;
 
     isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
                           pci_address_space_io(dev), errp);
@@ -198,20 +190,18 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
-    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
-                             "intr", 1);
-
     memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
                           "reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
                                         PIIX_RCR_IOPORT, &s->rcr_mem, 1);
 
     /* initialize i8259 pic */
-    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
-    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
+        return;
+    }
 
     /* initialize ISA irqs */
-    isa_bus_irqs(isa_bus, s->isa);
+    isa_bus_irqs(isa_bus, s->pic.in_irqs);
 
     /* initialize pit */
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
@@ -224,7 +214,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
         return;
     }
-    s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
+    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
 
     /* IDE */
     qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
@@ -251,7 +241,8 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
             return;
         }
-        qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
+        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
+                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
     }
 
     pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
@@ -261,6 +252,7 @@ static void piix4_init(Object *obj)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(obj);
 
+    object_initialize_child(obj, "pic", &s->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
 }
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index 4e7042f03d..d156de812c 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -1,5 +1,6 @@
 config MALTA
     bool
+    select I8259
     select ISA_SUPERIO
     select PIIX4
 
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index a930a91f00..1bb493353b 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -29,6 +29,7 @@
 #include "qemu/guest-random.h"
 #include "hw/clock.h"
 #include "hw/southbridge/piix.h"
+#include "hw/intc/i8259.h"
 #include "hw/isa/superio.h"
 #include "hw/char/serial.h"
 #include "net/net.h"
@@ -1280,10 +1281,11 @@ void mips_malta_init(MachineState *machine)
     PCIBus *pci_bus;
     ISABus *isa_bus;
     qemu_irq cbus_irq, i8259_irq;
+    qemu_irq *i8259;
     I2CBus *smbus;
     DriveInfo *dinfo;
     int fl_idx = 0;
-    int be;
+    int be, i;
     MaltaState *s;
     PCIDevice *piix4;
     DeviceState *dev;
@@ -1458,7 +1460,12 @@ void mips_malta_init(MachineState *machine)
     pci_ide_create_devs(PCI_DEVICE(dev));
 
     /* Interrupt controller */
-    qdev_connect_gpio_out_named(DEVICE(piix4), "intr", 0, i8259_irq);
+    dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "pic"));
+    i8259 = i8259_init(isa_bus, i8259_irq);
+    for (i = 0; i < ISA_NUM_IRQS; i++) {
+        qdev_connect_gpio_out(dev, i, i8259[i]);
+    }
+    g_free(i8259);
 
     pci_bus_map_irqs(pci_bus, pci_slot_get_pirq);
 
-- 
2.39.0

[PATCH v4 23/30] hw/isa/piix4: Reuse struct PIIXState from PIIX3

[Bernhard Beschow]

Now that PIIX4 also uses the "proxy-pic", both implementations
can share the same struct.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-34-shentey@gmail.com>
---
 hw/isa/piix4.c | 51 +++++++++++++++-----------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a68e45cd53..6f1580ae66 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -42,32 +42,10 @@
 #include "sysemu/runstate.h"
 #include "qom/object.h"
 
-struct PIIX4State {
-    PCIDevice dev;
-
-    ProxyPICState pic;
-    RTCState rtc;
-    PCIIDEState ide;
-    UHCIState uhci;
-    PIIX4PMState pm;
-
-    uint32_t smb_io_base;
-
-    /* Reset Control Register */
-    MemoryRegion rcr_mem;
-    uint8_t rcr;
-
-    bool has_acpi;
-    bool has_usb;
-    bool smm_enabled;
-};
-
-OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
-
 static void piix4_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
-    PIIX4State *s = opaque;
+    PIIXState *s = opaque;
     PCIBus *bus = pci_get_bus(&s->dev);
 
     /* now we change the pic irq level according to the piix irq mappings */
@@ -87,7 +65,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
 
 static void piix4_isa_reset(DeviceState *dev)
 {
-    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
+    PIIXState *d = PIIX_PCI_DEVICE(dev);
     uint8_t *pci_conf = d->dev.config;
 
     pci_conf[0x04] = 0x07; // master, memory and I/O
@@ -122,12 +100,13 @@ static void piix4_isa_reset(DeviceState *dev)
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
 
+    d->pic_levels = 0; /* not used in PIIX4 */
     d->rcr = 0;
 }
 
 static int piix4_post_load(void *opaque, int version_id)
 {
-    PIIX4State *s = opaque;
+    PIIXState *s = opaque;
 
     if (version_id == 2) {
         s->rcr = 0;
@@ -142,8 +121,8 @@ static const VMStateDescription vmstate_piix4 = {
     .minimum_version_id = 2,
     .post_load = piix4_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, PIIX4State),
-        VMSTATE_UINT8_V(rcr, PIIX4State, 3),
+        VMSTATE_PCI_DEVICE(dev, PIIXState),
+        VMSTATE_UINT8_V(rcr, PIIXState, 3),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -151,7 +130,7 @@ static const VMStateDescription vmstate_piix4 = {
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {
-    PIIX4State *s = opaque;
+    PIIXState *s = opaque;
 
     if (val & 4) {
         qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
@@ -163,7 +142,7 @@ static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
 
 static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned int len)
 {
-    PIIX4State *s = opaque;
+    PIIXState *s = opaque;
 
     return s->rcr;
 }
@@ -180,7 +159,7 @@ static const MemoryRegionOps piix4_rcr_ops = {
 
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
-    PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    PIIXState *s = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
 
@@ -250,7 +229,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 
 static void piix4_init(Object *obj)
 {
-    PIIX4State *s = PIIX4_PCI_DEVICE(obj);
+    PIIXState *s = PIIX_PCI_DEVICE(obj);
 
     object_initialize_child(obj, "pic", &s->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
@@ -258,10 +237,10 @@ static void piix4_init(Object *obj)
 }
 
 static Property piix4_props[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIX4State, smb_io_base, 0),
-    DEFINE_PROP_BOOL("has-acpi", PIIX4State, has_acpi, true),
-    DEFINE_PROP_BOOL("has-usb", PIIX4State, has_usb, true),
-    DEFINE_PROP_BOOL("smm-enabled", PIIX4State, smm_enabled, false),
+    DEFINE_PROP_UINT32("smb_io_base", PIIXState, smb_io_base, 0),
+    DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
+    DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
+    DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -289,7 +268,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix4_info = {
     .name          = TYPE_PIIX4_PCI_DEVICE,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIX4State),
+    .instance_size = sizeof(PIIXState),
     .instance_init = piix4_init,
     .class_init    = piix4_class_init,
     .interfaces = (InterfaceInfo[]) {
-- 
2.39.0

[PATCH v4 24/30] hw/isa/piix4: Rename reset control operations to match PIIX3

[Bernhard Beschow]

Both implementations are the same and will be shared upon merging.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-35-shentey@gmail.com>
---
 hw/isa/piix4.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 6f1580ae66..dbc6a16ac7 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -127,7 +127,7 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
-static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
+static void rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {
     PIIXState *s = opaque;
@@ -140,16 +140,16 @@ static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
     s->rcr = val & 2; /* keep System Reset type only */
 }
 
-static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned int len)
+static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned int len)
 {
     PIIXState *s = opaque;
 
     return s->rcr;
 }
 
-static const MemoryRegionOps piix4_rcr_ops = {
-    .read = piix4_rcr_read,
-    .write = piix4_rcr_write,
+static const MemoryRegionOps rcr_ops = {
+    .read = rcr_read,
+    .write = rcr_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -169,7 +169,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
-    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
+    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
                           "reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
                                         PIIX_RCR_IOPORT, &s->rcr_mem, 1);
-- 
2.39.0

[PATCH v4 25/30] hw/isa/piix3: Merge hw/isa/piix4.c

[Bernhard Beschow]

Now that the PIIX3 and PIIX4 device models are sufficiently consolidated,
their implementations can be merged into one file for further
consolidation.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-37-shentey@gmail.com>
---
 MAINTAINERS                |   6 +-
 hw/i386/Kconfig            |   2 +-
 hw/isa/Kconfig             |  12 +-
 hw/isa/meson.build         |   3 +-
 hw/isa/{piix3.c => piix.c} | 158 ++++++++++++++++++++
 hw/isa/piix4.c             | 285 -------------------------------------
 hw/mips/Kconfig            |   2 +-
 7 files changed, 165 insertions(+), 303 deletions(-)
 rename hw/isa/{piix3.c => piix.c} (75%)
 delete mode 100644 hw/isa/piix4.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f862bfc7d3..f37cbccbcf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1237,7 +1237,7 @@ Malta
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
 R: Aurelien Jarno <aurelien@aurel32.net>
 S: Odd Fixes
-F: hw/isa/piix4.c
+F: hw/isa/piix.c
 F: hw/acpi/piix4.c
 F: hw/mips/malta.c
 F: hw/mips/gt64xxx_pci.c
@@ -1655,7 +1655,7 @@ F: hw/pci-host/pam.c
 F: include/hw/pci-host/i440fx.h
 F: include/hw/pci-host/q35.h
 F: include/hw/pci-host/pam.h
-F: hw/isa/piix3.c
+F: hw/isa/piix.c
 F: hw/isa/lpc_ich9.c
 F: hw/i2c/smbus_ich9.c
 F: hw/acpi/piix4.c
@@ -2346,7 +2346,7 @@ PIIX4 South Bridge (i82371AB)
 M: Hervé Poussineau <hpoussin@reactos.org>
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
 S: Maintained
-F: hw/isa/piix4.c
+F: hw/isa/piix.c
 F: include/hw/southbridge/piix.h
 
 Firmware configuration (fw_cfg)
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 39a35467ca..15442ddbdf 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -74,7 +74,7 @@ config I440FX
     select ACPI_SMBUS
     select I8259
     select PCI_I440FX
-    select PIIX3
+    select PIIX
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 4dfa3310d9..0f3284220b 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -31,17 +31,7 @@ config PC87312
     select FDC_ISA
     select IDE_ISA
 
-config PIIX3
-    bool
-    select ACPI_PIIX4
-    select I8257
-    select IDE_PIIX
-    select ISA_BUS
-    select MC146818RTC
-    select PROXY_PIC
-    select USB_UHCI
-
-config PIIX4
+config PIIX
     bool
     # For historical reasons, SuperIO devices are created in the board
     # for PIIX4.
diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index 8bf678ca0a..314bbd0860 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -3,8 +3,7 @@ softmmu_ss.add(when: 'CONFIG_I82378', if_true: files('i82378.c'))
 softmmu_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
 softmmu_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
 softmmu_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
-softmmu_ss.add(when: 'CONFIG_PIIX3', if_true: files('piix3.c'))
-softmmu_ss.add(when: 'CONFIG_PIIX4', if_true: files('piix4.c'))
+softmmu_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
 softmmu_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
 
diff --git a/hw/isa/piix3.c b/hw/isa/piix.c
similarity index 75%
rename from hw/isa/piix3.c
rename to hw/isa/piix.c
index 970fad6549..4683b0fa95 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix.c
@@ -2,6 +2,7 @@
  * QEMU PIIX PCI ISA Bridge Emulation
  *
  * Copyright (c) 2006 Fabrice Bellard
+ * Copyright (c) 2018 Hervé Poussineau
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -27,6 +28,7 @@
 #include "qapi/error.h"
 #include "hw/dma/i8257.h"
 #include "hw/southbridge/piix.h"
+#include "hw/timer/i8254.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/ide/piix.h"
@@ -81,6 +83,27 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
     piix3_set_irq_level(piix3, pirq, level);
 }
 
+static void piix4_set_irq(void *opaque, int irq_num, int level)
+{
+    int i, pic_irq, pic_level;
+    PIIXState *s = opaque;
+    PCIBus *bus = pci_get_bus(&s->dev);
+
+    /* now we change the pic irq level according to the piix irq mappings */
+    /* XXX: optimize */
+    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
+    if (pic_irq < ISA_NUM_IRQS) {
+        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
+        pic_level = 0;
+        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
+            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
+                pic_level |= pci_bus_get_irq_level(bus, i);
+            }
+        }
+        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
+    }
+}
+
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 {
     PIIXState *piix3 = opaque;
@@ -208,6 +231,17 @@ static int piix3_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static int piix4_post_load(void *opaque, int version_id)
+{
+    PIIXState *s = opaque;
+
+    if (version_id == 2) {
+        s->rcr = 0;
+    }
+
+    return 0;
+}
+
 static int piix3_pre_save(void *opaque)
 {
     int i;
@@ -257,6 +291,17 @@ static const VMStateDescription vmstate_piix3 = {
     }
 };
 
+static const VMStateDescription vmstate_piix4 = {
+    .name = "PIIX4",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .post_load = piix4_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, PIIXState),
+        VMSTATE_UINT8_V(rcr, PIIXState, 3),
+        VMSTATE_END_OF_LIST()
+    }
+};
 
 static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
 {
@@ -489,11 +534,124 @@ static const TypeInfo piix3_xen_info = {
     .class_init    = piix3_xen_class_init,
 };
 
+static void piix4_realize(PCIDevice *dev, Error **errp)
+{
+    PIIXState *s = PIIX_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+    ISABus *isa_bus;
+
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
+        return;
+    }
+
+    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
+                          "reset-control", 1);
+    memory_region_add_subregion_overlap(pci_address_space_io(dev),
+                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
+
+    /* initialize i8259 pic */
+    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
+        return;
+    }
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->pic.in_irqs);
+
+    /* initialize pit */
+    i8254_pit_init(isa_bus, 0x40, 0, NULL);
+
+    /* DMA */
+    i8257_dma_init(isa_bus, 0);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
+    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
+
+    /* IDE */
+    qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
+    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
+        return;
+    }
+
+    /* USB */
+    if (s->has_usb) {
+        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
+                                TYPE_PIIX4_USB_UHCI);
+        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
+            return;
+        }
+    }
+
+    /* ACPI controller */
+    if (s->has_acpi) {
+        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
+        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
+        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
+        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
+        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
+            return;
+        }
+        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
+                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
+    }
+
+    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
+}
+
+static void piix4_init(Object *obj)
+{
+    PIIXState *s = PIIX_PCI_DEVICE(obj);
+
+    object_initialize_child(obj, "pic", &s->pic, TYPE_PROXY_PIC);
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
+}
+
+static void piix4_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = piix4_realize;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    dc->reset = piix_reset;
+    dc->desc = "ISA bridge";
+    dc->vmsd = &vmstate_piix4;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->user_creatable = false;
+    dc->hotpluggable = false;
+    device_class_set_props(dc, pci_piix_props);
+}
+
+static const TypeInfo piix4_info = {
+    .name          = TYPE_PIIX4_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PIIXState),
+    .instance_init = piix4_init,
+    .class_init    = piix4_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
 static void piix3_register_types(void)
 {
     type_register_static(&piix_pci_type_info);
     type_register_static(&piix3_info);
     type_register_static(&piix3_xen_info);
+    type_register_static(&piix4_info);
 }
 
 type_init(piix3_register_types)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
deleted file mode 100644
index dbc6a16ac7..0000000000
--- a/hw/isa/piix4.c
+++ /dev/null
@@ -1,285 +0,0 @@
-/*
- * QEMU PIIX4 PCI Bridge Emulation
- *
- * Copyright (c) 2006 Fabrice Bellard
- * Copyright (c) 2018 Hervé Poussineau
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "hw/irq.h"
-#include "hw/southbridge/piix.h"
-#include "hw/pci/pci.h"
-#include "hw/ide/piix.h"
-#include "hw/isa/isa.h"
-#include "hw/core/proxy-pic.h"
-#include "hw/dma/i8257.h"
-#include "hw/timer/i8254.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "hw/ide/pci.h"
-#include "hw/acpi/piix4.h"
-#include "hw/usb/hcd-uhci.h"
-#include "migration/vmstate.h"
-#include "sysemu/reset.h"
-#include "sysemu/runstate.h"
-#include "qom/object.h"
-
-static void piix4_set_irq(void *opaque, int irq_num, int level)
-{
-    int i, pic_irq, pic_level;
-    PIIXState *s = opaque;
-    PCIBus *bus = pci_get_bus(&s->dev);
-
-    /* now we change the pic irq level according to the piix irq mappings */
-    /* XXX: optimize */
-    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
-    if (pic_irq < ISA_NUM_IRQS) {
-        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
-        pic_level = 0;
-        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
-            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_bus_get_irq_level(bus, i);
-            }
-        }
-        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
-    }
-}
-
-static void piix4_isa_reset(DeviceState *dev)
-{
-    PIIXState *d = PIIX_PCI_DEVICE(dev);
-    uint8_t *pci_conf = d->dev.config;
-
-    pci_conf[0x04] = 0x07; // master, memory and I/O
-    pci_conf[0x05] = 0x00;
-    pci_conf[0x06] = 0x00;
-    pci_conf[0x07] = 0x02; // PCI_status_devsel_medium
-    pci_conf[0x4c] = 0x4d;
-    pci_conf[0x4e] = 0x03;
-    pci_conf[0x4f] = 0x00;
-    pci_conf[0x60] = 0x80;
-    pci_conf[0x61] = 0x80;
-    pci_conf[0x62] = 0x80;
-    pci_conf[0x63] = 0x80;
-    pci_conf[0x69] = 0x02;
-    pci_conf[0x70] = 0x80;
-    pci_conf[0x76] = 0x0c;
-    pci_conf[0x77] = 0x0c;
-    pci_conf[0x78] = 0x02;
-    pci_conf[0x79] = 0x00;
-    pci_conf[0x80] = 0x00;
-    pci_conf[0x82] = 0x00;
-    pci_conf[0xa0] = 0x08;
-    pci_conf[0xa2] = 0x00;
-    pci_conf[0xa3] = 0x00;
-    pci_conf[0xa4] = 0x00;
-    pci_conf[0xa5] = 0x00;
-    pci_conf[0xa6] = 0x00;
-    pci_conf[0xa7] = 0x00;
-    pci_conf[0xa8] = 0x0f;
-    pci_conf[0xaa] = 0x00;
-    pci_conf[0xab] = 0x00;
-    pci_conf[0xac] = 0x00;
-    pci_conf[0xae] = 0x00;
-
-    d->pic_levels = 0; /* not used in PIIX4 */
-    d->rcr = 0;
-}
-
-static int piix4_post_load(void *opaque, int version_id)
-{
-    PIIXState *s = opaque;
-
-    if (version_id == 2) {
-        s->rcr = 0;
-    }
-
-    return 0;
-}
-
-static const VMStateDescription vmstate_piix4 = {
-    .name = "PIIX4",
-    .version_id = 3,
-    .minimum_version_id = 2,
-    .post_load = piix4_post_load,
-    .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, PIIXState),
-        VMSTATE_UINT8_V(rcr, PIIXState, 3),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static void rcr_write(void *opaque, hwaddr addr, uint64_t val,
-                            unsigned int len)
-{
-    PIIXState *s = opaque;
-
-    if (val & 4) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
-        return;
-    }
-
-    s->rcr = val & 2; /* keep System Reset type only */
-}
-
-static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned int len)
-{
-    PIIXState *s = opaque;
-
-    return s->rcr;
-}
-
-static const MemoryRegionOps rcr_ops = {
-    .read = rcr_read,
-    .write = rcr_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
-
-static void piix4_realize(PCIDevice *dev, Error **errp)
-{
-    PIIXState *s = PIIX_PCI_DEVICE(dev);
-    PCIBus *pci_bus = pci_get_bus(dev);
-    ISABus *isa_bus;
-
-    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                          pci_address_space_io(dev), errp);
-    if (!isa_bus) {
-        return;
-    }
-
-    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
-                          "reset-control", 1);
-    memory_region_add_subregion_overlap(pci_address_space_io(dev),
-                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
-
-    /* initialize i8259 pic */
-    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
-        return;
-    }
-
-    /* initialize ISA irqs */
-    isa_bus_irqs(isa_bus, s->pic.in_irqs);
-
-    /* initialize pit */
-    i8254_pit_init(isa_bus, 0x40, 0, NULL);
-
-    /* DMA */
-    i8257_dma_init(isa_bus, 0);
-
-    /* RTC */
-    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
-    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
-        return;
-    }
-    s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
-
-    /* IDE */
-    qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
-    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
-        return;
-    }
-
-    /* USB */
-    if (s->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
-                                TYPE_PIIX4_USB_UHCI);
-        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
-            return;
-        }
-    }
-
-    /* ACPI controller */
-    if (s->has_acpi) {
-        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
-        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
-        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
-        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
-        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
-            return;
-        }
-        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
-                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
-    }
-
-    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
-}
-
-static void piix4_init(Object *obj)
-{
-    PIIXState *s = PIIX_PCI_DEVICE(obj);
-
-    object_initialize_child(obj, "pic", &s->pic, TYPE_PROXY_PIC);
-    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
-    object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
-}
-
-static Property piix4_props[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIXState, smb_io_base, 0),
-    DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
-    DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
-    DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void piix4_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = piix4_realize;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
-    k->class_id = PCI_CLASS_BRIDGE_ISA;
-    dc->reset = piix4_isa_reset;
-    dc->desc = "ISA bridge";
-    dc->vmsd = &vmstate_piix4;
-    /*
-     * Reason: part of PIIX4 southbridge, needs to be wired up,
-     * e.g. by mips_malta_init()
-     */
-    dc->user_creatable = false;
-    dc->hotpluggable = false;
-    device_class_set_props(dc, piix4_props);
-}
-
-static const TypeInfo piix4_info = {
-    .name          = TYPE_PIIX4_PCI_DEVICE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIXState),
-    .instance_init = piix4_init,
-    .class_init    = piix4_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
-};
-
-static void piix4_register_types(void)
-{
-    type_register_static(&piix4_info);
-}
-
-type_init(piix4_register_types)
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index d156de812c..5b16ff4ed2 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -2,7 +2,7 @@ config MALTA
     bool
     select I8259
     select ISA_SUPERIO
-    select PIIX4
+    select PIIX
 
 config MIPSSIM
     bool
-- 
2.39.0

[PATCH v4 26/30] hw/isa/piix: Harmonize names of reset control memory regions

[Bernhard Beschow]

There is no need for having different names here. Having the same name
further allows code to be shared between PIIX3 and PIIX4.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-38-shentey@gmail.com>
---
 hw/isa/piix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 4683b0fa95..0bb508481f 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -351,7 +351,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
     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);
+                          "piix-reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
                                         PIIX_RCR_IOPORT, &d->rcr_mem, 1);
 
@@ -547,7 +547,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     }
 
     memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
-                          "reset-control", 1);
+                          "piix-reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
                                         PIIX_RCR_IOPORT, &s->rcr_mem, 1);
 
-- 
2.39.0

[PATCH v4 27/30] hw/isa/piix: Reuse PIIX3 base class' realize method in PIIX4

[Bernhard Beschow]

Resolves duplicate code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-39-shentey@gmail.com>
---
 hw/isa/piix.c | 65 +++++++--------------------------------------------
 1 file changed, 9 insertions(+), 56 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 0bb508481f..de54ac5abe 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -331,7 +331,8 @@ static const MemoryRegionOps rcr_ops = {
     },
 };
 
-static void pci_piix3_realize(PCIDevice *dev, Error **errp)
+static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
+                             Error **errp)
 {
     PIIXState *d = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
@@ -371,8 +372,7 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 
     /* USB */
     if (d->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &d->uhci,
-                                TYPE_PIIX3_USB_UHCI);
+        object_initialize_child(OBJECT(dev), "uhci", &d->uhci, uhci_type);
         qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
         if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
             return;
@@ -477,7 +477,7 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
     PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
-    pci_piix3_realize(dev, errp);
+    pci_piix_realize(dev, TYPE_PIIX3_USB_UHCI, errp);
     if (*errp) {
         return;
     }
@@ -506,7 +506,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
     PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
-    pci_piix3_realize(dev, errp);
+    pci_piix_realize(dev, TYPE_PIIX3_USB_UHCI, errp);
     if (*errp) {
         return;
     }
@@ -536,71 +536,24 @@ static const TypeInfo piix3_xen_info = {
 
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
+    ERRP_GUARD();
     PIIXState *s = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
 
-    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                          pci_address_space_io(dev), errp);
-    if (!isa_bus) {
-        return;
-    }
-
-    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &rcr_ops, s,
-                          "piix-reset-control", 1);
-    memory_region_add_subregion_overlap(pci_address_space_io(dev),
-                                        PIIX_RCR_IOPORT, &s->rcr_mem, 1);
-
-    /* initialize i8259 pic */
-    if (!qdev_realize(DEVICE(&s->pic), NULL, errp)) {
+    pci_piix_realize(dev, TYPE_PIIX4_USB_UHCI, errp);
+    if (*errp) {
         return;
     }
 
-    /* initialize ISA irqs */
-    isa_bus_irqs(isa_bus, s->pic.in_irqs);
+    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
 
     /* initialize pit */
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
 
-    /* DMA */
-    i8257_dma_init(isa_bus, 0);
-
     /* RTC */
-    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
-    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
-        return;
-    }
     s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
 
-    /* IDE */
-    qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
-    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
-        return;
-    }
-
-    /* USB */
-    if (s->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &s->uhci,
-                                TYPE_PIIX4_USB_UHCI);
-        qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
-            return;
-        }
-    }
-
-    /* ACPI controller */
-    if (s->has_acpi) {
-        object_initialize_child(OBJECT(s), "pm", &s->pm, TYPE_PIIX4_PM);
-        qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
-        qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", s->smb_io_base);
-        qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", s->smm_enabled);
-        if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
-            return;
-        }
-        qdev_connect_gpio_out(DEVICE(&s->pm), 0,
-                              qdev_get_gpio_in(DEVICE(&s->pic), 9));
-    }
-
     pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
 }
 
-- 
2.39.0

[PATCH v4 28/30] hw/isa/piix: Rename functions to be shared for interrupt triggering

[Bernhard Beschow]

PIIX4 will get the same optimizations which are already implemented for
PIIX3.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-40-shentey@gmail.com>
---
 hw/isa/piix.c | 56 +++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index de54ac5abe..db7ed72c1e 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -40,47 +40,47 @@
 
 #define XEN_PIIX_NUM_PIRQS      128ULL
 
-static void piix3_set_irq_pic(PIIXState *piix3, int pic_irq)
+static void piix_set_irq_pic(PIIXState *piix, int pic_irq)
 {
-    qemu_set_irq(piix3->pic.in_irqs[pic_irq],
-                 !!(piix3->pic_levels &
+    qemu_set_irq(piix->pic.in_irqs[pic_irq],
+                 !!(piix->pic_levels &
                     (((1ULL << PIIX_NUM_PIRQS) - 1) <<
                      (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level_internal(PIIXState *piix3, int pirq, int level)
+static void piix_set_irq_level_internal(PIIXState *piix, int pirq, int level)
 {
     int pic_irq;
     uint64_t mask;
 
-    pic_irq = piix3->dev.config[PIIX_PIRQCA + pirq];
+    pic_irq = piix->dev.config[PIIX_PIRQCA + pirq];
     if (pic_irq >= ISA_NUM_IRQS) {
         return;
     }
 
     mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
-    piix3->pic_levels &= ~mask;
-    piix3->pic_levels |= mask * !!level;
+    piix->pic_levels &= ~mask;
+    piix->pic_levels |= mask * !!level;
 }
 
-static void piix3_set_irq_level(PIIXState *piix3, int pirq, int level)
+static void piix_set_irq_level(PIIXState *piix, int pirq, int level)
 {
     int pic_irq;
 
-    pic_irq = piix3->dev.config[PIIX_PIRQCA + pirq];
+    pic_irq = piix->dev.config[PIIX_PIRQCA + pirq];
     if (pic_irq >= ISA_NUM_IRQS) {
         return;
     }
 
-    piix3_set_irq_level_internal(piix3, pirq, level);
+    piix_set_irq_level_internal(piix, pirq, level);
 
-    piix3_set_irq_pic(piix3, pic_irq);
+    piix_set_irq_pic(piix, pic_irq);
 }
 
-static void piix3_set_irq(void *opaque, int pirq, int level)
+static void piix_set_irq(void *opaque, int pirq, int level)
 {
-    PIIXState *piix3 = opaque;
-    piix3_set_irq_level(piix3, pirq, level);
+    PIIXState *piix = opaque;
+    piix_set_irq_level(piix, pirq, level);
 }
 
 static void piix4_set_irq(void *opaque, int irq_num, int level)
@@ -121,29 +121,29 @@ static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 }
 
 /* irq routing is changed. so rebuild bitmap */
-static void piix3_update_irq_levels(PIIXState *piix3)
+static void piix_update_irq_levels(PIIXState *piix)
 {
-    PCIBus *bus = pci_get_bus(&piix3->dev);
+    PCIBus *bus = pci_get_bus(&piix->dev);
     int pirq;
 
-    piix3->pic_levels = 0;
+    piix->pic_levels = 0;
     for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
-        piix3_set_irq_level(piix3, pirq, pci_bus_get_irq_level(bus, pirq));
+        piix_set_irq_level(piix, pirq, pci_bus_get_irq_level(bus, pirq));
     }
 }
 
-static void piix3_write_config(PCIDevice *dev,
-                               uint32_t address, uint32_t val, int len)
+static void piix_write_config(PCIDevice *dev, uint32_t address, uint32_t val,
+                              int len)
 {
     pci_default_write_config(dev, address, val, len);
     if (ranges_overlap(address, len, PIIX_PIRQCA, 4)) {
-        PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
+        PIIXState *piix = PIIX_PCI_DEVICE(dev);
         int pic_irq;
 
-        pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix3->dev));
-        piix3_update_irq_levels(piix3);
+        pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix->dev));
+        piix_update_irq_levels(piix);
         for (pic_irq = 0; pic_irq < ISA_NUM_IRQS; pic_irq++) {
-            piix3_set_irq_pic(piix3, pic_irq);
+            piix_set_irq_pic(piix, pic_irq);
         }
     }
 }
@@ -165,7 +165,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
         }
     }
 
-    piix3_write_config(dev, address, val, len);
+    piix_write_config(dev, address, val, len);
 }
 
 static void piix_reset(DeviceState *dev)
@@ -225,7 +225,7 @@ static int piix3_post_load(void *opaque, int version_id)
      */
     piix3->pic_levels = 0;
     for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
-        piix3_set_irq_level_internal(piix3, pirq,
+        piix_set_irq_level_internal(piix3, pirq,
             pci_bus_get_irq_level(pci_get_bus(&piix3->dev), pirq));
     }
     return 0;
@@ -482,7 +482,7 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
-    pci_bus_irqs(pci_bus, piix3_set_irq, piix3, PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, piix_set_irq, piix3, PIIX_NUM_PIRQS);
     pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
 }
 
@@ -490,7 +490,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix3_write_config;
+    k->config_write = piix_write_config;
     k->realize = piix3_realize;
 }
 
-- 
2.39.0

[PATCH v4 29/30] hw/isa/piix: Consolidate IRQ triggering

[Bernhard Beschow]

Speeds up PIIX4 which resolves an old TODO.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-41-shentey@gmail.com>
---
 hw/isa/piix.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index db7ed72c1e..bd72015435 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -83,27 +83,6 @@ static void piix_set_irq(void *opaque, int pirq, int level)
     piix_set_irq_level(piix, pirq, level);
 }
 
-static void piix4_set_irq(void *opaque, int irq_num, int level)
-{
-    int i, pic_irq, pic_level;
-    PIIXState *s = opaque;
-    PCIBus *bus = pci_get_bus(&s->dev);
-
-    /* now we change the pic irq level according to the piix irq mappings */
-    /* XXX: optimize */
-    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
-    if (pic_irq < ISA_NUM_IRQS) {
-        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
-        pic_level = 0;
-        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
-            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_bus_get_irq_level(bus, i);
-            }
-        }
-        qemu_set_irq(s->pic.in_irqs[pic_irq], pic_level);
-    }
-}
-
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 {
     PIIXState *piix3 = opaque;
@@ -239,7 +218,7 @@ static int piix4_post_load(void *opaque, int version_id)
         s->rcr = 0;
     }
 
-    return 0;
+    return piix3_post_load(opaque, version_id);
 }
 
 static int piix3_pre_save(void *opaque)
@@ -554,7 +533,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     /* RTC */
     s->rtc.irq = qdev_get_gpio_in(DEVICE(&s->pic), s->rtc.isairq);
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, piix_set_irq, s, PIIX_NUM_PIRQS);
 }
 
 static void piix4_init(Object *obj)
@@ -571,6 +550,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+    k->config_write = piix_write_config;
     k->realize = piix4_realize;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
-- 
2.39.0

[PATCH v4 30/30] hw/isa/piix: Share PIIX3's base class with PIIX4

[Bernhard Beschow]

Having a common base class will allow for substituting PIIX3 with PIIX4
and vice versa. Moreover, it makes PIIX4 implement the
acpi-dev-aml-interface.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-42-shentey@gmail.com>
---
 hw/isa/piix.c | 49 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index bd72015435..ae8a27c53c 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -396,13 +396,12 @@ static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
     }
 }
 
-static void pci_piix3_init(Object *obj)
+static void pci_piix_init(Object *obj)
 {
     PIIXState *d = PIIX_PCI_DEVICE(obj);
 
     object_initialize_child(obj, "pic", &d->pic, TYPE_PROXY_PIC);
     object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
-    object_initialize_child(obj, "ide", &d->ide, TYPE_PIIX3_IDE);
 }
 
 static Property pci_piix_props[] = {
@@ -413,7 +412,7 @@ static Property pci_piix_props[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void pci_piix3_class_init(ObjectClass *klass, void *data)
+static void pci_piix_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -421,11 +420,8 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
 
     dc->reset       = piix_reset;
     dc->desc        = "ISA bridge";
-    dc->vmsd        = &vmstate_piix3;
     dc->hotpluggable   = false;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
-    /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
-    k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
     k->class_id     = PCI_CLASS_BRIDGE_ISA;
     /*
      * Reason: part of PIIX3 southbridge, needs to be wired up by
@@ -440,9 +436,9 @@ static const TypeInfo piix_pci_type_info = {
     .name = TYPE_PIIX_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIXState),
-    .instance_init = pci_piix3_init,
+    .instance_init = pci_piix_init,
     .abstract = true,
-    .class_init = pci_piix3_class_init,
+    .class_init = pci_piix_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { TYPE_ACPI_DEV_AML_IF },
@@ -465,17 +461,29 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
     pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
 }
 
+static void piix3_init(Object *obj)
+{
+    PIIXState *d = PIIX_PCI_DEVICE(obj);
+
+    object_initialize_child(obj, "ide", &d->ide, TYPE_PIIX3_IDE);
+}
+
 static void piix3_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix_write_config;
     k->realize = piix3_realize;
+    /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
+    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
+    dc->vmsd = &vmstate_piix3;
 }
 
 static const TypeInfo piix3_info = {
     .name          = TYPE_PIIX3_DEVICE,
     .parent        = TYPE_PIIX_PCI_DEVICE,
+    .instance_init = piix3_init,
     .class_init    = piix3_class_init,
 };
 
@@ -501,15 +509,20 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
 
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix3_write_config_xen;
     k->realize = piix3_xen_realize;
+    /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
+    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
+    dc->vmsd = &vmstate_piix3;
 }
 
 static const TypeInfo piix3_xen_info = {
     .name          = TYPE_PIIX3_XEN_DEVICE,
     .parent        = TYPE_PIIX_PCI_DEVICE,
+    .instance_init = piix3_init,
     .class_init    = piix3_xen_class_init,
 };
 
@@ -540,8 +553,6 @@ static void piix4_init(Object *obj)
 {
     PIIXState *s = PIIX_PCI_DEVICE(obj);
 
-    object_initialize_child(obj, "pic", &s->pic, TYPE_PROXY_PIC);
-    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
 }
 
@@ -552,31 +563,15 @@ static void piix4_class_init(ObjectClass *klass, void *data)
 
     k->config_write = piix_write_config;
     k->realize = piix4_realize;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
-    k->class_id = PCI_CLASS_BRIDGE_ISA;
-    dc->reset = piix_reset;
-    dc->desc = "ISA bridge";
     dc->vmsd = &vmstate_piix4;
-    /*
-     * Reason: part of PIIX4 southbridge, needs to be wired up,
-     * e.g. by mips_malta_init()
-     */
-    dc->user_creatable = false;
-    dc->hotpluggable = false;
-    device_class_set_props(dc, pci_piix_props);
 }
 
 static const TypeInfo piix4_info = {
     .name          = TYPE_PIIX4_PCI_DEVICE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIXState),
+    .parent        = TYPE_PIIX_PCI_DEVICE,
     .instance_init = piix4_init,
     .class_init    = piix4_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
 };
 
 static void piix3_register_types(void)
-- 
2.39.0

Re: [PATCH v4 00/30] This series consolidates the implementations of the PIIX3 and PIIX4 south

[Philippe Mathieu-Daudé]

On 21/12/22 17:59, Bernhard Beschow wrote:
> code as possible and to bring both device models to feature parity such that
> perhaps PIIX4 can become a drop-in-replacement for PIIX3 in the pc machine. This
> could resolve the "Frankenstein" PIIX4-PM problem in PIIX3 discussed on this
> list before.
> 
> The series is structured as follows: First, PIIX3 is changed to instantiate
> internal devices itself, like PIIX4 does already. Second, PIIX3 gets prepared
> for the merge with PIIX4 which includes some fixes, cleanups, and renamings.
> Third, the same is done for PIIX4. In step four the implementations are merged.
> Since some consolidations could be done easier with merged implementations, the
> consolidation continues in step five which concludes the series.
> 
> One particular challenge in this series was that the PIC of PIIX3 used to be
> instantiated outside of the south bridge 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). Due to consolidation PIIX4 gained the proxy PIC as well.
> 
> Another challenge was dealing with optional devices where Peter already gave
> advice in [1] which this series implements.
> 
> Last but not least there might be some opportunity to consolidate VM state
> handling, probably by reusing the one from PIIX3. Since I'm not very familiar
> with the requirements I didn't touch it so far.
> 
> v4:
> - Rebase onto "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges"
>    since it is already queued via mips-next. This eliminates patches
>    'hw/isa/piix3: Prefix pci_slot_get_pirq() with "piix3_"' and 'hw/isa/piix4:
>    Prefix pci_slot_get_pirq() with "piix4_"'.
> - Squash 'hw/isa/piix: Drop the "3" from the PIIX base class' into
>    'hw/isa/piix3: Rename typedef PIIX3State to PIIXState'. I originally only
>    split these patches since I wasn't sure whether renaming a type was allowed.
> - Add new patch 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is
>    created' for forther cleanup of INTx-to-LNKx route decoupling.

Sigh I did the rebase this morning and was waiting for the test suite...
https://gitlab.com/philmd/qemu/-/commits/mips-testing/

Anyway I'll double-check with this series.

Re: [PATCH v4 00/30] This series consolidates the implementations of the PIIX3 and PIIX4 south

[Bernhard Beschow]

Am 21. Dezember 2022 19:15:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 21/12/22 17:59, Bernhard Beschow wrote:
>> code as possible and to bring both device models to feature parity such that
>> perhaps PIIX4 can become a drop-in-replacement for PIIX3 in the pc machine. This
>> could resolve the "Frankenstein" PIIX4-PM problem in PIIX3 discussed on this
>> list before.
>> 
>> The series is structured as follows: First, PIIX3 is changed to instantiate
>> internal devices itself, like PIIX4 does already. Second, PIIX3 gets prepared
>> for the merge with PIIX4 which includes some fixes, cleanups, and renamings.
>> Third, the same is done for PIIX4. In step four the implementations are merged.
>> Since some consolidations could be done easier with merged implementations, the
>> consolidation continues in step five which concludes the series.
>> 
>> One particular challenge in this series was that the PIC of PIIX3 used to be
>> instantiated outside of the south bridge 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). Due to consolidation PIIX4 gained the proxy PIC as well.
>> 
>> Another challenge was dealing with optional devices where Peter already gave
>> advice in [1] which this series implements.
>> 
>> Last but not least there might be some opportunity to consolidate VM state
>> handling, probably by reusing the one from PIIX3. Since I'm not very familiar
>> with the requirements I didn't touch it so far.
>> 
>> v4:
>> - Rebase onto "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges"
>>    since it is already queued via mips-next. This eliminates patches
>>    'hw/isa/piix3: Prefix pci_slot_get_pirq() with "piix3_"' and 'hw/isa/piix4:
>>    Prefix pci_slot_get_pirq() with "piix4_"'.
>> - Squash 'hw/isa/piix: Drop the "3" from the PIIX base class' into
>>    'hw/isa/piix3: Rename typedef PIIX3State to PIIXState'. I originally only
>>    split these patches since I wasn't sure whether renaming a type was allowed.
>> - Add new patch 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is
>>    created' for forther cleanup of INTx-to-LNKx route decoupling.
>
>Sigh I did the rebase this morning and was waiting for the test suite...
>https://gitlab.com/philmd/qemu/-/commits/mips-testing/

Hmm, I'm a little confused. I thought to do the rebase myself in order to help which I announced yesterday.

AFAICS the nanoMIPS bootloader still needs to be adapted before this series can be queued and tested. Let me know if I could help.

>Anyway I'll double-check with this series.

Thank you!

Best regards,
Bernhard

Re: [PATCH v4 08/30] hw/i386/pc: Create RTC controllers in south bridges

[Thomas Huth]

On 21/12/2022 17.59, Bernhard Beschow wrote:
> Just like in the real hardware (and in PIIX4), create the RTC
> controllers in the south bridges.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-11-shentey@gmail.com>
> ---
>   hw/i386/pc.c                  | 12 +++++++++++-
>   hw/i386/pc_piix.c             |  8 ++++++++
>   hw/i386/pc_q35.c              |  1 +
>   hw/isa/Kconfig                |  2 ++
>   hw/isa/lpc_ich9.c             |  8 ++++++++
>   hw/isa/piix3.c                | 15 +++++++++++++++
>   include/hw/i386/ich9.h        |  2 ++
>   include/hw/southbridge/piix.h |  3 +++
>   8 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fa69b6f43e..d154eedcb3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1299,7 +1299,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);
> +
> +    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);
> +    }
> +    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
> +                              "date");
I think you could turn now the "ISADevice **rtc_state" parameter of this 
function into a normal "ISADevice *rtc_state" since the pointer is not a 
return value anymore.

  Thomas

Re: [PATCH v4 08/30] hw/i386/pc: Create RTC controllers in south bridges

[Bernhard Beschow]

Am 2. Januar 2023 17:03:29 UTC schrieb Thomas Huth <thuth@redhat.com>:
>On 21/12/2022 17.59, Bernhard Beschow wrote:
>> Just like in the real hardware (and in PIIX4), create the RTC
>> controllers in the south bridges.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-11-shentey@gmail.com>
>> ---
>>   hw/i386/pc.c                  | 12 +++++++++++-
>>   hw/i386/pc_piix.c             |  8 ++++++++
>>   hw/i386/pc_q35.c              |  1 +
>>   hw/isa/Kconfig                |  2 ++
>>   hw/isa/lpc_ich9.c             |  8 ++++++++
>>   hw/isa/piix3.c                | 15 +++++++++++++++
>>   include/hw/i386/ich9.h        |  2 ++
>>   include/hw/southbridge/piix.h |  3 +++
>>   8 files changed, 50 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index fa69b6f43e..d154eedcb3 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1299,7 +1299,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);
>> +
>> +    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);
>> +    }
>> +    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
>> +                              "date");
>I think you could turn now the "ISADevice **rtc_state" parameter of this function into a normal "ISADevice *rtc_state" since the pointer is not a return value anymore.

This is done in patch 9/30: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg03799.html

Best regards,
Bernhard

>
> Thomas
>

Re: [PATCH v4 08/30] hw/i386/pc: Create RTC controllers in south bridges

[Thomas Huth]

On 02/01/2023 19.25, Bernhard Beschow wrote:
> 
> 
> Am 2. Januar 2023 17:03:29 UTC schrieb Thomas Huth <thuth@redhat.com>:
>> On 21/12/2022 17.59, Bernhard Beschow wrote:
>>> Just like in the real hardware (and in PIIX4), create the RTC
>>> controllers in the south bridges.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-11-shentey@gmail.com>
>>> ---
>>>    hw/i386/pc.c                  | 12 +++++++++++-
>>>    hw/i386/pc_piix.c             |  8 ++++++++
>>>    hw/i386/pc_q35.c              |  1 +
>>>    hw/isa/Kconfig                |  2 ++
>>>    hw/isa/lpc_ich9.c             |  8 ++++++++
>>>    hw/isa/piix3.c                | 15 +++++++++++++++
>>>    include/hw/i386/ich9.h        |  2 ++
>>>    include/hw/southbridge/piix.h |  3 +++
>>>    8 files changed, 50 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index fa69b6f43e..d154eedcb3 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1299,7 +1299,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);
>>> +
>>> +    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);
>>> +    }
>>> +    object_property_add_alias(OBJECT(pcms), "rtc-time", OBJECT(*rtc_state),
>>> +                              "date");
>> I think you could turn now the "ISADevice **rtc_state" parameter of this function into a normal "ISADevice *rtc_state" since the pointer is not a return value anymore.
> 
> This is done in patch 9/30: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg03799.html

Ah, right, sorry for my shortsightness, then this is fine here:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 09/30] hw/i386/pc: No need for rtc_state to be an out-parameter

[Thomas Huth]

On 21/12/2022 17.59, Bernhard Beschow 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-12-shentey@gmail.com>
> ---
>   hw/i386/pc.c         | 12 ++++++------
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     |  2 +-
>   include/hw/i386/pc.h |  2 +-
>   4 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Philippe Mathieu-Daudé]

On 21/12/22 17:59, Bernhard Beschow wrote:
> Having a proxy PIC allows for ISA PICs to be created and wired up in
> southbridges. This is especially useful 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
> ---
>   MAINTAINERS                 |  2 ++
>   hw/core/Kconfig             |  3 ++
>   hw/core/meson.build         |  1 +
>   hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>   include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>   5 files changed, 130 insertions(+)
>   create mode 100644 hw/core/proxy-pic.c
>   create mode 100644 include/hw/core/proxy-pic.h

Please enable scripts/git.orderfile.

> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
> new file mode 100644
> index 0000000000..0eb40c478a
> --- /dev/null
> +++ b/include/hw/core/proxy-pic.h
> @@ -0,0 +1,54 @@
> +/*
> + * Proxy interrupt controller device.
> + *
> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.

This is the MIT license right? Do you mind adding a SPDX tag along?

  * SPDX-License-Identifier: MIT

> + */
> +
> +#ifndef HW_PROXY_PIC_H
> +#define HW_PROXY_PIC_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +#include "hw/irq.h"
> +
> +#define TYPE_PROXY_PIC "proxy-pic"
> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
> +
> +#define MAX_PROXY_PIC_LINES 16
> +
> +/**
> + * This is a simple device which has 16 pairs of GPIO input and output lines.
> + * Any change on an input line is forwarded to the respective output.
> + *
> + * QEMU interface:
> + *  + 16 unnamed GPIO inputs: the input lines
> + *  + 16 unnamed GPIO outputs: the output lines
> + */

Why restrict to 16 and not use a class property and allocate
on the heap? See TYPE_SPLIT_IRQ for example.

> +struct ProxyPICState {
> +    /*< private >*/
> +    struct DeviceState parent_obj;
> +    /*< public >*/
> +
> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
> +};
> +
> +#endif /* HW_PROXY_PIC_H */

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 21/12/22 17:59, Bernhard Beschow wrote:
>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>> southbridges. This is especially useful 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>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>> ---
>>   MAINTAINERS                 |  2 ++
>>   hw/core/Kconfig             |  3 ++
>>   hw/core/meson.build         |  1 +
>>   hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>   include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>   5 files changed, 130 insertions(+)
>>   create mode 100644 hw/core/proxy-pic.c
>>   create mode 100644 include/hw/core/proxy-pic.h
>
>Please enable scripts/git.orderfile.

Will do.

>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>> new file mode 100644
>> index 0000000000..0eb40c478a
>> --- /dev/null
>> +++ b/include/hw/core/proxy-pic.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Proxy interrupt controller device.
>> + *
>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>
>This is the MIT license right? Do you mind adding a SPDX tag along?

I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.

> * SPDX-License-Identifier: MIT

Or just replace the wall of text with this line? This should suffice, no?

>> + */
>> +
>> +#ifndef HW_PROXY_PIC_H
>> +#define HW_PROXY_PIC_H
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +#include "hw/irq.h"
>> +
>> +#define TYPE_PROXY_PIC "proxy-pic"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>> +
>> +#define MAX_PROXY_PIC_LINES 16
>> +
>> +/**
>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>> + * Any change on an input line is forwarded to the respective output.
>> + *
>> + * QEMU interface:
>> + *  + 16 unnamed GPIO inputs: the input lines
>> + *  + 16 unnamed GPIO outputs: the output lines
>> + */
>
>Why restrict to 16 and not use a class property and allocate
>on the heap? See TYPE_SPLIT_IRQ for example.

TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)

I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.

Thanks,
Bernhard

>
>> +struct ProxyPICState {
>> +    /*< private >*/
>> +    struct DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
>> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
>> +};
>> +
>> +#endif /* HW_PROXY_PIC_H */
>

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Philippe Mathieu-Daudé]

On 4/1/23 17:01, Bernhard Beschow wrote:
> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>> southbridges. This is especially useful 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

Typo "virtualization".

>>> populated with fully initialized qemu_irq's, they can already be wired
>>> up inside PIIX3.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>> ---
>>>    MAINTAINERS                 |  2 ++
>>>    hw/core/Kconfig             |  3 ++
>>>    hw/core/meson.build         |  1 +
>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>    5 files changed, 130 insertions(+)
>>>    create mode 100644 hw/core/proxy-pic.c
>>>    create mode 100644 include/hw/core/proxy-pic.h
>>
>> Please enable scripts/git.orderfile.
> 
> Will do.
> 
>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>> new file mode 100644
>>> index 0000000000..0eb40c478a
>>> --- /dev/null
>>> +++ b/include/hw/core/proxy-pic.h
>>> @@ -0,0 +1,54 @@
>>> +/*
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>
>> This is the MIT license right? Do you mind adding a SPDX tag along?
> 
> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.
> 
>> * SPDX-License-Identifier: MIT
> 
> Or just replace the wall of text with this line? This should suffice, no?

IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
chose, not ask you to remove the text; but since you ask/propose, the
tag suffices indeed. I suggest the tag use because it is clearer than
trying to match the full (often copy/pasted with typos) license text.

>>> + */
>>> +
>>> +#ifndef HW_PROXY_PIC_H
>>> +#define HW_PROXY_PIC_H
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +#include "hw/irq.h"
>>> +
>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>> +
>>> +#define MAX_PROXY_PIC_LINES 16
>>> +
>>> +/**
>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>> + * Any change on an input line is forwarded to the respective output.
>>> + *
>>> + * QEMU interface:
>>> + *  + 16 unnamed GPIO inputs: the input lines
>>> + *  + 16 unnamed GPIO outputs: the output lines
>>> + */
>>
>> Why restrict to 16 and not use a class property and allocate
>> on the heap? See TYPE_SPLIT_IRQ for example.
> 
> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
> 
> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.

Oh indeed. Well this model as is is OK, but it could be more useful
if able to proxy any range of IRQs.

I have the feeling we are cycling around this IRQ proxy:

22ec3283ef ("irq: introduce qemu_irq_proxy()")
078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in 
datasheet")
fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")

What is our problem? IRQ lines connect 2 devices in a fixed direction.
Current model expects one edge to be wired to a device before wiring
the other device, so device composition with IRQs in middle is
impossible? If so, this doesn't scale with dynamic machine creation.

Maybe the IRQ wiring should be another machine phase, after all
devices are instantiated?

Your approach is to create the 'IRQ proxy' first, like drawing the
wires on a board, then plug the devices, like soldering chips on
the printed board IRQs. So maybe devices shouldn't be the QOM owners
of IRQs, the board should...

Yeah, just thinking loudly...

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Mark Cave-Ayland]

On 04/01/2023 16:35, Philippe Mathieu-Daudé wrote:

> On 4/1/23 17:01, Bernhard Beschow wrote:
>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>> southbridges. This is especially useful 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
> 
> Typo "virtualization".
> 
>>>> populated with fully initialized qemu_irq's, they can already be wired
>>>> up inside PIIX3.
>>>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>>> ---
>>>>    MAINTAINERS                 |  2 ++
>>>>    hw/core/Kconfig             |  3 ++
>>>>    hw/core/meson.build         |  1 +
>>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>>    5 files changed, 130 insertions(+)
>>>>    create mode 100644 hw/core/proxy-pic.c
>>>>    create mode 100644 include/hw/core/proxy-pic.h
>>>
>>> Please enable scripts/git.orderfile.
>>
>> Will do.
>>
>>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>>> new file mode 100644
>>>> index 0000000000..0eb40c478a
>>>> --- /dev/null
>>>> +++ b/include/hw/core/proxy-pic.h
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * Proxy interrupt controller device.
>>>> + *
>>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>
>>> This is the MIT license right? Do you mind adding a SPDX tag along?
>>
>> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus 
>> preserved the license.
>>
>>> * SPDX-License-Identifier: MIT
>>
>> Or just replace the wall of text with this line? This should suffice, no?
> 
> IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
> chose, not ask you to remove the text; but since you ask/propose, the
> tag suffices indeed. I suggest the tag use because it is clearer than
> trying to match the full (often copy/pasted with typos) license text.
> 
>>>> + */
>>>> +
>>>> +#ifndef HW_PROXY_PIC_H
>>>> +#define HW_PROXY_PIC_H
>>>> +
>>>> +#include "hw/qdev-core.h"
>>>> +#include "qom/object.h"
>>>> +#include "hw/irq.h"
>>>> +
>>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>>> +
>>>> +#define MAX_PROXY_PIC_LINES 16
>>>> +
>>>> +/**
>>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>>> + * Any change on an input line is forwarded to the respective output.
>>>> + *
>>>> + * QEMU interface:
>>>> + *  + 16 unnamed GPIO inputs: the input lines
>>>> + *  + 16 unnamed GPIO outputs: the output lines
>>>> + */
>>>
>>> Why restrict to 16 and not use a class property and allocate
>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>
>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of 
>> MAX_SPLIT_LINES which equals 16 ;)
>>
>> I was unsure on when to free the memory and how to dispose the elements so I went 
>> with this solution for simplicity. I'll look for inspitation in other device models 
>> and respin.
> 
> Oh indeed. Well this model as is is OK, but it could be more useful
> if able to proxy any range of IRQs.
> 
> I have the feeling we are cycling around this IRQ proxy:
> 
> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
> 
> What is our problem? IRQ lines connect 2 devices in a fixed direction.
> Current model expects one edge to be wired to a device before wiring
> the other device, so device composition with IRQs in middle is
> impossible? If so, this doesn't scale with dynamic machine creation.
> 
> Maybe the IRQ wiring should be another machine phase, after all
> devices are instantiated?
> 
> Your approach is to create the 'IRQ proxy' first, like drawing the
> wires on a board, then plug the devices, like soldering chips on
> the printed board IRQs. So maybe devices shouldn't be the QOM owners
> of IRQs, the board should...
> 
> Yeah, just thinking loudly...

The main problem is that a lot of the old x86 code references QEMU IRQs directly 
instead of using qdev gpios, and so this proxy-pic device is a temporary glue which 
allows the x86 PIC board wiring to be done with qdev gpios, but still allow the 
various PIC implementations to access the QEMU IRQs directly as required.

One of my review comments from a previous version of the patch is that whilst this 
isn't a full qdev gpio conversion of all the x86 PIC implementations (which is likely 
a whole project in itself), there is a lot of good tidy-up work in this series. So as 
long as proxy-pic isn't directly exposed (for example, having qdev properties that 
are set by command line) I still think the series is worth merging in its current form.


ATB,

Mark.

Re: [PATCH v4 06/30] hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is created

[Bernhard Beschow]

Am 21. Dezember 2022 16:59:39 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Observe that the pci_map_irq_fn's don't depend on the south bridge
>instance. So associate them immediately when the PCI bus is created to
>keep things logically together.
>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>

This patch was new in v4 and therefore still lacks review. It could be squashed into https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03312.html for a cleaner history.

Best regards,
Bernhard
>---
> hw/i386/pc_piix.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index e4bb8994da..bfa7cb513b 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -229,6 +229,9 @@ static void pc_init1(MachineState *machine,
>                               x86ms->below_4g_mem_size,
>                               x86ms->above_4g_mem_size,
>                               pci_memory, ram_memory);
>+        pci_bus_map_irqs(pci_bus,
>+                         xen_enabled() ? xen_pci_slot_get_pirq
>+                                       : pci_slot_get_pirq);
>         pcms->bus = pci_bus;
> 
>         pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
>@@ -236,10 +239,6 @@ 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"));
>-
>-        pci_bus_map_irqs(pci_bus,
>-                         xen_enabled() ? xen_pci_slot_get_pirq
>-                                       : pci_slot_get_pirq);
>     } else {
>         pci_bus = NULL;
>         isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,

[PATCH] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Having a proxy PIC allows for ISA PICs to be created and wired up in
southbridges. This is especially useful 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
virtualization 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20221022150508.26830-15-shentey@gmail.com>
---
Changes since v4:
* Change license to GPL-2.0-or-later and use SPDX-License-Identifier
* Fix typo in commit message
---
 include/hw/core/proxy-pic.h | 38 ++++++++++++++++++++++++++
 hw/core/proxy-pic.c         | 54 +++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |  2 ++
 hw/core/Kconfig             |  3 +++
 hw/core/meson.build         |  1 +
 5 files changed, 98 insertions(+)
 create mode 100644 include/hw/core/proxy-pic.h
 create mode 100644 hw/core/proxy-pic.c

diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
new file mode 100644
index 0000000000..32bc7936bd
--- /dev/null
+++ b/include/hw/core/proxy-pic.h
@@ -0,0 +1,38 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
+ */
+
+#ifndef HW_PROXY_PIC_H
+#define HW_PROXY_PIC_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+#include "hw/irq.h"
+
+#define TYPE_PROXY_PIC "proxy-pic"
+OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
+
+#define MAX_PROXY_PIC_LINES 16
+
+/**
+ * This is a simple device which has 16 pairs of GPIO input and output lines.
+ * Any change on an input line is forwarded to the respective output.
+ *
+ * QEMU interface:
+ *  + 16 unnamed GPIO inputs: the input lines
+ *  + 16 unnamed GPIO outputs: the output lines
+ */
+struct ProxyPICState {
+    /*< private >*/
+    struct DeviceState parent_obj;
+    /*< public >*/
+
+    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
+    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
+};
+
+#endif /* HW_PROXY_PIC_H */
diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
new file mode 100644
index 0000000000..40fd70b9e2
--- /dev/null
+++ b/hw/core/proxy-pic.c
@@ -0,0 +1,54 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/proxy-pic.h"
+
+static void proxy_pic_set_irq(void *opaque, int irq, int level)
+{
+    ProxyPICState *s = opaque;
+
+    qemu_set_irq(s->out_irqs[irq], level);
+}
+
+static void proxy_pic_realize(DeviceState *dev, Error **errp)
+{
+    ProxyPICState *s = PROXY_PIC(dev);
+
+    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
+    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
+
+    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
+        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
+    }
+}
+
+static void proxy_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    /* No state to reset or migrate */
+    dc->realize = proxy_pic_realize;
+
+    /* Reason: Needs to be wired up to work */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo proxy_pic_info = {
+    .name          = TYPE_PROXY_PIC,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(ProxyPICState),
+    .class_init = proxy_pic_class_init,
+};
+
+static void split_irq_register_types(void)
+{
+    type_register_static(&proxy_pic_info);
+}
+
+type_init(split_irq_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a40d4d865..295a76bfbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1674,6 +1674,7 @@ S: Supported
 F: hw/char/debugcon.c
 F: hw/char/parallel*
 F: hw/char/serial*
+F: hw/core/proxy-pic.c
 F: hw/dma/i8257*
 F: hw/i2c/pm_smbus.c
 F: hw/input/pckbd.c
@@ -1690,6 +1691,7 @@ F: hw/watchdog/wdt_ib700.c
 F: hw/watchdog/wdt_i6300esb.c
 F: include/hw/display/vga.h
 F: include/hw/char/parallel.h
+F: include/hw/core/proxy-pic.h
 F: include/hw/dma/i8257.h
 F: include/hw/i2c/pm_smbus.h
 F: include/hw/input/i8042.h
diff --git a/hw/core/Kconfig b/hw/core/Kconfig
index 9397503656..a7224f4ca0 100644
--- a/hw/core/Kconfig
+++ b/hw/core/Kconfig
@@ -22,6 +22,9 @@ config OR_IRQ
 config PLATFORM_BUS
     bool
 
+config PROXY_PIC
+    bool
+
 config REGISTER
     bool
 
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 7a4d02b6c0..e86aef6ec3 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
 softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
 softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
 softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
+softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
 softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
 softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
-- 
2.39.0

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 4/1/23 17:01, Bernhard Beschow wrote:
>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>> southbridges. This is especially useful 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
>
>Typo "virtualization".

Fixed...

>>>> populated with fully initialized qemu_irq's, they can already be wired
>>>> up inside PIIX3.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>>> ---
>>>>    MAINTAINERS                 |  2 ++
>>>>    hw/core/Kconfig             |  3 ++
>>>>    hw/core/meson.build         |  1 +
>>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>>    5 files changed, 130 insertions(+)
>>>>    create mode 100644 hw/core/proxy-pic.c
>>>>    create mode 100644 include/hw/core/proxy-pic.h
>>> 
>>> Please enable scripts/git.orderfile.
>> 
>> Will do.
>> 
>>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>>> new file mode 100644
>>>> index 0000000000..0eb40c478a
>>>> --- /dev/null
>>>> +++ b/include/hw/core/proxy-pic.h
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * Proxy interrupt controller device.
>>>> + *
>>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>> 
>>> This is the MIT license right? Do you mind adding a SPDX tag along?
>> 
>> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.
>> 
>>> * SPDX-License-Identifier: MIT
>> 
>> Or just replace the wall of text with this line? This should suffice, no?
>
>IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
>chose, not ask you to remove the text; but since you ask/propose, the
>tag suffices indeed. I suggest the tag use because it is clearer than
>trying to match the full (often copy/pasted with typos) license text.

Changed...

>>>> + */
>>>> +
>>>> +#ifndef HW_PROXY_PIC_H
>>>> +#define HW_PROXY_PIC_H
>>>> +
>>>> +#include "hw/qdev-core.h"
>>>> +#include "qom/object.h"
>>>> +#include "hw/irq.h"
>>>> +
>>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>>> +
>>>> +#define MAX_PROXY_PIC_LINES 16
>>>> +
>>>> +/**
>>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>>> + * Any change on an input line is forwarded to the respective output.
>>>> + *
>>>> + * QEMU interface:
>>>> + *  + 16 unnamed GPIO inputs: the input lines
>>>> + *  + 16 unnamed GPIO outputs: the output lines
>>>> + */
>>> 
>>> Why restrict to 16 and not use a class property and allocate
>>> on the heap? See TYPE_SPLIT_IRQ for example.
>> 
>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>> 
>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>
>Oh indeed. Well this model as is is OK, but it could be more useful
>if able to proxy any range of IRQs.

I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?

>I have the feeling we are cycling around this IRQ proxy:
>
>22ec3283ef ("irq: introduce qemu_irq_proxy()")
>078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>
>What is our problem? IRQ lines connect 2 devices in a fixed direction.
>Current model expects one edge to be wired to a device before wiring
>the other device, so device composition with IRQs in middle is
>impossible? If so, this doesn't scale with dynamic machine creation.

My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.

Best regards,
Bernhard

>Maybe the IRQ wiring should be another machine phase, after all
>devices are instantiated?
>
>Your approach is to create the 'IRQ proxy' first, like drawing the
>wires on a board, then plug the devices, like soldering chips on
>the printed board IRQs. So maybe devices shouldn't be the QOM owners
>of IRQs, the board should...
>
>Yeah, just thinking loudly...

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Philippe Mathieu-Daudé]

On 4/1/23 21:12, Bernhard Beschow wrote:
> 
> 
> Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 4/1/23 17:01, Bernhard Beschow wrote:
>>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>>> southbridges. This is especially useful 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
>>
>> Typo "virtualization".
> 
> Fixed...


>>>> Why restrict to 16 and not use a class property and allocate
>>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>>
>>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>>>
>>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>>
>> Oh indeed. Well this model as is is OK, but it could be more useful
>> if able to proxy any range of IRQs.
> 
> I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?

I think we are good :) If you can point me to a branch with all your 
patches, I could verify everything is properly applied locally.

>> I have the feeling we are cycling around this IRQ proxy:
>>
>> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
>> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>>
>> What is our problem? IRQ lines connect 2 devices in a fixed direction.
>> Current model expects one edge to be wired to a device before wiring
>> the other device, so device composition with IRQs in middle is
>> impossible? If so, this doesn't scale with dynamic machine creation.
> 
> My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.

Yeah (I have been there too...). Also Mark Cave-Ayland confirmed
elsewhere in this thread that yourv effort points toward the right
direction :)

Regards,

Phil.

Re: [PATCH v4 12/30] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Am 4. Januar 2023 20:31:15 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 4/1/23 21:12, Bernhard Beschow wrote:
>> 
>> 
>> Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> On 4/1/23 17:01, Bernhard Beschow wrote:
>>>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>>>> southbridges. This is especially useful 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
>>> 
>>> Typo "virtualization".
>> 
>> Fixed...
>
>
>>>>> Why restrict to 16 and not use a class property and allocate
>>>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>>> 
>>>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>>>> 
>>>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>>> 
>>> Oh indeed. Well this model as is is OK, but it could be more useful
>>> if able to proxy any range of IRQs.
>> 
>> I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?
>
>I think we are good :) If you can point me to a branch with all your patches, I could verify everything is properly applied locally.

Sure, here we go: https://github.com/shentok/qemu/commits/piix-consolidate

Thanks for your help and for picking up this beast ;)

>
>>> I have the feeling we are cycling around this IRQ proxy:
>>> 
>>> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
>>> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>>> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>>> 
>>> What is our problem? IRQ lines connect 2 devices in a fixed direction.
>>> Current model expects one edge to be wired to a device before wiring
>>> the other device, so device composition with IRQs in middle is
>>> impossible? If so, this doesn't scale with dynamic machine creation.
>> 
>> My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.
>
>Yeah (I have been there too...).

I've seen it. Eventually I'll also pick up your work of eliminating the isabus global...

Best regards,
Bernhard

> Also Mark Cave-Ayland confirmed
>elsewhere in this thread that yourv effort points toward the right
>direction :)
>
>Regards,
>
>Phil.

Re: [PATCH] hw/core: Introduce proxy-pic

[Mark Cave-Ayland]

On 04/01/2023 19:53, Bernhard Beschow wrote:

> Having a proxy PIC allows for ISA PICs to be created and wired up in
> southbridges. This is especially useful 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
> virtualization 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
> ---
> Changes since v4:
> * Change license to GPL-2.0-or-later and use SPDX-License-Identifier
> * Fix typo in commit message
> ---
>   include/hw/core/proxy-pic.h | 38 ++++++++++++++++++++++++++
>   hw/core/proxy-pic.c         | 54 +++++++++++++++++++++++++++++++++++++
>   MAINTAINERS                 |  2 ++
>   hw/core/Kconfig             |  3 +++
>   hw/core/meson.build         |  1 +
>   5 files changed, 98 insertions(+)
>   create mode 100644 include/hw/core/proxy-pic.h
>   create mode 100644 hw/core/proxy-pic.c
> 
> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
> new file mode 100644
> index 0000000000..32bc7936bd
> --- /dev/null
> +++ b/include/hw/core/proxy-pic.h
> @@ -0,0 +1,38 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Proxy interrupt controller device.
> + *
> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
> + */
> +
> +#ifndef HW_PROXY_PIC_H
> +#define HW_PROXY_PIC_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +#include "hw/irq.h"
> +
> +#define TYPE_PROXY_PIC "proxy-pic"
> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
> +
> +#define MAX_PROXY_PIC_LINES 16
> +
> +/**
> + * This is a simple device which has 16 pairs of GPIO input and output lines.
> + * Any change on an input line is forwarded to the respective output.
> + *
> + * QEMU interface:
> + *  + 16 unnamed GPIO inputs: the input lines
> + *  + 16 unnamed GPIO outputs: the output lines
> + */

Re-reading this as a standalone patch, I can understand now why Phil was asking about 
device properties etc. because aside from the commit message, it isn't particularly 
clear that this is a workaround for QEMU's PIC devices and accelerator 
implementations not (yet) supporting direct wiring with qdev gpios. I would 
definitely argue that it is a special purpose and not a generic device.

I apologise that this is quite late in the review process, however given that this 
wasn't immediately clear I do think it is worth making a few minor changes. Perhaps 
something like:

- Update the comment above in proxy_pic.h clarifying that this is only for wiring up
   ISA PICs (similar to the commit message) until gpios can be used

- Move the .c and .h files from hw/core/proxy-pic.c and include/hw/core/proxy_pic.h
   to hw/i386/proxy-pic.c and include/hw/i386/proxy_pic.h to provide a strong hint
   that the device is restricted to x86-only

I think this makes it more obvious what the device is doing, and also prevent its 
usage leaking into other places in the codebase. In fact in its current form there is 
no need for device properties to configure the PIC lines, since legacy x86 PICs 
always have 16 (ISA) IRQ lines.

> +struct ProxyPICState {
> +    /*< private >*/
> +    struct DeviceState parent_obj;
> +    /*< public >*/
> +
> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
> +};
> +
> +#endif /* HW_PROXY_PIC_H */
> diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
> new file mode 100644
> index 0000000000..40fd70b9e2
> --- /dev/null
> +++ b/hw/core/proxy-pic.c
> @@ -0,0 +1,54 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Proxy interrupt controller device.
> + *
> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/proxy-pic.h"
> +
> +static void proxy_pic_set_irq(void *opaque, int irq, int level)
> +{
> +    ProxyPICState *s = opaque;
> +
> +    qemu_set_irq(s->out_irqs[irq], level);
> +}
> +
> +static void proxy_pic_realize(DeviceState *dev, Error **errp)
> +{
> +    ProxyPICState *s = PROXY_PIC(dev);
> +
> +    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
> +    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
> +
> +    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
> +        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
> +    }
> +}
> +
> +static void proxy_pic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    /* No state to reset or migrate */
> +    dc->realize = proxy_pic_realize;
> +
> +    /* Reason: Needs to be wired up to work */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo proxy_pic_info = {
> +    .name          = TYPE_PROXY_PIC,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(ProxyPICState),
> +    .class_init = proxy_pic_class_init,
> +};
> +
> +static void split_irq_register_types(void)
> +{
> +    type_register_static(&proxy_pic_info);
> +}
> +
> +type_init(split_irq_register_types)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a40d4d865..295a76bfbd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1674,6 +1674,7 @@ S: Supported
>   F: hw/char/debugcon.c
>   F: hw/char/parallel*
>   F: hw/char/serial*
> +F: hw/core/proxy-pic.c
>   F: hw/dma/i8257*
>   F: hw/i2c/pm_smbus.c
>   F: hw/input/pckbd.c
> @@ -1690,6 +1691,7 @@ F: hw/watchdog/wdt_ib700.c
>   F: hw/watchdog/wdt_i6300esb.c
>   F: include/hw/display/vga.h
>   F: include/hw/char/parallel.h
> +F: include/hw/core/proxy-pic.h
>   F: include/hw/dma/i8257.h
>   F: include/hw/i2c/pm_smbus.h
>   F: include/hw/input/i8042.h
> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
> index 9397503656..a7224f4ca0 100644
> --- a/hw/core/Kconfig
> +++ b/hw/core/Kconfig
> @@ -22,6 +22,9 @@ config OR_IRQ
>   config PLATFORM_BUS
>       bool
>   
> +config PROXY_PIC
> +    bool
> +
>   config REGISTER
>       bool
>   
> diff --git a/hw/core/meson.build b/hw/core/meson.build
> index 7a4d02b6c0..e86aef6ec3 100644
> --- a/hw/core/meson.build
> +++ b/hw/core/meson.build
> @@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
>   softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
>   softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
>   softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
> +softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
>   softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>   softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>   softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))


ATB,

Mark.

Re: [PATCH] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Am 4. Januar 2023 22:22:01 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 04/01/2023 19:53, Bernhard Beschow wrote:
>
>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>> southbridges. This is especially useful 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
>> virtualization 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>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>> ---
>> Changes since v4:
>> * Change license to GPL-2.0-or-later and use SPDX-License-Identifier
>> * Fix typo in commit message
>> ---
>>   include/hw/core/proxy-pic.h | 38 ++++++++++++++++++++++++++
>>   hw/core/proxy-pic.c         | 54 +++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                 |  2 ++
>>   hw/core/Kconfig             |  3 +++
>>   hw/core/meson.build         |  1 +
>>   5 files changed, 98 insertions(+)
>>   create mode 100644 include/hw/core/proxy-pic.h
>>   create mode 100644 hw/core/proxy-pic.c
>> 
>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>> new file mode 100644
>> index 0000000000..32bc7936bd
>> --- /dev/null
>> +++ b/include/hw/core/proxy-pic.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * Proxy interrupt controller device.
>> + *
>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>> + */
>> +
>> +#ifndef HW_PROXY_PIC_H
>> +#define HW_PROXY_PIC_H
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +#include "hw/irq.h"
>> +
>> +#define TYPE_PROXY_PIC "proxy-pic"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>> +
>> +#define MAX_PROXY_PIC_LINES 16
>> +
>> +/**
>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>> + * Any change on an input line is forwarded to the respective output.
>> + *
>> + * QEMU interface:
>> + *  + 16 unnamed GPIO inputs: the input lines
>> + *  + 16 unnamed GPIO outputs: the output lines
>> + */
>
>Re-reading this as a standalone patch, I can understand now why Phil was asking about device properties etc. because aside from the commit message, it isn't particularly clear that this is a workaround for QEMU's PIC devices and accelerator implementations not (yet) supporting direct wiring with qdev gpios. I would definitely argue that it is a special purpose and not a generic device.
>
>I apologise that this is quite late in the review process, however given that this wasn't immediately clear I do think it is worth making a few minor changes. Perhaps something like:
>
>- Update the comment above in proxy_pic.h clarifying that this is only for wiring up
>  ISA PICs (similar to the commit message) until gpios can be used

Will do.

>- Move the .c and .h files from hw/core/proxy-pic.c and include/hw/core/proxy_pic.h
>  to hw/i386/proxy-pic.c and include/hw/i386/proxy_pic.h to provide a strong hint
>  that the device is restricted to x86-only

The device gets used in PIIX4 as well, i.e. MIPS, too. I therefore think it is not x86 but rather PIC specific. I propose to move it back to hw/intc/i8259 where it was implemented until v2: https://patchew.org/QEMU/20221022150508.26830-1-shentey@gmail.com/20221022150508.26830-15-shentey@gmail.com/ . I can also rename the device back to isa-pic to make things more obvious.

What do you think?

Best regards,
Bernhard

>
>I think this makes it more obvious what the device is doing, and also prevent its usage leaking into other places in the codebase. In fact in its current form there is no need for device properties to configure the PIC lines, since legacy x86 PICs always have 16 (ISA) IRQ lines.
>
>> +struct ProxyPICState {
>> +    /*< private >*/
>> +    struct DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
>> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
>> +};
>> +
>> +#endif /* HW_PROXY_PIC_H */
>> diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
>> new file mode 100644
>> index 0000000000..40fd70b9e2
>> --- /dev/null
>> +++ b/hw/core/proxy-pic.c
>> @@ -0,0 +1,54 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * Proxy interrupt controller device.
>> + *
>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/core/proxy-pic.h"
>> +
>> +static void proxy_pic_set_irq(void *opaque, int irq, int level)
>> +{
>> +    ProxyPICState *s = opaque;
>> +
>> +    qemu_set_irq(s->out_irqs[irq], level);
>> +}
>> +
>> +static void proxy_pic_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ProxyPICState *s = PROXY_PIC(dev);
>> +
>> +    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
>> +    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
>> +
>> +    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
>> +        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
>> +    }
>> +}
>> +
>> +static void proxy_pic_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    /* No state to reset or migrate */
>> +    dc->realize = proxy_pic_realize;
>> +
>> +    /* Reason: Needs to be wired up to work */
>> +    dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo proxy_pic_info = {
>> +    .name          = TYPE_PROXY_PIC,
>> +    .parent        = TYPE_DEVICE,
>> +    .instance_size = sizeof(ProxyPICState),
>> +    .class_init = proxy_pic_class_init,
>> +};
>> +
>> +static void split_irq_register_types(void)
>> +{
>> +    type_register_static(&proxy_pic_info);
>> +}
>> +
>> +type_init(split_irq_register_types)
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7a40d4d865..295a76bfbd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1674,6 +1674,7 @@ S: Supported
>>   F: hw/char/debugcon.c
>>   F: hw/char/parallel*
>>   F: hw/char/serial*
>> +F: hw/core/proxy-pic.c
>>   F: hw/dma/i8257*
>>   F: hw/i2c/pm_smbus.c
>>   F: hw/input/pckbd.c
>> @@ -1690,6 +1691,7 @@ F: hw/watchdog/wdt_ib700.c
>>   F: hw/watchdog/wdt_i6300esb.c
>>   F: include/hw/display/vga.h
>>   F: include/hw/char/parallel.h
>> +F: include/hw/core/proxy-pic.h
>>   F: include/hw/dma/i8257.h
>>   F: include/hw/i2c/pm_smbus.h
>>   F: include/hw/input/i8042.h
>> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
>> index 9397503656..a7224f4ca0 100644
>> --- a/hw/core/Kconfig
>> +++ b/hw/core/Kconfig
>> @@ -22,6 +22,9 @@ config OR_IRQ
>>   config PLATFORM_BUS
>>       bool
>>   +config PROXY_PIC
>> +    bool
>> +
>>   config REGISTER
>>       bool
>>   diff --git a/hw/core/meson.build b/hw/core/meson.build
>> index 7a4d02b6c0..e86aef6ec3 100644
>> --- a/hw/core/meson.build
>> +++ b/hw/core/meson.build
>> @@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
>>   softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
>>   softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
>>   softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
>> +softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
>>   softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>>   softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>>   softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
>
>
>ATB,
>
>Mark.

Re: [PATCH] hw/core: Introduce proxy-pic

[Bernhard Beschow]

Am 5. Januar 2023 09:50:03 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 4. Januar 2023 22:22:01 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>On 04/01/2023 19:53, Bernhard Beschow wrote:
>>
>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>> southbridges. This is especially useful 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
>>> virtualization 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>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>> ---
>>> Changes since v4:
>>> * Change license to GPL-2.0-or-later and use SPDX-License-Identifier
>>> * Fix typo in commit message
>>> ---
>>>   include/hw/core/proxy-pic.h | 38 ++++++++++++++++++++++++++
>>>   hw/core/proxy-pic.c         | 54 +++++++++++++++++++++++++++++++++++++
>>>   MAINTAINERS                 |  2 ++
>>>   hw/core/Kconfig             |  3 +++
>>>   hw/core/meson.build         |  1 +
>>>   5 files changed, 98 insertions(+)
>>>   create mode 100644 include/hw/core/proxy-pic.h
>>>   create mode 100644 hw/core/proxy-pic.c
>>> 
>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>> new file mode 100644
>>> index 0000000000..32bc7936bd
>>> --- /dev/null
>>> +++ b/include/hw/core/proxy-pic.h
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>> + */
>>> +
>>> +#ifndef HW_PROXY_PIC_H
>>> +#define HW_PROXY_PIC_H
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +#include "hw/irq.h"
>>> +
>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>> +
>>> +#define MAX_PROXY_PIC_LINES 16
>>> +
>>> +/**
>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>> + * Any change on an input line is forwarded to the respective output.
>>> + *
>>> + * QEMU interface:
>>> + *  + 16 unnamed GPIO inputs: the input lines
>>> + *  + 16 unnamed GPIO outputs: the output lines
>>> + */
>>
>>Re-reading this as a standalone patch, I can understand now why Phil was asking about device properties etc. because aside from the commit message, it isn't particularly clear that this is a workaround for QEMU's PIC devices and accelerator implementations not (yet) supporting direct wiring with qdev gpios. I would definitely argue that it is a special purpose and not a generic device.
>>
>>I apologise that this is quite late in the review process, however given that this wasn't immediately clear I do think it is worth making a few minor changes. Perhaps something like:
>>
>>- Update the comment above in proxy_pic.h clarifying that this is only for wiring up
>>  ISA PICs (similar to the commit message) until gpios can be used
>
>Will do.
>
>>- Move the .c and .h files from hw/core/proxy-pic.c and include/hw/core/proxy_pic.h
>>  to hw/i386/proxy-pic.c and include/hw/i386/proxy_pic.h to provide a strong hint
>>  that the device is restricted to x86-only
>
>The device gets used in PIIX4 as well, i.e. MIPS, too. I therefore think it is not x86 but rather PIC specific. I propose to move it back to hw/intc/i8259 where it was implemented until v2: https://patchew.org/QEMU/20221022150508.26830-1-shentey@gmail.com/20221022150508.26830-15-shentey@gmail.com/ . I can also rename the device back to isa-pic to make things more obvious.

I've submitted v5 of the series. Mark, would you be available for review?

For Phil's convenience I've pushed a git tag here: https://github.com/shentok/qemu/commits/piix-consolidate-v5

Best regards,
Bernhard
>
>What do you think?
>
>Best regards,
>Bernhard
>
>>
>>I think this makes it more obvious what the device is doing, and also prevent its usage leaking into other places in the codebase. In fact in its current form there is no need for device properties to configure the PIC lines, since legacy x86 PICs always have 16 (ISA) IRQ lines.
>>
>>> +struct ProxyPICState {
>>> +    /*< private >*/
>>> +    struct DeviceState parent_obj;
>>> +    /*< public >*/
>>> +
>>> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
>>> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
>>> +};
>>> +
>>> +#endif /* HW_PROXY_PIC_H */
>>> diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
>>> new file mode 100644
>>> index 0000000000..40fd70b9e2
>>> --- /dev/null
>>> +++ b/hw/core/proxy-pic.c
>>> @@ -0,0 +1,54 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/core/proxy-pic.h"
>>> +
>>> +static void proxy_pic_set_irq(void *opaque, int irq, int level)
>>> +{
>>> +    ProxyPICState *s = opaque;
>>> +
>>> +    qemu_set_irq(s->out_irqs[irq], level);
>>> +}
>>> +
>>> +static void proxy_pic_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    ProxyPICState *s = PROXY_PIC(dev);
>>> +
>>> +    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
>>> +    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
>>> +
>>> +    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
>>> +        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
>>> +    }
>>> +}
>>> +
>>> +static void proxy_pic_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    /* No state to reset or migrate */
>>> +    dc->realize = proxy_pic_realize;
>>> +
>>> +    /* Reason: Needs to be wired up to work */
>>> +    dc->user_creatable = false;
>>> +}
>>> +
>>> +static const TypeInfo proxy_pic_info = {
>>> +    .name          = TYPE_PROXY_PIC,
>>> +    .parent        = TYPE_DEVICE,
>>> +    .instance_size = sizeof(ProxyPICState),
>>> +    .class_init = proxy_pic_class_init,
>>> +};
>>> +
>>> +static void split_irq_register_types(void)
>>> +{
>>> +    type_register_static(&proxy_pic_info);
>>> +}
>>> +
>>> +type_init(split_irq_register_types)
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 7a40d4d865..295a76bfbd 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1674,6 +1674,7 @@ S: Supported
>>>   F: hw/char/debugcon.c
>>>   F: hw/char/parallel*
>>>   F: hw/char/serial*
>>> +F: hw/core/proxy-pic.c
>>>   F: hw/dma/i8257*
>>>   F: hw/i2c/pm_smbus.c
>>>   F: hw/input/pckbd.c
>>> @@ -1690,6 +1691,7 @@ F: hw/watchdog/wdt_ib700.c
>>>   F: hw/watchdog/wdt_i6300esb.c
>>>   F: include/hw/display/vga.h
>>>   F: include/hw/char/parallel.h
>>> +F: include/hw/core/proxy-pic.h
>>>   F: include/hw/dma/i8257.h
>>>   F: include/hw/i2c/pm_smbus.h
>>>   F: include/hw/input/i8042.h
>>> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
>>> index 9397503656..a7224f4ca0 100644
>>> --- a/hw/core/Kconfig
>>> +++ b/hw/core/Kconfig
>>> @@ -22,6 +22,9 @@ config OR_IRQ
>>>   config PLATFORM_BUS
>>>       bool
>>>   +config PROXY_PIC
>>> +    bool
>>> +
>>>   config REGISTER
>>>       bool
>>>   diff --git a/hw/core/meson.build b/hw/core/meson.build
>>> index 7a4d02b6c0..e86aef6ec3 100644
>>> --- a/hw/core/meson.build
>>> +++ b/hw/core/meson.build
>>> @@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
>>>   softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
>>>   softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
>>>   softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
>>> +softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
>>>   softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>>>   softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>>>   softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
>>
>>
>>ATB,
>>
>>Mark.