[PATCH 00/12] hw/acpi/piix4: remove legacy piix4_pm_init() function

[PATCH 00/12] hw/acpi/piix4: remove legacy piix4_pm_init() function

[Mark Cave-Ayland]

Whilst reviewing Bernhard's PIIX Southbridge QOMifcation patches at 
https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04329.html, I
noticed that we should first eliminate the legacy device init function
piix4_pm_init().

This series moves the outstanding logic from piix4_pm_init() into the
relevant instance init() and realize() functions, changes the IRQs to
use qdev gpios, and then finally removes the now-unused piix4_pm_initfn()
function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (12):
  hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to
    piix4_pm_realize()
  hw/acpi/piix4: change smm_enabled from int to bool
  hw/acpi/piix4: convert smm_enabled bool to qdev property
  hw/acpi/piix4: move PIIX4PMState into separate piix4.h header
  hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState
  hw/acpi/piix4: rename piix4_pm_init() to piix4_pm_initfn()
  hw/acpi/piix4: introduce piix4_pm_init() instance init function
  hw/acpi/piix4: use qdev gpio to wire up sci_irq
  hw/acpi/piix4: use qdev gpio to wire up smi_irq
  hw/i386/pc_piix: create PIIX4_PM device directly instead of using
    piix4_pm_initfn()
  hw/isa/piix4.c: create PIIX4_PM device directly instead of using
    piix4_pm_initfn()
  hw/acpi/piix4: remove unused piix4_pm_initfn() function

 hw/acpi/piix4.c               | 77 ++++++-----------------------------
 hw/i386/acpi-build.c          |  1 +
 hw/i386/pc_piix.c             | 16 +++++---
 hw/isa/piix4.c                | 11 +++--
 include/hw/acpi/piix4.h       | 75 ++++++++++++++++++++++++++++++++++
 include/hw/southbridge/piix.h |  6 ---
 6 files changed, 107 insertions(+), 79 deletions(-)
 create mode 100644 include/hw/acpi/piix4.h

-- 
2.20.1

[PATCH 01/12] hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to piix4_pm_realize()

[Mark Cave-Ayland]

This logic can be included as part of piix4_pm_realize() and does not need to
be handled externally.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index fe5625d07a..bf20fa139b 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -525,6 +525,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     s->machine_ready.notify = piix4_pm_machine_ready;
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 
+    if (xen_enabled()) {
+        s->use_acpi_hotplug_bridge = false;
+    }
+
     piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
                                    pci_get_bus(dev), s);
     qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
@@ -551,9 +555,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     s->irq = sci_irq;
     s->smi_irq = smi_irq;
     s->smm_enabled = smm_enabled;
-    if (xen_enabled()) {
-        s->use_acpi_hotplug_bridge = false;
-    }
 
     pci_realize_and_unref(pci_dev, bus, &error_fatal);
 
-- 
2.20.1

[PATCH 02/12] hw/acpi/piix4: change smm_enabled from int to bool

[Mark Cave-Ayland]

This is in preparation for conversion to a qdev property.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index bf20fa139b..fcfaafc175 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -74,7 +74,7 @@ struct PIIX4PMState {
 
     qemu_irq irq;
     qemu_irq smi_irq;
-    int smm_enabled;
+    bool smm_enabled;
     bool smm_compat;
     Notifier machine_ready;
     Notifier powerdown_notifier;
-- 
2.20.1

[PATCH 03/12] hw/acpi/piix4: convert smm_enabled bool to qdev property

[Mark Cave-Ayland]

This allows the smm_enabled value to be set using a standard qdev property instead
of being referenced directly in piix4_pm_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index fcfaafc175..2735ff375e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -547,6 +547,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     pci_dev = pci_new(devfn, TYPE_PIIX4_PM);
     dev = DEVICE(pci_dev);
     qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
+    qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
     if (piix4_pm) {
         *piix4_pm = dev;
     }
@@ -554,7 +555,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     s = PIIX4_PM(dev);
     s->irq = sci_irq;
     s->smi_irq = smi_irq;
-    s->smm_enabled = smm_enabled;
 
     pci_realize_and_unref(pci_dev, bus, &error_fatal);
 
@@ -664,6 +664,7 @@ static Property piix4_pm_properties[] = {
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),
     DEFINE_PROP_BOOL("smm-compat", PIIX4PMState, smm_compat, false),
+    DEFINE_PROP_BOOL("smm-enabled", PIIX4PMState, smm_enabled, false),
     DEFINE_PROP_BOOL("x-not-migrate-acpi-index", PIIX4PMState,
                       not_migrate_acpi_index, false),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.20.1

[PATCH 04/12] hw/acpi/piix4: move PIIX4PMState into separate piix4.h header

[Mark Cave-Ayland]

This allows the QOM types in hw/acpi/piix4.c to be used elsewhere by simply including
hw/acpi/piix4.h.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 43 +-------------------
 hw/i386/acpi-build.c          |  1 +
 include/hw/acpi/piix4.h       | 75 +++++++++++++++++++++++++++++++++++
 include/hw/southbridge/piix.h |  2 -
 4 files changed, 78 insertions(+), 43 deletions(-)
 create mode 100644 include/hw/acpi/piix4.h

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2735ff375e..7ee65b1bff 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -28,6 +28,8 @@
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/pcihp.h"
+#include "hw/acpi/piix4.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/xen.h"
@@ -56,47 +58,6 @@ struct pci_status {
     uint32_t down;
 };
 
-struct PIIX4PMState {
-    /*< private >*/
-    PCIDevice parent_obj;
-    /*< public >*/
-
-    MemoryRegion io;
-    uint32_t io_base;
-
-    MemoryRegion io_gpe;
-    ACPIREGS ar;
-
-    APMState apm;
-
-    PMSMBus smb;
-    uint32_t smb_io_base;
-
-    qemu_irq irq;
-    qemu_irq smi_irq;
-    bool smm_enabled;
-    bool smm_compat;
-    Notifier machine_ready;
-    Notifier powerdown_notifier;
-
-    AcpiPciHpState acpi_pci_hotplug;
-    bool use_acpi_hotplug_bridge;
-    bool use_acpi_root_pci_hotplug;
-    bool not_migrate_acpi_index;
-
-    uint8_t disable_s3;
-    uint8_t disable_s4;
-    uint8_t s4_val;
-
-    bool cpu_hotplug_legacy;
-    AcpiCpuHotplug gpe_cpu;
-    CPUHotplugState cpuhp_state;
-
-    MemHotplugState acpi_memory_hotplug;
-};
-
-OBJECT_DECLARE_SIMPLE_TYPE(PIIX4PMState, PIIX4_PM)
-
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                                            PCIBus *bus, PIIX4PMState *s);
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c125939ed6..89ac326d7f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -46,6 +46,7 @@
 #include "hw/acpi/tpm.h"
 #include "hw/acpi/vmgenid.h"
 #include "hw/acpi/erst.h"
+#include "hw/acpi/piix4.h"
 #include "sysemu/tpm_backend.h"
 #include "hw/rtc/mc146818rtc_regs.h"
 #include "migration/vmstate.h"
diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
new file mode 100644
index 0000000000..32686a75c5
--- /dev/null
+++ b/include/hw/acpi/piix4.h
@@ -0,0 +1,75 @@
+/*
+ * ACPI implementation
+ *
+ * Copyright (c) 2006 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2.1 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#ifndef HW_ACPI_PIIX4_H
+#define HW_ACPI_PIIX4_H
+
+#include "hw/pci/pci.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/cpu_hotplug.h"
+#include "hw/acpi/memory_hotplug.h"
+#include "hw/acpi/pcihp.h"
+#include "hw/i2c/pm_smbus.h"
+#include "hw/isa/apm.h"
+
+#define TYPE_PIIX4_PM "PIIX4_PM"
+OBJECT_DECLARE_SIMPLE_TYPE(PIIX4PMState, PIIX4_PM)
+
+struct PIIX4PMState {
+    /*< private >*/
+    PCIDevice parent_obj;
+    /*< public >*/
+
+    MemoryRegion io;
+    uint32_t io_base;
+
+    MemoryRegion io_gpe;
+    ACPIREGS ar;
+
+    APMState apm;
+
+    PMSMBus smb;
+    uint32_t smb_io_base;
+
+    qemu_irq irq;
+    qemu_irq smi_irq;
+    bool smm_enabled;
+    bool smm_compat;
+    Notifier machine_ready;
+    Notifier powerdown_notifier;
+
+    AcpiPciHpState acpi_pci_hotplug;
+    bool use_acpi_hotplug_bridge;
+    bool use_acpi_root_pci_hotplug;
+    bool not_migrate_acpi_index;
+
+    uint8_t disable_s3;
+    uint8_t disable_s4;
+    uint8_t s4_val;
+
+    bool cpu_hotplug_legacy;
+    AcpiCpuHotplug gpe_cpu;
+    CPUHotplugState cpuhp_state;
+
+    MemHotplugState acpi_memory_hotplug;
+};
+
+#endif
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index f63f83e5c6..c5b842b45d 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -15,8 +15,6 @@
 #include "hw/pci/pci.h"
 #include "qom/object.h"
 
-#define TYPE_PIIX4_PM "PIIX4_PM"
-
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                       qemu_irq sci_irq, qemu_irq smi_irq,
                       int smm_enabled, DeviceState **piix4_pm);
-- 
2.20.1

[PATCH 05/12] hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState

[Mark Cave-Ayland]

This exposes the PIIX4_PM device to the caller to allow any qdev gpios to be
mapped outside of piix4_pm_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 11 ++++-------
 hw/i386/pc_piix.c             | 10 +++++-----
 hw/isa/piix4.c                |  8 +++++---
 include/hw/southbridge/piix.h |  7 ++++---
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 7ee65b1bff..05c129b6af 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -497,9 +497,9 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     piix4_pm_add_properties(s);
 }
 
-I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                      qemu_irq sci_irq, qemu_irq smi_irq,
-                      int smm_enabled, DeviceState **piix4_pm)
+PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
+                            qemu_irq sci_irq, qemu_irq smi_irq,
+                            int smm_enabled)
 {
     PCIDevice *pci_dev;
     DeviceState *dev;
@@ -509,9 +509,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     dev = DEVICE(pci_dev);
     qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
     qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
-    if (piix4_pm) {
-        *piix4_pm = dev;
-    }
 
     s = PIIX4_PM(dev);
     s->irq = sci_irq;
@@ -519,7 +516,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 
     pci_realize_and_unref(pci_dev, bus, &error_fatal);
 
-    return s->smb.smbus;
+    return s;
 }
 
 static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 578e537b35..0300be5ef4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -281,14 +281,14 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
-        DeviceState *piix4_pm;
+        PIIX4PMState *piix4_pm;
 
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
-        pcms->smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-                                    x86ms->gsi[9], smi_irq,
-                                    x86_machine_is_smm_enabled(x86ms),
-                                    &piix4_pm);
+        piix4_pm = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
+                                 x86ms->gsi[9], smi_irq,
+                                 x86_machine_is_smm_enabled(x86ms));
+        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
         object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 8607e0ac36..7d9bedd1bb 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -293,6 +293,7 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
     PIIX4State *s;
+    PIIX4PMState *pms;
     PCIDevice *pci;
     DeviceState *dev;
     int devfn = PCI_DEVFN(10, 0);
@@ -310,9 +311,10 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
-        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
-                               qdev_get_gpio_in_named(dev, "isa", 9),
-                               NULL, 0, NULL);
+        pms = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
+                            qdev_get_gpio_in_named(dev, "isa", 9),
+                            NULL, 0);
+        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
     }
 
     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index c5b842b45d..108800ab06 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -14,10 +14,11 @@
 
 #include "hw/pci/pci.h"
 #include "qom/object.h"
+#include "hw/acpi/piix4.h"
 
-I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                      qemu_irq sci_irq, qemu_irq smi_irq,
-                      int smm_enabled, DeviceState **piix4_pm);
+PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
+                            qemu_irq sci_irq, qemu_irq smi_irq,
+                            int smm_enabled);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
-- 
2.20.1

[PATCH 06/12] hw/acpi/piix4: rename piix4_pm_init() to piix4_pm_initfn()

[Mark Cave-Ayland]

When QOMifying a device it is typical to use _init() as the suffix for an
instance_init function, however this name is already in use by the legacy
piix4_pm_init() wrapper function. Eventually the wrapper function will be
removed, but for now rename it to piix4_pm_initfn() to avoid a naming
collision.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 6 +++---
 hw/i386/pc_piix.c             | 6 +++---
 hw/isa/piix4.c                | 6 +++---
 include/hw/southbridge/piix.h | 6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 05c129b6af..d897d2dee6 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -497,9 +497,9 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     piix4_pm_add_properties(s);
 }
 
-PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                            qemu_irq sci_irq, qemu_irq smi_irq,
-                            int smm_enabled)
+PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
+                              qemu_irq sci_irq, qemu_irq smi_irq,
+                              int smm_enabled)
 {
     PCIDevice *pci_dev;
     DeviceState *dev;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0300be5ef4..ae21946981 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -285,9 +285,9 @@ static void pc_init1(MachineState *machine,
 
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
-        piix4_pm = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-                                 x86ms->gsi[9], smi_irq,
-                                 x86_machine_is_smm_enabled(x86ms));
+        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100,
+                                   x86ms->gsi[9], smi_irq,
+                                   x86_machine_is_smm_enabled(x86ms));
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 7d9bedd1bb..33a7015ea3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -311,9 +311,9 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
-        pms = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
-                            qdev_get_gpio_in_named(dev, "isa", 9),
-                            NULL, 0);
+        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100,
+                              qdev_get_gpio_in_named(dev, "isa", 9),
+                              NULL, 0);
         *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
     }
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 108800ab06..479d88e242 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -16,9 +16,9 @@
 #include "qom/object.h"
 #include "hw/acpi/piix4.h"
 
-PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                            qemu_irq sci_irq, qemu_irq smi_irq,
-                            int smm_enabled);
+PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
+                              qemu_irq sci_irq, qemu_irq smi_irq,
+                              int smm_enabled);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
-- 
2.20.1

[PATCH 07/12] hw/acpi/piix4: introduce piix4_pm_init() instance init function

[Mark Cave-Ayland]

Use the new piix4_pm_init() instance init function to initialise 2 separate qdev
gpios for the SCI and SMI IRQs.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index d897d2dee6..454fa34df1 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -497,6 +497,14 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     piix4_pm_add_properties(s);
 }
 
+static void piix4_pm_init(Object *obj)
+{
+    PIIX4PMState *s = PIIX4_PM(obj);
+
+    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
+}
+
 PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
                               qemu_irq sci_irq, qemu_irq smi_irq,
                               int smm_enabled)
@@ -663,6 +671,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix4_pm_info = {
     .name          = TYPE_PIIX4_PM,
     .parent        = TYPE_PCI_DEVICE,
+    .instance_init  = piix4_pm_init,
     .instance_size = sizeof(PIIX4PMState),
     .class_init    = piix4_pm_class_init,
     .interfaces = (InterfaceInfo[]) {
-- 
2.20.1

[PATCH 08/12] hw/acpi/piix4: use qdev gpio to wire up sci_irq

[Mark Cave-Ayland]

The sci_irq can now be wired up directly using a qdev gpio instead of having to
set the IRQ externally in piix4_pm_initfn().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 4 +---
 hw/i386/pc_piix.c             | 4 ++--
 hw/isa/piix4.c                | 6 +++---
 include/hw/southbridge/piix.h | 3 +--
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 454fa34df1..141852b7d4 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -506,8 +506,7 @@ static void piix4_pm_init(Object *obj)
 }
 
 PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              qemu_irq sci_irq, qemu_irq smi_irq,
-                              int smm_enabled)
+                              qemu_irq smi_irq, int smm_enabled)
 {
     PCIDevice *pci_dev;
     DeviceState *dev;
@@ -519,7 +518,6 @@ PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
     qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
 
     s = PIIX4_PM(dev);
-    s->irq = sci_irq;
     s->smi_irq = smi_irq;
 
     pci_realize_and_unref(pci_dev, bus, &error_fatal);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ae21946981..21e6159166 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -285,9 +285,9 @@ static void pc_init1(MachineState *machine,
 
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
-        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100,
-                                   x86ms->gsi[9], smi_irq,
+        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100, smi_irq,
                                    x86_machine_is_smm_enabled(x86ms));
+        qdev_connect_gpio_out(DEVICE(piix4_pm), 0, x86ms->gsi[9]);
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 33a7015ea3..0b6ea22143 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -311,9 +311,9 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
-        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100,
-                              qdev_get_gpio_in_named(dev, "isa", 9),
-                              NULL, 0);
+        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100, NULL, 0);
+        qdev_connect_gpio_out(DEVICE(pms), 0,
+                              qdev_get_gpio_in_named(dev, "isa", 9));
         *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
     }
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 479d88e242..aefdaebc3f 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -17,8 +17,7 @@
 #include "hw/acpi/piix4.h"
 
 PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              qemu_irq sci_irq, qemu_irq smi_irq,
-                              int smm_enabled);
+                              qemu_irq smi_irq, int smm_enabled);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
-- 
2.20.1

[PATCH 09/12] hw/acpi/piix4: use qdev gpio to wire up smi_irq

[Mark Cave-Ayland]

The smi_irq can now be wired up directly using a qdev gpio instead of having to
set the IRQ externally in piix4_pm_initfn().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 3 +--
 hw/i386/pc_piix.c             | 3 ++-
 hw/isa/piix4.c                | 2 +-
 include/hw/southbridge/piix.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 141852b7d4..fd20e1eccc 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -506,7 +506,7 @@ static void piix4_pm_init(Object *obj)
 }
 
 PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              qemu_irq smi_irq, int smm_enabled)
+                              int smm_enabled)
 {
     PCIDevice *pci_dev;
     DeviceState *dev;
@@ -518,7 +518,6 @@ PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
     qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
 
     s = PIIX4_PM(dev);
-    s->smi_irq = smi_irq;
 
     pci_realize_and_unref(pci_dev, bus, &error_fatal);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 21e6159166..82696fc707 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -285,9 +285,10 @@ static void pc_init1(MachineState *machine,
 
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
-        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100, smi_irq,
+        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100,
                                    x86_machine_is_smm_enabled(x86ms));
         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"));
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0b6ea22143..775e15eb20 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -311,7 +311,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
-        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100, NULL, 0);
+        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100, 0);
         qdev_connect_gpio_out(DEVICE(pms), 0,
                               qdev_get_gpio_in_named(dev, "isa", 9));
         *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index aefdaebc3f..d284475f07 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -17,7 +17,7 @@
 #include "hw/acpi/piix4.h"
 
 PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              qemu_irq smi_irq, int smm_enabled);
+                              int smm_enabled);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
-- 
2.20.1

[PATCH 10/12] hw/i386/pc_piix: create PIIX4_PM device directly instead of using piix4_pm_initfn()

[Mark Cave-Ayland]

Now that all external logic has been removed from piix4_pm_initfn() the PIIX4_PM
device can be instantiated directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/i386/pc_piix.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 82696fc707..4120fd52e5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -47,6 +47,7 @@
 #include "hw/xen/xen-x86.h"
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/piix4.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/xen.h"
@@ -281,12 +282,16 @@ static void pc_init1(MachineState *machine,
     }
 
     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
-        PIIX4PMState *piix4_pm;
+        PCIDevice *piix4_pm;
 
         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
         /* TODO: Populate SPD eeprom data.  */
-        piix4_pm = piix4_pm_initfn(pci_bus, piix3_devfn + 3, 0xb100,
-                                   x86_machine_is_smm_enabled(x86ms));
+        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);
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
-- 
2.20.1

[PATCH 11/12] hw/isa/piix4.c: create PIIX4_PM device directly instead of using piix4_pm_initfn()

[Mark Cave-Ayland]

Now that all external logic has been removed from piix4_pm_initfn() the PIIX4_PM
device can be instantiated directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/isa/piix4.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 775e15eb20..9a6d981037 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -34,6 +34,7 @@
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/ide/pci.h"
+#include "hw/acpi/piix4.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
@@ -293,7 +294,6 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
     PIIX4State *s;
-    PIIX4PMState *pms;
     PCIDevice *pci;
     DeviceState *dev;
     int devfn = PCI_DEVFN(10, 0);
@@ -311,10 +311,13 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
-        pms = piix4_pm_initfn(pci_bus, devfn + 3, 0x1100, 0);
-        qdev_connect_gpio_out(DEVICE(pms), 0,
+        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
+        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
+        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
+        pci_realize_and_unref(pci, pci_bus, &error_fatal);
+        qdev_connect_gpio_out(DEVICE(pci), 0,
                               qdev_get_gpio_in_named(dev, "isa", 9));
-        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
+        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
     }
 
     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
-- 
2.20.1

[PATCH 12/12] hw/acpi/piix4: remove unused piix4_pm_initfn() function

[Mark Cave-Ayland]

This function is now unused and so can be completely removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/acpi/piix4.c               | 19 -------------------
 include/hw/southbridge/piix.h |  4 ----
 2 files changed, 23 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index fd20e1eccc..0a81f1ad93 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -505,25 +505,6 @@ static void piix4_pm_init(Object *obj)
     qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
 }
 
-PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              int smm_enabled)
-{
-    PCIDevice *pci_dev;
-    DeviceState *dev;
-    PIIX4PMState *s;
-
-    pci_dev = pci_new(devfn, TYPE_PIIX4_PM);
-    dev = DEVICE(pci_dev);
-    qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
-    qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
-
-    s = PIIX4_PM(dev);
-
-    pci_realize_and_unref(pci_dev, bus, &error_fatal);
-
-    return s;
-}
-
 static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
 {
     PIIX4PMState *s = opaque;
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index d284475f07..976b4da582 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -14,10 +14,6 @@
 
 #include "hw/pci/pci.h"
 #include "qom/object.h"
-#include "hw/acpi/piix4.h"
-
-PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                              int smm_enabled);
 
 /* PIRQRC[A:D]: PIRQx Route Control Registers */
 #define PIIX_PIRQCA 0x60
-- 
2.20.1

Re: [PATCH 00/12] hw/acpi/piix4: remove legacy piix4_pm_init() function

[Philippe Mathieu-Daudé via]

On 28/5/22 11:19, Mark Cave-Ayland wrote:
> Whilst reviewing Bernhard's PIIX Southbridge QOMifcation patches at
> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04329.html, I
> noticed that we should first eliminate the legacy device init function
> piix4_pm_init().
> 
> This series moves the outstanding logic from piix4_pm_init() into the
> relevant instance init() and realize() functions, changes the IRQs to
> use qdev gpios, and then finally removes the now-unused piix4_pm_initfn()
> function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (12):
>    hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to
>      piix4_pm_realize()
>    hw/acpi/piix4: change smm_enabled from int to bool
>    hw/acpi/piix4: convert smm_enabled bool to qdev property
>    hw/acpi/piix4: move PIIX4PMState into separate piix4.h header
>    hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState
>    hw/acpi/piix4: rename piix4_pm_init() to piix4_pm_initfn()
>    hw/acpi/piix4: introduce piix4_pm_init() instance init function
>    hw/acpi/piix4: use qdev gpio to wire up sci_irq
>    hw/acpi/piix4: use qdev gpio to wire up smi_irq
>    hw/i386/pc_piix: create PIIX4_PM device directly instead of using
>      piix4_pm_initfn()
>    hw/isa/piix4.c: create PIIX4_PM device directly instead of using
>      piix4_pm_initfn()
>    hw/acpi/piix4: remove unused piix4_pm_initfn() function

Nitpicking, SCI could also be a named GPIO :)

Series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 05/12] hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState

[Bernhard Beschow]

Am 28. Mai 2022 09:19:27 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>This exposes the PIIX4_PM device to the caller to allow any qdev gpios to be
>mapped outside of piix4_pm_init().
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/acpi/piix4.c               | 11 ++++-------
> hw/i386/pc_piix.c             | 10 +++++-----
> hw/isa/piix4.c                |  8 +++++---
> include/hw/southbridge/piix.h |  7 ++++---
> 4 files changed, 18 insertions(+), 18 deletions(-)
>
>diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>index 7ee65b1bff..05c129b6af 100644
>--- a/hw/acpi/piix4.c
>+++ b/hw/acpi/piix4.c
>@@ -497,9 +497,9 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>     piix4_pm_add_properties(s);
> }
> 
>-I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>-                      qemu_irq sci_irq, qemu_irq smi_irq,
>-                      int smm_enabled, DeviceState **piix4_pm)
>+PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>+                            qemu_irq sci_irq, qemu_irq smi_irq,
>+                            int smm_enabled)
> {
>     PCIDevice *pci_dev;
>     DeviceState *dev;
>@@ -509,9 +509,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>     dev = DEVICE(pci_dev);
>     qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
>     qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
>-    if (piix4_pm) {
>-        *piix4_pm = dev;
>-    }
> 
>     s = PIIX4_PM(dev);
>     s->irq = sci_irq;
>@@ -519,7 +516,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> 
>     pci_realize_and_unref(pci_dev, bus, &error_fatal);
> 
>-    return s->smb.smbus;
>+    return s;
> }
> 
> static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index 578e537b35..0300be5ef4 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -281,14 +281,14 @@ static void pc_init1(MachineState *machine,
>     }
> 
>     if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
>-        DeviceState *piix4_pm;
>+        PIIX4PMState *piix4_pm;
> 
>         smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
>         /* TODO: Populate SPD eeprom data.  */

While at it: Perhaps move this comment...

>-        pcms->smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>-                                    x86ms->gsi[9], smi_irq,
>-                                    x86_machine_is_smm_enabled(x86ms),
>-                                    &piix4_pm);
>+        piix4_pm = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>+                                 x86ms->gsi[9], smi_irq,
>+                                 x86_machine_is_smm_enabled(x86ms));
>+        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));

... here? At least the Malta board seems to do this business here.

>         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
> 
>         object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
>diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>index 8607e0ac36..7d9bedd1bb 100644
>--- a/hw/isa/piix4.c
>+++ b/hw/isa/piix4.c
>@@ -293,6 +293,7 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
> {
>     PIIX4State *s;
>+    PIIX4PMState *pms;
>     PCIDevice *pci;
>     DeviceState *dev;
>     int devfn = PCI_DEVFN(10, 0);
>@@ -310,9 +311,10 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
> 
>     pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
>     if (smbus) {
>-        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>-                               qdev_get_gpio_in_named(dev, "isa", 9),
>-                               NULL, 0, NULL);
>+        pms = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>+                            qdev_get_gpio_in_named(dev, "isa", 9),
>+                            NULL, 0);
>+        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
>     }
> 
>     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>index c5b842b45d..108800ab06 100644
>--- a/include/hw/southbridge/piix.h
>+++ b/include/hw/southbridge/piix.h
>@@ -14,10 +14,11 @@
> 
> #include "hw/pci/pci.h"
> #include "qom/object.h"
>+#include "hw/acpi/piix4.h"
> 
>-I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>-                      qemu_irq sci_irq, qemu_irq smi_irq,
>-                      int smm_enabled, DeviceState **piix4_pm);
>+PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>+                            qemu_irq sci_irq, qemu_irq smi_irq,
>+                            int smm_enabled);
> 
> /* PIRQRC[A:D]: PIRQx Route Control Registers */
> #define PIIX_PIRQCA 0x60

Re: [PATCH 00/12] hw/acpi/piix4: remove legacy piix4_pm_init() function

[Bernhard Beschow]

Am 29. Mai 2022 18:05:41 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 28/5/22 11:19, Mark Cave-Ayland wrote:
>> Whilst reviewing Bernhard's PIIX Southbridge QOMifcation patches at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04329.html, I
>> noticed that we should first eliminate the legacy device init function
>> piix4_pm_init().
>> 
>> This series moves the outstanding logic from piix4_pm_init() into the
>> relevant instance init() and realize() functions, changes the IRQs to
>> use qdev gpios, and then finally removes the now-unused piix4_pm_initfn()
>> function.
>> 
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> 
>> 
>> Mark Cave-Ayland (12):
>>    hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to
>>      piix4_pm_realize()
>>    hw/acpi/piix4: change smm_enabled from int to bool
>>    hw/acpi/piix4: convert smm_enabled bool to qdev property
>>    hw/acpi/piix4: move PIIX4PMState into separate piix4.h header
>>    hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState
>>    hw/acpi/piix4: rename piix4_pm_init() to piix4_pm_initfn()
>>    hw/acpi/piix4: introduce piix4_pm_init() instance init function
>>    hw/acpi/piix4: use qdev gpio to wire up sci_irq
>>    hw/acpi/piix4: use qdev gpio to wire up smi_irq
>>    hw/i386/pc_piix: create PIIX4_PM device directly instead of using
>>      piix4_pm_initfn()
>>    hw/isa/piix4.c: create PIIX4_PM device directly instead of using
>>      piix4_pm_initfn()
>>    hw/acpi/piix4: remove unused piix4_pm_initfn() function
>
>Nitpicking, SCI could also be a named GPIO :)

Yes... I was just wondering the same!

Best regards,
Bernhard

>
>Series:
>Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 04/12] hw/acpi/piix4: move PIIX4PMState into separate piix4.h header

[Bernhard Beschow]

Am 28. Mai 2022 09:19:26 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>This allows the QOM types in hw/acpi/piix4.c to be used elsewhere by simply including
>hw/acpi/piix4.h.
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/acpi/piix4.c               | 43 +-------------------
> hw/i386/acpi-build.c          |  1 +
> include/hw/acpi/piix4.h       | 75 +++++++++++++++++++++++++++++++++++
> include/hw/southbridge/piix.h |  2 -
> 4 files changed, 78 insertions(+), 43 deletions(-)
> create mode 100644 include/hw/acpi/piix4.h
>
>diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>index 2735ff375e..7ee65b1bff 100644
>--- a/hw/acpi/piix4.c
>+++ b/hw/acpi/piix4.c
>@@ -28,6 +28,8 @@
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> #include "hw/acpi/acpi.h"
>+#include "hw/acpi/pcihp.h"

No need to be included twice.

>+#include "hw/acpi/piix4.h"
> #include "sysemu/runstate.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/xen.h"
>@@ -56,47 +58,6 @@ struct pci_status {
>     uint32_t down;
> };
> 
>-struct PIIX4PMState {
>-    /*< private >*/
>-    PCIDevice parent_obj;
>-    /*< public >*/
>-
>-    MemoryRegion io;
>-    uint32_t io_base;
>-
>-    MemoryRegion io_gpe;
>-    ACPIREGS ar;
>-
>-    APMState apm;
>-
>-    PMSMBus smb;
>-    uint32_t smb_io_base;
>-
>-    qemu_irq irq;
>-    qemu_irq smi_irq;
>-    bool smm_enabled;
>-    bool smm_compat;
>-    Notifier machine_ready;
>-    Notifier powerdown_notifier;
>-
>-    AcpiPciHpState acpi_pci_hotplug;
>-    bool use_acpi_hotplug_bridge;
>-    bool use_acpi_root_pci_hotplug;
>-    bool not_migrate_acpi_index;
>-
>-    uint8_t disable_s3;
>-    uint8_t disable_s4;
>-    uint8_t s4_val;
>-
>-    bool cpu_hotplug_legacy;
>-    AcpiCpuHotplug gpe_cpu;
>-    CPUHotplugState cpuhp_state;
>-
>-    MemHotplugState acpi_memory_hotplug;
>-};
>-
>-OBJECT_DECLARE_SIMPLE_TYPE(PIIX4PMState, PIIX4_PM)
>-
> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                                            PCIBus *bus, PIIX4PMState *s);
> 
>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>index c125939ed6..89ac326d7f 100644
>--- a/hw/i386/acpi-build.c
>+++ b/hw/i386/acpi-build.c
>@@ -46,6 +46,7 @@
> #include "hw/acpi/tpm.h"
> #include "hw/acpi/vmgenid.h"
> #include "hw/acpi/erst.h"
>+#include "hw/acpi/piix4.h"
> #include "sysemu/tpm_backend.h"
> #include "hw/rtc/mc146818rtc_regs.h"
> #include "migration/vmstate.h"
>diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
>new file mode 100644
>index 0000000000..32686a75c5
>--- /dev/null
>+++ b/include/hw/acpi/piix4.h
>@@ -0,0 +1,75 @@
>+/*
>+ * ACPI implementation
>+ *
>+ * Copyright (c) 2006 Fabrice Bellard
>+ *
>+ * This library is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Lesser General Public
>+ * License version 2.1 as published by the Free Software Foundation.
>+ *
>+ * This library is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Lesser General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Lesser General Public
>+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
>+ *
>+ * Contributions after 2012-01-13 are licensed under the terms of the
>+ * GNU GPL, version 2 or (at your option) any later version.
>+ */
>+
>+#ifndef HW_ACPI_PIIX4_H
>+#define HW_ACPI_PIIX4_H
>+
>+#include "hw/pci/pci.h"
>+#include "hw/acpi/acpi.h"
>+#include "hw/acpi/cpu_hotplug.h"
>+#include "hw/acpi/memory_hotplug.h"
>+#include "hw/acpi/pcihp.h"
>+#include "hw/i2c/pm_smbus.h"
>+#include "hw/isa/apm.h"

These headers don't need to be included in the .c file any more.

>+
>+#define TYPE_PIIX4_PM "PIIX4_PM"
>+OBJECT_DECLARE_SIMPLE_TYPE(PIIX4PMState, PIIX4_PM)
>+
>+struct PIIX4PMState {
>+    /*< private >*/
>+    PCIDevice parent_obj;
>+    /*< public >*/
>+
>+    MemoryRegion io;
>+    uint32_t io_base;
>+
>+    MemoryRegion io_gpe;
>+    ACPIREGS ar;
>+
>+    APMState apm;
>+
>+    PMSMBus smb;
>+    uint32_t smb_io_base;
>+
>+    qemu_irq irq;
>+    qemu_irq smi_irq;
>+    bool smm_enabled;
>+    bool smm_compat;
>+    Notifier machine_ready;
>+    Notifier powerdown_notifier;
>+
>+    AcpiPciHpState acpi_pci_hotplug;
>+    bool use_acpi_hotplug_bridge;
>+    bool use_acpi_root_pci_hotplug;
>+    bool not_migrate_acpi_index;
>+
>+    uint8_t disable_s3;
>+    uint8_t disable_s4;
>+    uint8_t s4_val;
>+
>+    bool cpu_hotplug_legacy;
>+    AcpiCpuHotplug gpe_cpu;
>+    CPUHotplugState cpuhp_state;
>+
>+    MemHotplugState acpi_memory_hotplug;
>+};
>+
>+#endif
>diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>index f63f83e5c6..c5b842b45d 100644
>--- a/include/hw/southbridge/piix.h
>+++ b/include/hw/southbridge/piix.h
>@@ -15,8 +15,6 @@
> #include "hw/pci/pci.h"
> #include "qom/object.h"
> 
>-#define TYPE_PIIX4_PM "PIIX4_PM"
>-
> I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                       qemu_irq sci_irq, qemu_irq smi_irq,
>                       int smm_enabled, DeviceState **piix4_pm);

Re: [PATCH 07/12] hw/acpi/piix4: introduce piix4_pm_init() instance init function

[Bernhard Beschow]

Am 28. Mai 2022 09:19:29 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>Use the new piix4_pm_init() instance init function to initialise 2 separate qdev
>gpios for the SCI and SMI IRQs.
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/acpi/piix4.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>index d897d2dee6..454fa34df1 100644
>--- a/hw/acpi/piix4.c
>+++ b/hw/acpi/piix4.c
>@@ -497,6 +497,14 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>     piix4_pm_add_properties(s);
> }
> 
>+static void piix4_pm_init(Object *obj)
>+{
>+    PIIX4PMState *s = PIIX4_PM(obj);
>+
>+    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>+    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
>+}

The two IRQs still get connected internally. Doesn't this create the risk of double connections until patches 8 and 9 are applied?

>+
> PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                               qemu_irq sci_irq, qemu_irq smi_irq,
>                               int smm_enabled)
>@@ -663,6 +671,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> static const TypeInfo piix4_pm_info = {
>     .name          = TYPE_PIIX4_PM,
>     .parent        = TYPE_PCI_DEVICE,
>+    .instance_init  = piix4_pm_init,
>     .instance_size = sizeof(PIIX4PMState),
>     .class_init    = piix4_pm_class_init,
>     .interfaces = (InterfaceInfo[]) {

Re: [PATCH 01/12] hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to piix4_pm_realize()

[Ani Sinha]

On Sat, May 28, 2022 at 2:49 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This logic can be included as part of piix4_pm_realize() and does not need to
> be handled externally.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/piix4.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index fe5625d07a..bf20fa139b 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -525,6 +525,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>      s->machine_ready.notify = piix4_pm_machine_ready;
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
>
> +    if (xen_enabled()) {
> +        s->use_acpi_hotplug_bridge = false;
> +    }
> +
>      piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
>                                     pci_get_bus(dev), s);
>      qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
> @@ -551,9 +555,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>      s->irq = sci_irq;
>      s->smi_irq = smi_irq;
>      s->smm_enabled = smm_enabled;
> -    if (xen_enabled()) {
> -        s->use_acpi_hotplug_bridge = false;
> -    }
>
>      pci_realize_and_unref(pci_dev, bus, &error_fatal);
>
> --
> 2.20.1
>

Re: [PATCH 02/12] hw/acpi/piix4: change smm_enabled from int to bool

[Ani Sinha]

On Sat, May 28, 2022 at 2:49 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This is in preparation for conversion to a qdev property.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

other than the comment below,
Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/piix4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index bf20fa139b..fcfaafc175 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -74,7 +74,7 @@ struct PIIX4PMState {
>
>      qemu_irq irq;
>      qemu_irq smi_irq;
> -    int smm_enabled;
> +    bool smm_enabled;

For the sake of consistency, I would also change the signature of
piix4_pm_init(), that is, change simm_enabled from int to bool.
We are good in pc_init1 since x86_machine_is_smm_enabled() returns
bool. In piix4_create() in isa, we pass integer 0 which we might want
to make boolean.

>      bool smm_compat;
>      Notifier machine_ready;
>      Notifier powerdown_notifier;
> --
> 2.20.1
>

Re: [PATCH 03/12] hw/acpi/piix4: convert smm_enabled bool to qdev property

[Ani Sinha]

On Sat, May 28, 2022 at 2:49 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This allows the smm_enabled value to be set using a standard qdev property instead
> of being referenced directly in piix4_pm_init().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/piix4.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index fcfaafc175..2735ff375e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -547,6 +547,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>      pci_dev = pci_new(devfn, TYPE_PIIX4_PM);
>      dev = DEVICE(pci_dev);
>      qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
> +    qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
>      if (piix4_pm) {
>          *piix4_pm = dev;
>      }
> @@ -554,7 +555,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>      s = PIIX4_PM(dev);
>      s->irq = sci_irq;
>      s->smi_irq = smi_irq;
> -    s->smm_enabled = smm_enabled;
>
>      pci_realize_and_unref(pci_dev, bus, &error_fatal);
>
> @@ -664,6 +664,7 @@ static Property piix4_pm_properties[] = {
>      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>                       acpi_memory_hotplug.is_enabled, true),
>      DEFINE_PROP_BOOL("smm-compat", PIIX4PMState, smm_compat, false),
> +    DEFINE_PROP_BOOL("smm-enabled", PIIX4PMState, smm_enabled, false),
>      DEFINE_PROP_BOOL("x-not-migrate-acpi-index", PIIX4PMState,
>                        not_migrate_acpi_index, false),
>      DEFINE_PROP_END_OF_LIST(),
> --
> 2.20.1
>

Re: [PATCH 07/12] hw/acpi/piix4: introduce piix4_pm_init() instance init function

[Philippe Mathieu-Daudé via]

On 29/5/22 21:06, Bernhard Beschow wrote:
> Am 28. Mai 2022 09:19:29 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> Use the new piix4_pm_init() instance init function to initialise 2 separate qdev
>> gpios for the SCI and SMI IRQs.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/acpi/piix4.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index d897d2dee6..454fa34df1 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -497,6 +497,14 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>      piix4_pm_add_properties(s);
>> }
>>
>> +static void piix4_pm_init(Object *obj)
>> +{
>> +    PIIX4PMState *s = PIIX4_PM(obj);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>> +    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
>> +}
> 
> The two IRQs still get connected internally. Doesn't this create the risk of double connections until patches 8 and 9 are applied?

The same field is initialized twice indeed, but we shouldn't have 
"double connections". It could be cleaner to split this patch
and squash in the following 2 patches. Can do.

Re: [PATCH 02/12] hw/acpi/piix4: change smm_enabled from int to bool

[Philippe Mathieu-Daudé via]

On 30/5/22 06:56, Ani Sinha wrote:
> On Sat, May 28, 2022 at 2:49 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This is in preparation for conversion to a qdev property.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> other than the comment below,
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> 
>> ---
>>   hw/acpi/piix4.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index bf20fa139b..fcfaafc175 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -74,7 +74,7 @@ struct PIIX4PMState {
>>
>>       qemu_irq irq;
>>       qemu_irq smi_irq;
>> -    int smm_enabled;
>> +    bool smm_enabled;
> 
> For the sake of consistency, I would also change the signature of
> piix4_pm_init(), that is, change simm_enabled from int to bool.
> We are good in pc_init1 since x86_machine_is_smm_enabled() returns
> bool. In piix4_create() in isa, we pass integer 0 which we might want
> to make boolean.

This function ends up removed... But OK.

Re: [PATCH 00/12] hw/acpi/piix4: remove legacy piix4_pm_init() function

[Mark Cave-Ayland]

On 29/05/2022 19:05, Philippe Mathieu-Daudé via wrote:

> On 28/5/22 11:19, Mark Cave-Ayland wrote:
>> Whilst reviewing Bernhard's PIIX Southbridge QOMifcation patches at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04329.html, I
>> noticed that we should first eliminate the legacy device init function
>> piix4_pm_init().
>>
>> This series moves the outstanding logic from piix4_pm_init() into the
>> relevant instance init() and realize() functions, changes the IRQs to
>> use qdev gpios, and then finally removes the now-unused piix4_pm_initfn()
>> function.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (12):
>>    hw/acpi/piix4: move xen_enabled() logic from piix4_pm_init() to
>>      piix4_pm_realize()
>>    hw/acpi/piix4: change smm_enabled from int to bool
>>    hw/acpi/piix4: convert smm_enabled bool to qdev property
>>    hw/acpi/piix4: move PIIX4PMState into separate piix4.h header
>>    hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState
>>    hw/acpi/piix4: rename piix4_pm_init() to piix4_pm_initfn()
>>    hw/acpi/piix4: introduce piix4_pm_init() instance init function
>>    hw/acpi/piix4: use qdev gpio to wire up sci_irq
>>    hw/acpi/piix4: use qdev gpio to wire up smi_irq
>>    hw/i386/pc_piix: create PIIX4_PM device directly instead of using
>>      piix4_pm_initfn()
>>    hw/isa/piix4.c: create PIIX4_PM device directly instead of using
>>      piix4_pm_initfn()
>>    hw/acpi/piix4: remove unused piix4_pm_initfn() function
> 
> Nitpicking, SCI could also be a named GPIO :)

FWIW I used the SCI irq as the "primary" GPIO since it was referenced as just "irq" 
within PIIX4PMState which is typical for a single output IRQ (my guess would be that 
"smi_irq" came along later).

> Series:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!


ATB,

Mark.

Re: [PATCH 05/12] hw/acpi/piix4: alter piix4_pm_init() to return PIIX4PMState

[Mark Cave-Ayland]

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

> Am 28. Mai 2022 09:19:27 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> This exposes the PIIX4_PM device to the caller to allow any qdev gpios to be
>> mapped outside of piix4_pm_init().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/acpi/piix4.c               | 11 ++++-------
>> hw/i386/pc_piix.c             | 10 +++++-----
>> hw/isa/piix4.c                |  8 +++++---
>> include/hw/southbridge/piix.h |  7 ++++---
>> 4 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 7ee65b1bff..05c129b6af 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -497,9 +497,9 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>      piix4_pm_add_properties(s);
>> }
>>
>> -I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>> -                      qemu_irq sci_irq, qemu_irq smi_irq,
>> -                      int smm_enabled, DeviceState **piix4_pm)
>> +PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>> +                            qemu_irq sci_irq, qemu_irq smi_irq,
>> +                            int smm_enabled)
>> {
>>      PCIDevice *pci_dev;
>>      DeviceState *dev;
>> @@ -509,9 +509,6 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>>      dev = DEVICE(pci_dev);
>>      qdev_prop_set_uint32(dev, "smb_io_base", smb_io_base);
>>      qdev_prop_set_bit(dev, "smm-enabled", smm_enabled);
>> -    if (piix4_pm) {
>> -        *piix4_pm = dev;
>> -    }
>>
>>      s = PIIX4_PM(dev);
>>      s->irq = sci_irq;
>> @@ -519,7 +516,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>>
>>      pci_realize_and_unref(pci_dev, bus, &error_fatal);
>>
>> -    return s->smb.smbus;
>> +    return s;
>> }
>>
>> static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 578e537b35..0300be5ef4 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -281,14 +281,14 @@ static void pc_init1(MachineState *machine,
>>      }
>>
>>      if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
>> -        DeviceState *piix4_pm;
>> +        PIIX4PMState *piix4_pm;
>>
>>          smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
>>          /* TODO: Populate SPD eeprom data.  */
> 
> While at it: Perhaps move this comment...
> 
>> -        pcms->smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>> -                                    x86ms->gsi[9], smi_irq,
>> -                                    x86_machine_is_smm_enabled(x86ms),
>> -                                    &piix4_pm);
>> +        piix4_pm = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>> +                                 x86ms->gsi[9], smi_irq,
>> +                                 x86_machine_is_smm_enabled(x86ms));
>> +        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(piix4_pm), "i2c"));
> 
> ... here? At least the Malta board seems to do this business here.

Possibly, although it was still close enough to smbus_eeprom_init() to be useful when 
working on the series. I could do this if there were enough comments to spin out a v2 
though.

>>          smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
>>
>>          object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 8607e0ac36..7d9bedd1bb 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -293,6 +293,7 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>> {
>>      PIIX4State *s;
>> +    PIIX4PMState *pms;
>>      PCIDevice *pci;
>>      DeviceState *dev;
>>      int devfn = PCI_DEVFN(10, 0);
>> @@ -310,9 +311,10 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>
>>      pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
>>      if (smbus) {
>> -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>> -                               qdev_get_gpio_in_named(dev, "isa", 9),
>> -                               NULL, 0, NULL);
>> +        pms = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>> +                            qdev_get_gpio_in_named(dev, "isa", 9),
>> +                            NULL, 0);
>> +        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pms), "i2c"));
>>      }
>>
>>      pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index c5b842b45d..108800ab06 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -14,10 +14,11 @@
>>
>> #include "hw/pci/pci.h"
>> #include "qom/object.h"
>> +#include "hw/acpi/piix4.h"
>>
>> -I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>> -                      qemu_irq sci_irq, qemu_irq smi_irq,
>> -                      int smm_enabled, DeviceState **piix4_pm);
>> +PIIX4PMState *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>> +                            qemu_irq sci_irq, qemu_irq smi_irq,
>> +                            int smm_enabled);
>>
>> /* PIRQRC[A:D]: PIRQx Route Control Registers */
>> #define PIIX_PIRQCA 0x60


ATB,

Mark.

Re: [PATCH 07/12] hw/acpi/piix4: introduce piix4_pm_init() instance init function

[Mark Cave-Ayland]

On 29/05/2022 20:06, Bernhard Beschow wrote:

> Am 28. Mai 2022 09:19:29 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> Use the new piix4_pm_init() instance init function to initialise 2 separate qdev
>> gpios for the SCI and SMI IRQs.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/acpi/piix4.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index d897d2dee6..454fa34df1 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -497,6 +497,14 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>      piix4_pm_add_properties(s);
>> }
>>
>> +static void piix4_pm_init(Object *obj)
>> +{
>> +    PIIX4PMState *s = PIIX4_PM(obj);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>> +    qdev_init_gpio_out_named(DEVICE(obj), &s->smi_irq, "smi-irq", 1);
>> +}
> 
> The two IRQs still get connected internally. Doesn't this create the risk of double connections until patches 8 and 9 are applied?

No, that should be fine. Here the address of the IRQ is being made available as a 
qdev gpio for use by qdev_connect_gpio_out(). Since that isn't being used yet, and 
the 2 IRQs are still being set afterwards in piix4_pm_initfn(), everything should 
still work just as before.

>> +
>> PIIX4PMState *piix4_pm_initfn(PCIBus *bus, int devfn, uint32_t smb_io_base,
>>                                qemu_irq sci_irq, qemu_irq smi_irq,
>>                                int smm_enabled)
>> @@ -663,6 +671,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>> static const TypeInfo piix4_pm_info = {
>>      .name          = TYPE_PIIX4_PM,
>>      .parent        = TYPE_PCI_DEVICE,
>> +    .instance_init  = piix4_pm_init,
>>      .instance_size = sizeof(PIIX4PMState),
>>      .class_init    = piix4_pm_class_init,
>>      .interfaces = (InterfaceInfo[]) {

ATB,

Mark.