[PATCH v3 0/7] QOM'ify PIIX southbridge creation

[PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Bernhard Beschow]

v3:
* Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1]
* Use embedded structs for touched PCI devices (Mark)
* Fix piix4's rtc embedded struct to be initialized by
  object_initialize_child() (Peter) [2]

Testing done:

1)
`make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
Result: All pass.

2)
* `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
* `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
  debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`

In both cases the system booted successfully and it was possible to shut down
the system using the `poweroff` command.


v2:
* Preserve `DeviceState *` as return value of piix4_create() (Mark)
* Aggregate all type name movements into first commit (Mark)
* Have piix4 southbridge rather than malta board instantiate piix4 pm (me)

Testing done:

1)
`make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
Result: All pass.

2)
Modify pci_piix3_realize() to start with
    error_setg(errp, "This is a test");
Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom 
archlinux-2022.05.01-x86_64.iso`.
Result: qemu-system-x86_64 aborts with: "This is a test"


v1:
The piix3 and piix4 southbridge devices still rely on create() functions which
are deprecated. This series resolves these functions piece by piece to
modernize the code.

Both devices are modified in lockstep where possible to provide more context.

Testing done:
* `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
* `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
  debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`

In both cases the system booted successfully and it was possible to shut down
the system using the `poweroff` command.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html

Bernhard Beschow (7):
  include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
  hw/isa/piix4: Use object_initialize_child() for embedded struct
  hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
  hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
  hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
  hw/isa/piix4: QOM'ify PIIX4 PM creation
  hw/isa/piix{3,4}: Inline and remove create() functions

 hw/i386/pc_piix.c             |   7 +-
 hw/isa/piix3.c                |  98 ++++++++++++++-------------
 hw/isa/piix4.c                | 120 +++++++++++++++++-----------------
 hw/mips/malta.c               |   7 +-
 include/hw/isa/isa.h          |   2 -
 include/hw/southbridge/piix.h |   6 +-
 6 files changed, 127 insertions(+), 113 deletions(-)

-- 
2.36.1

[PATCH v3 1/7] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Bernhard Beschow]

TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
ones, too.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/piix3.c                | 3 ---
 include/hw/isa/isa.h          | 2 --
 include/hw/southbridge/piix.h | 4 ++++
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index dab901c9ad..d96ce2b788 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -35,9 +35,6 @@
 
 #define XEN_PIIX_NUM_PIRQS      128ULL
 
-#define TYPE_PIIX3_DEVICE "PIIX3"
-#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
-
 static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
     qemu_set_irq(piix3->pic[pic_irq],
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 034d706ba1..e9fa2f5cea 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -144,6 +144,4 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
     return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
 }
 
-#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
-
 #endif
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 976b4da582..3b97186f75 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -64,6 +64,10 @@ typedef struct PIIXState PIIX3State;
 DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
                          TYPE_PIIX3_PCI_DEVICE)
 
+#define TYPE_PIIX3_DEVICE "PIIX3"
+#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
+#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
+
 PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
 
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
-- 
2.36.1

[PATCH v3 2/7] hw/isa/piix4: Use object_initialize_child() for embedded struct

[Bernhard Beschow]

Found-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 9a6d981037..1d04fb6a55 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -224,7 +224,7 @@ static void piix4_init(Object *obj)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(obj);
 
-    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
 }
 
 static void piix4_class_init(ObjectClass *klass, void *data)
-- 
2.36.1

[PATCH v3 3/7] hw/isa/piix{3, 4}: Move pci_map_irq_fn's near pci_set_irq_fn's

[Bernhard Beschow]

The pci_map_irq_fn's were implemented below type_init() which made them
inaccessible to QOM functions. So move them up.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/piix3.c | 22 +++++++++++-----------
 hw/isa/piix4.c | 50 +++++++++++++++++++++++++-------------------------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index d96ce2b788..c7a9014c3f 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -78,6 +78,17 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
     piix3_set_irq_level(piix3, pirq, level);
 }
 
+/*
+ * Return the global irq number corresponding to a given device irq
+ * pin. We could also use the bus number to have a more precise mapping.
+ */
+static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
+{
+    int slot_addend;
+    slot_addend = PCI_SLOT(pci_dev->devfn) - 1;
+    return (pci_intx + slot_addend) & 3;
+}
+
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pin)
 {
     PIIX3State *piix3 = opaque;
@@ -350,17 +361,6 @@ static void piix3_register_types(void)
 
 type_init(piix3_register_types)
 
-/*
- * Return the global irq number corresponding to a given device irq
- * pin. We could also use the bus number to have a more precise mapping.
- */
-static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
-{
-    int slot_addend;
-    slot_addend = PCI_SLOT(pci_dev->devfn) - 1;
-    return (pci_intx + slot_addend) & 3;
-}
-
 PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus)
 {
     PIIX3State *piix3;
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 1d04fb6a55..18aa24424f 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -74,6 +74,31 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
     }
 }
 
+static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
+{
+    int slot;
+
+    slot = PCI_SLOT(pci_dev->devfn);
+
+    switch (slot) {
+    /* PIIX4 USB */
+    case 10:
+        return 3;
+    /* AMD 79C973 Ethernet */
+    case 11:
+        return 1;
+    /* Crystal 4281 Sound */
+    case 12:
+        return 2;
+    /* PCI slot 1 to 4 */
+    case 18 ... 21:
+        return ((slot - 18) + irq_num) & 0x03;
+    /* Unknown device, don't do any translation */
+    default:
+        return irq_num;
+    }
+}
+
 static void piix4_isa_reset(DeviceState *dev)
 {
     PIIX4State *d = PIIX4_PCI_DEVICE(dev);
@@ -266,31 +291,6 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
-{
-    int slot;
-
-    slot = PCI_SLOT(pci_dev->devfn);
-
-    switch (slot) {
-    /* PIIX4 USB */
-    case 10:
-        return 3;
-    /* AMD 79C973 Ethernet */
-    case 11:
-        return 1;
-    /* Crystal 4281 Sound */
-    case 12:
-        return 2;
-    /* PCI slot 1 to 4 */
-    case 18 ... 21:
-        return ((slot - 18) + irq_num) & 0x03;
-    /* Unknown device, don't do any translation */
-    default:
-        return irq_num;
-    }
-}
-
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
     PIIX4State *s;
-- 
2.36.1

[PATCH v3 4/7] hw/isa/piix{3, 4}: QOM'ify PCI device creation and wiring

[Bernhard Beschow]

PCI interrupt wiring and device creation (piix4 only) were performed
in create() functions which are obsolete. Move these tasks into QOM
functions to modernize the code.

In order to avoid duplicate checking for xen_enabled() the piix3 realize
methods are now split.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix3.c | 67 +++++++++++++++++++++++++++++++++-----------------
 hw/isa/piix4.c | 30 ++++++++++++++++------
 2 files changed, 67 insertions(+), 30 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index c7a9014c3f..de532cc692 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/range.h"
+#include "qapi/error.h"
 #include "hw/southbridge/piix.h"
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
@@ -277,7 +278,7 @@ static const MemoryRegionOps rcr_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN
 };
 
-static void piix3_realize(PCIDevice *dev, Error **errp)
+static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
 
@@ -302,7 +303,6 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
     dc->hotpluggable   = false;
-    k->realize      = piix3_realize;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
@@ -326,11 +326,28 @@ static const TypeInfo piix3_pci_type_info = {
     },
 };
 
+static void piix3_realize(PCIDevice *dev, Error **errp)
+{
+    ERRP_GUARD();
+    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+
+    pci_piix3_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
+
+    pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
+                 piix3, PIIX_NUM_PIRQS);
+    pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
+};
+
 static void piix3_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix3_write_config;
+    k->realize = piix3_realize;
 }
 
 static const TypeInfo piix3_info = {
@@ -339,11 +356,33 @@ static const TypeInfo piix3_info = {
     .class_init    = piix3_class_init,
 };
 
+static void piix3_xen_realize(PCIDevice *dev, Error **errp)
+{
+    ERRP_GUARD();
+    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+
+    pci_piix3_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
+
+    /*
+     * Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI.
+     */
+    pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+                 piix3, XEN_PIIX_NUM_PIRQS);
+};
+
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix3_write_config_xen;
+    k->realize = piix3_xen_realize;
 };
 
 static const TypeInfo piix3_xen_info = {
@@ -365,27 +404,11 @@ PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus)
 {
     PIIX3State *piix3;
     PCIDevice *pci_dev;
+    const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
+                                     : TYPE_PIIX3_DEVICE;
 
-    /*
-     * Xen supports additional interrupt routes from the PCI devices to
-     * the IOAPIC: the four pins of each PCI device on the bus are also
-     * connected to the IOAPIC directly.
-     * These additional routes can be discovered through ACPI.
-     */
-    if (xen_enabled()) {
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
-                                                  TYPE_PIIX3_XEN_DEVICE);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
-        pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
-                     piix3, XEN_PIIX_NUM_PIRQS);
-    } else {
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
-                                                  TYPE_PIIX3_DEVICE);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
-        pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
-                     piix3, PIIX_NUM_PIRQS);
-        pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
-    }
+    pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+    piix3 = PIIX3_PCI_DEVICE(pci_dev);
     *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
 
     return piix3;
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 18aa24424f..058bebb5e2 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -35,6 +35,7 @@
 #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"
@@ -46,6 +47,8 @@ struct PIIX4State {
     qemu_irq *isa;
 
     RTCState rtc;
+    PCIIDEState ide;
+    UHCIState uhci;
     /* Reset Control Register */
     MemoryRegion rcr_mem;
     uint8_t rcr;
@@ -205,6 +208,7 @@ static const MemoryRegionOps piix4_rcr_ops = {
 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;
 
@@ -243,6 +247,21 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
     s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), 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;
+    }
+    pci_ide_create_devs(PCI_DEVICE(&s->ide));
+
+    /* USB */
+    qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
+    if (!qdev_realize(DEVICE(&s->uhci), BUS(pci_bus), errp)) {
+        return;
+    }
+
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 }
 
 static void piix4_init(Object *obj)
@@ -250,6 +269,8 @@ static void piix4_init(Object *obj)
     PIIX4State *s = PIIX4_PCI_DEVICE(obj);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
+    object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
 }
 
 static void piix4_class_init(ObjectClass *klass, void *data)
@@ -293,7 +314,6 @@ type_init(piix4_register_types)
 
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
-    PIIX4State *s;
     PCIDevice *pci;
     DeviceState *dev;
     int devfn = PCI_DEVFN(10, 0);
@@ -301,15 +321,11 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
     pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
                                           TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
-    s = PIIX4_PCI_DEVICE(pci);
+
     if (isa_bus) {
         *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     }
 
-    pci = pci_create_simple(pci_bus, devfn + 1, "piix4-ide");
-    pci_ide_create_devs(pci);
-
-    pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
     if (smbus) {
         pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
         qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
@@ -320,7 +336,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
         *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);
-
     return dev;
 }
-- 
2.36.1

[PATCH v3 5/7] hw/isa/piix{3, 4}: Factor out ISABus retrieval from create() functions

[Bernhard Beschow]

Modernizes the code and even saves a few lines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/pc_piix.c             | 3 ++-
 hw/isa/piix3.c                | 3 +--
 hw/isa/piix4.c                | 6 +-----
 hw/mips/malta.c               | 3 ++-
 include/hw/southbridge/piix.h | 4 ++--
 5 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4120fd52e5..9d2076a7ca 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -207,9 +207,10 @@ static void pc_init1(MachineState *machine,
                               pci_memory, ram_memory);
         pcms->bus = pci_bus;
 
-        piix3 = piix3_create(pci_bus, &isa_bus);
+        piix3 = piix3_create(pci_bus);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
+        isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
     } else {
         pci_bus = NULL;
         i440fx_state = NULL;
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index de532cc692..c6ff7795f4 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -400,7 +400,7 @@ static void piix3_register_types(void)
 
 type_init(piix3_register_types)
 
-PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus)
+PIIX3State *piix3_create(PCIBus *pci_bus)
 {
     PIIX3State *piix3;
     PCIDevice *pci_dev;
@@ -409,7 +409,6 @@ PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus)
 
     pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
     piix3 = PIIX3_PCI_DEVICE(pci_dev);
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
 
     return piix3;
 }
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 058bebb5e2..96df21a610 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -312,7 +312,7 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
+DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
 {
     PCIDevice *pci;
     DeviceState *dev;
@@ -322,10 +322,6 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
                                           TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
 
-    if (isa_bus) {
-        *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
-    }
-
     if (smbus) {
         pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
         qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9ffdc5b8f1..e446b25ad0 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1399,7 +1399,8 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &isa_bus, &smbus);
+    dev = piix4_create(pci_bus, &smbus);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 3b97186f75..0a2ef0c7b6 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -68,8 +68,8 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
 #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
 #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
 
-PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
+PIIX3State *piix3_create(PCIBus *pci_bus);
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
+DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
 
 #endif
-- 
2.36.1

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

[Bernhard Beschow]

Just like the real hardware, create the PIIX4 ACPI controller as part of
the PIIX4 southbridge. This also mirrors how the IDE and USB functions
are already created.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c                | 25 ++++++++++++++-----------
 hw/mips/malta.c               |  3 ++-
 include/hw/southbridge/piix.h |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 96df21a610..217989227d 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -49,6 +49,7 @@ struct PIIX4State {
     RTCState rtc;
     PCIIDEState ide;
     UHCIState uhci;
+    PIIX4PMState pm;
     /* Reset Control Register */
     MemoryRegion rcr_mem;
     uint8_t rcr;
@@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
+    /* ACPI controller */
+    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
+    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
+        return;
+    }
+    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
+    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm), "i2c");
+
     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 }
 
@@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
     object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
+
+    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
+    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
+    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
 }
 
 static void piix4_class_init(ObjectClass *klass, void *data)
@@ -312,7 +325,7 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
+DeviceState *piix4_create(PCIBus *pci_bus)
 {
     PCIDevice *pci;
     DeviceState *dev;
@@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
                                           TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
 
-    if (smbus) {
-        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
-        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
-        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
-        pci_realize_and_unref(pci, pci_bus, &error_fatal);
-        qdev_connect_gpio_out(DEVICE(pci), 0,
-                              qdev_get_gpio_in_named(dev, "isa", 9));
-        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
-    }
-
     return dev;
 }
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index e446b25ad0..b0fc84ccbb 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &smbus);
+    dev = piix4_create(pci_bus);
     isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 0a2ef0c7b6..e1f5d6d5c8 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
 
 PIIX3State *piix3_create(PCIBus *pci_bus);
 
-DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
+DeviceState *piix4_create(PCIBus *pci_bus);
 
 #endif
-- 
2.36.1

[PATCH v3 7/7] hw/isa/piix{3, 4}: Inline and remove create() functions

[Bernhard Beschow]

During the previous changesets the create() functions became trivial
wrappers around more generic functions. Modernize the code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/pc_piix.c             |  6 +++++-
 hw/isa/piix3.c                | 13 -------------
 hw/isa/piix4.c                | 13 -------------
 hw/mips/malta.c               |  5 ++++-
 include/hw/southbridge/piix.h |  4 ----
 5 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9d2076a7ca..107efa1ca6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -197,6 +197,9 @@ static void pc_init1(MachineState *machine,
 
     if (pcmc->pci_enabled) {
         PIIX3State *piix3;
+        PCIDevice *pci_dev;
+        const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
+                                         : TYPE_PIIX3_DEVICE;
 
         pci_bus = i440fx_init(host_type,
                               pci_type,
@@ -207,7 +210,8 @@ static void pc_init1(MachineState *machine,
                               pci_memory, ram_memory);
         pcms->bus = pci_bus;
 
-        piix3 = piix3_create(pci_bus);
+        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+        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"));
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index c6ff7795f4..01c376b39a 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -399,16 +399,3 @@ static void piix3_register_types(void)
 }
 
 type_init(piix3_register_types)
-
-PIIX3State *piix3_create(PCIBus *pci_bus)
-{
-    PIIX3State *piix3;
-    PCIDevice *pci_dev;
-    const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
-                                     : TYPE_PIIX3_DEVICE;
-
-    pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
-    piix3 = PIIX3_PCI_DEVICE(pci_dev);
-
-    return piix3;
-}
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 217989227d..677bc2f658 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -324,16 +324,3 @@ static void piix4_register_types(void)
 }
 
 type_init(piix4_register_types)
-
-DeviceState *piix4_create(PCIBus *pci_bus)
-{
-    PCIDevice *pci;
-    DeviceState *dev;
-    int devfn = PCI_DEVFN(10, 0);
-
-    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
-                                          TYPE_PIIX4_PCI_DEVICE);
-    dev = DEVICE(pci);
-
-    return dev;
-}
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index b0fc84ccbb..7afc4bac9a 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1238,6 +1238,7 @@ void mips_malta_init(MachineState *machine)
     int be;
     MaltaState *s;
     DeviceState *dev;
+    PCIDevice *piix4;
 
     s = MIPS_MALTA(qdev_new(TYPE_MIPS_MALTA));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
@@ -1399,7 +1400,9 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus);
+    piix4 = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
+                                            TYPE_PIIX4_PCI_DEVICE);
+    dev = DEVICE(piix4);
     isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
 
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index e1f5d6d5c8..2693778b23 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -68,8 +68,4 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
 #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
 #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
 
-PIIX3State *piix3_create(PCIBus *pci_bus);
-
-DeviceState *piix4_create(PCIBus *pci_bus);
-
 #endif
-- 
2.36.1

Re: [PATCH v3 1/7] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Mark Cave-Ayland]

On 28/05/2022 20:20, Bernhard Beschow wrote:

> TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
> ones, too.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/isa/piix3.c                | 3 ---
>   include/hw/isa/isa.h          | 2 --
>   include/hw/southbridge/piix.h | 4 ++++
>   3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index dab901c9ad..d96ce2b788 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -35,9 +35,6 @@
>   
>   #define XEN_PIIX_NUM_PIRQS      128ULL
>   
> -#define TYPE_PIIX3_DEVICE "PIIX3"
> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> -
>   static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>   {
>       qemu_set_irq(piix3->pic[pic_irq],
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 034d706ba1..e9fa2f5cea 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -144,6 +144,4 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
>       return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
>   }
>   
> -#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> -
>   #endif
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 976b4da582..3b97186f75 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -64,6 +64,10 @@ typedef struct PIIXState PIIX3State;
>   DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>                            TYPE_PIIX3_PCI_DEVICE)
>   
> +#define TYPE_PIIX3_DEVICE "PIIX3"
> +#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> +#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> +
>   PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>   
>   DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);

One tiny nit here: there's a typo in the subject line which I missed when reviewing v2.

s/soughbridge/southbridge/


ATB,

Mark.

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

[Mark Cave-Ayland]

On 28/05/2022 20:20, Bernhard Beschow wrote:

> Just like the real hardware, create the PIIX4 ACPI controller as part of
> the PIIX4 southbridge. This also mirrors how the IDE and USB functions
> are already created.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c                | 25 ++++++++++++++-----------
>   hw/mips/malta.c               |  3 ++-
>   include/hw/southbridge/piix.h |  2 +-
>   3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 96df21a610..217989227d 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -49,6 +49,7 @@ struct PIIX4State {
>       RTCState rtc;
>       PCIIDEState ide;
>       UHCIState uhci;
> +    PIIX4PMState pm;
>       /* Reset Control Register */
>       MemoryRegion rcr_mem;
>       uint8_t rcr;
> @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>           return;
>       }
>   
> +    /* ACPI controller */
> +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
> +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> +        return;
> +    }
> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
> +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm), "i2c");

Now that the PM device is QOMified you don't actually need this alias anymore (see 
below). In general aliasing parts of contained devices onto the container isn't 
recommended, since it starts to blur the line between individual devices and then you 
find some wiring gets done to the container, some directly to the contained device 
and then it starts to get confusing quite quickly.

>       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>   }
>   
> @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
>       object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>       object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
>       object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
> +
> +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
> +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
> +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
>   }
>   
>   static void piix4_class_init(ObjectClass *klass, void *data)
> @@ -312,7 +325,7 @@ static void piix4_register_types(void)
>   
>   type_init(piix4_register_types)
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> +DeviceState *piix4_create(PCIBus *pci_bus)
>   {
>       PCIDevice *pci;
>       DeviceState *dev;
> @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>                                             TYPE_PIIX4_PCI_DEVICE);
>       dev = DEVICE(pci);
>   
> -    if (smbus) {
> -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
> -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
> -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
> -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
> -        qdev_connect_gpio_out(DEVICE(pci), 0,
> -                              qdev_get_gpio_in_named(dev, "isa", 9));
> -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
> -    }
> -
>       return dev;
>   }
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index e446b25ad0..b0fc84ccbb 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
>       empty_slot_init("GT64120", 0, 0x20000000);
>   
>       /* Southbridge */
> -    dev = piix4_create(pci_bus, &smbus);
> +    dev = piix4_create(pci_bus);
>       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));

It should now be possible to do something like this:

     pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
     smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));

whereby we grab the reference to the PIIX4_PM device by resolving the "pm" child 
object and then use that to obtain the reference to smbus.

>       /* Interrupt controller */
>       qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 0a2ef0c7b6..e1f5d6d5c8 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>   
>   PIIX3State *piix3_create(PCIBus *pci_bus);
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> +DeviceState *piix4_create(PCIBus *pci_bus);
>   
>   #endif


ATB,

Mark.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Mark Cave-Ayland]

On 28/05/2022 20:20, Bernhard Beschow wrote:

> v3:
> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1]
> * Use embedded structs for touched PCI devices (Mark)
> * Fix piix4's rtc embedded struct to be initialized by
>    object_initialize_child() (Peter) [2]
> 
> Testing done:
> 
> 1)
> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
> Result: All pass.
> 
> 2)
> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
> 
> In both cases the system booted successfully and it was possible to shut down
> the system using the `poweroff` command.
> 
> 
> v2:
> * Preserve `DeviceState *` as return value of piix4_create() (Mark)
> * Aggregate all type name movements into first commit (Mark)
> * Have piix4 southbridge rather than malta board instantiate piix4 pm (me)
> 
> Testing done:
> 
> 1)
> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
> Result: All pass.
> 
> 2)
> Modify pci_piix3_realize() to start with
>      error_setg(errp, "This is a test");
> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom
> archlinux-2022.05.01-x86_64.iso`.
> Result: qemu-system-x86_64 aborts with: "This is a test"
> 
> 
> v1:
> The piix3 and piix4 southbridge devices still rely on create() functions which
> are deprecated. This series resolves these functions piece by piece to
> modernize the code.
> 
> Both devices are modified in lockstep where possible to provide more context.
> 
> Testing done:
> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
> 
> In both cases the system booted successfully and it was possible to shut down
> the system using the `poweroff` command.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html
> 
> Bernhard Beschow (7):
>    include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
>    hw/isa/piix4: Use object_initialize_child() for embedded struct
>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
>    hw/isa/piix4: QOM'ify PIIX4 PM creation
>    hw/isa/piix{3,4}: Inline and remove create() functions
> 
>   hw/i386/pc_piix.c             |   7 +-
>   hw/isa/piix3.c                |  98 ++++++++++++++-------------
>   hw/isa/piix4.c                | 120 +++++++++++++++++-----------------
>   hw/mips/malta.c               |   7 +-
>   include/hw/isa/isa.h          |   2 -
>   include/hw/southbridge/piix.h |   6 +-
>   6 files changed, 127 insertions(+), 113 deletions(-)

Hi Bernhard,

I've spotted a couple of small things, but once those are fixed this series looks 
good to me so I'm happy to give a:

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

Thanks for your patience with these series too: you've done some good work here, 
however patchsets like this can sometimes take a while to get reviewed because they 
both i) touch legacy code/APIs and ii) cut across multiple machines and maintainers. 
I'd really like to get this work, along with your RTC updates, merged soon as it is a 
great improvement.

One reason that you may not get many reviews is because you've not added the relevant 
maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a few 
maintainers will filter based upon their own email address so it is definitely worth 
adding them in.

Fortunately you can easily find the relevant maintainer email addresses by running 
"./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git format-patch 
directory. I'd recommend doing this, and also dropping qemu-trivial since I would say 
these patches are now beyond the trivial threshold.


ATB,

Mark.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Mark Cave-Ayland]

On 29/05/2022 10:46, Mark Cave-Ayland wrote:

> On 28/05/2022 20:20, Bernhard Beschow wrote:
> 
>> v3:
>> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1]
>> * Use embedded structs for touched PCI devices (Mark)
>> * Fix piix4's rtc embedded struct to be initialized by
>>    object_initialize_child() (Peter) [2]
>>
>> Testing done:
>>
>> 1)
>> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
>> Result: All pass.
>>
>> 2)
>> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
>> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
>>
>> In both cases the system booted successfully and it was possible to shut down
>> the system using the `poweroff` command.
>>
>>
>> v2:
>> * Preserve `DeviceState *` as return value of piix4_create() (Mark)
>> * Aggregate all type name movements into first commit (Mark)
>> * Have piix4 southbridge rather than malta board instantiate piix4 pm (me)
>>
>> Testing done:
>>
>> 1)
>> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
>> Result: All pass.
>>
>> 2)
>> Modify pci_piix3_realize() to start with
>>      error_setg(errp, "This is a test");
>> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom
>> archlinux-2022.05.01-x86_64.iso`.
>> Result: qemu-system-x86_64 aborts with: "This is a test"
>>
>>
>> v1:
>> The piix3 and piix4 southbridge devices still rely on create() functions which
>> are deprecated. This series resolves these functions piece by piece to
>> modernize the code.
>>
>> Both devices are modified in lockstep where possible to provide more context.
>>
>> Testing done:
>> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
>> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
>>
>> In both cases the system booted successfully and it was possible to shut down
>> the system using the `poweroff` command.
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html
>> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html
>>
>> Bernhard Beschow (7):
>>    include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
>>    hw/isa/piix4: Use object_initialize_child() for embedded struct
>>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
>>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
>>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
>>    hw/isa/piix4: QOM'ify PIIX4 PM creation
>>    hw/isa/piix{3,4}: Inline and remove create() functions
>>
>>   hw/i386/pc_piix.c             |   7 +-
>>   hw/isa/piix3.c                |  98 ++++++++++++++-------------
>>   hw/isa/piix4.c                | 120 +++++++++++++++++-----------------
>>   hw/mips/malta.c               |   7 +-
>>   include/hw/isa/isa.h          |   2 -
>>   include/hw/southbridge/piix.h |   6 +-
>>   6 files changed, 127 insertions(+), 113 deletions(-)
> 
> Hi Bernhard,
> 
> I've spotted a couple of small things, but once those are fixed this series looks 
> good to me so I'm happy to give a:
> 
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks for your patience with these series too: you've done some good work here, 
> however patchsets like this can sometimes take a while to get reviewed because they 
> both i) touch legacy code/APIs and ii) cut across multiple machines and maintainers. 
> I'd really like to get this work, along with your RTC updates, merged soon as it is a 
> great improvement.
> 
> One reason that you may not get many reviews is because you've not added the relevant 
> maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a few 
> maintainers will filter based upon their own email address so it is definitely worth 
> adding them in.
> 
> Fortunately you can easily find the relevant maintainer email addresses by running 
> "./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git format-patch 
> directory. I'd recommend doing this, and also dropping qemu-trivial since I would say 
> these patches are now beyond the trivial threshold.

Oh wait - I see now it's just the cover letter which is missing the additional 
maintainer addresses :)  If you could add them into the cover letter for your next 
revision that would be great, since it gives context for maintainers to help with the 
review process.


ATB,

Mark.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 5459 bytes --]

On Sun, May 29, 2022 at 12:06 PM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 29/05/2022 10:46, Mark Cave-Ayland wrote:
>
> > On 28/05/2022 20:20, Bernhard Beschow wrote:
> >
> >> v3:
> >> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function'
> (Mark) [1]
> >> * Use embedded structs for touched PCI devices (Mark)
> >> * Fix piix4's rtc embedded struct to be initialized by
> >>    object_initialize_child() (Peter) [2]
> >>
> >> Testing done:
> >>
> >> 1)
> >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
> >> Result: All pass.
> >>
> >> 2)
> >> * `qemu-system-x86_64 -M pc -m 2G -cdrom
> archlinux-2022.05.01-x86_64.iso`
> >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
> >>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1
> console=tty0"`
> >>
> >> In both cases the system booted successfully and it was possible to
> shut down
> >> the system using the `poweroff` command.
> >>
> >>
> >> v2:
> >> * Preserve `DeviceState *` as return value of piix4_create() (Mark)
> >> * Aggregate all type name movements into first commit (Mark)
> >> * Have piix4 southbridge rather than malta board instantiate piix4 pm
> (me)
> >>
> >> Testing done:
> >>
> >> 1)
> >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
> >> Result: All pass.
> >>
> >> 2)
> >> Modify pci_piix3_realize() to start with
> >>      error_setg(errp, "This is a test");
> >> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom
> >> archlinux-2022.05.01-x86_64.iso`.
> >> Result: qemu-system-x86_64 aborts with: "This is a test"
> >>
> >>
> >> v1:
> >> The piix3 and piix4 southbridge devices still rely on create()
> functions which
> >> are deprecated. This series resolves these functions piece by piece to
> >> modernize the code.
> >>
> >> Both devices are modified in lockstep where possible to provide more
> context.
> >>
> >> Testing done:
> >> * `qemu-system-x86_64 -M pc -m 2G -cdrom
> archlinux-2022.05.01-x86_64.iso`
> >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
> >>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1
> console=tty0"`
> >>
> >> In both cases the system booted successfully and it was possible to
> shut down
> >> the system using the `poweroff` command.
> >>
> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html
> >> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html
> >>
> >> Bernhard Beschow (7):
> >>    include/hw/southbridge/piix: Aggregate all PIIX soughbridge type
> names
> >>    hw/isa/piix4: Use object_initialize_child() for embedded struct
> >>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
> >>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
> >>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
> >>    hw/isa/piix4: QOM'ify PIIX4 PM creation
> >>    hw/isa/piix{3,4}: Inline and remove create() functions
> >>
> >>   hw/i386/pc_piix.c             |   7 +-
> >>   hw/isa/piix3.c                |  98 ++++++++++++++-------------
> >>   hw/isa/piix4.c                | 120 +++++++++++++++++-----------------
> >>   hw/mips/malta.c               |   7 +-
> >>   include/hw/isa/isa.h          |   2 -
> >>   include/hw/southbridge/piix.h |   6 +-
> >>   6 files changed, 127 insertions(+), 113 deletions(-)
> >
> > Hi Bernhard,
> >
> > I've spotted a couple of small things, but once those are fixed this
> series looks
> > good to me so I'm happy to give a:
> >
> > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >
> > Thanks for your patience with these series too: you've done some good
> work here,
> > however patchsets like this can sometimes take a while to get reviewed
> because they
> > both i) touch legacy code/APIs and ii) cut across multiple machines and
> maintainers.
> > I'd really like to get this work, along with your RTC updates, merged
> soon as it is a
> > great improvement.
> >
> > One reason that you may not get many reviews is because you've not added
> the relevant
> > maintainers on To/CC. Due to the large volume of emails on qemu-devel,
> quite a few
> > maintainers will filter based upon their own email address so it is
> definitely worth
> > adding them in.
> >
> > Fortunately you can easily find the relevant maintainer email addresses
> by running
> > "./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git
> format-patch
> > directory. I'd recommend doing this, and also dropping qemu-trivial
> since I would say
> > these patches are now beyond the trivial threshold.
>
> Oh wait - I see now it's just the cover letter which is missing the
> additional
> maintainer addresses :)  If you could add them into the cover letter for
> your next
> revision that would be great, since it gives context for maintainers to
> help with the
> review process.
>

Hi Mark,

Thanks for your great work you put into reviews and the useful insights! It
seems to me that the time it takes for patches to be queued depends on the
subsystem - some are faster, some are slower...

I've automated my setup as described in [1]. However, it doesn't seem to
work for the cover letter which I'd expect to be sent to the union of all
reviewers of all patches. Any idea how to fix this?

Best regards,
Bernhard

[1]
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer

>
>
> ATB,
>
> Mark.
>

[-- Attachment #2: Type: text/html, Size: 7551 bytes --]

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

[Bernhard Beschow]

On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 28/05/2022 20:20, Bernhard Beschow wrote:
>
> > Just like the real hardware, create the PIIX4 ACPI controller as part of
> > the PIIX4 southbridge. This also mirrors how the IDE and USB functions
> > are already created.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >   hw/isa/piix4.c                | 25 ++++++++++++++-----------
> >   hw/mips/malta.c               |  3 ++-
> >   include/hw/southbridge/piix.h |  2 +-
> >   3 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 96df21a610..217989227d 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -49,6 +49,7 @@ struct PIIX4State {
> >       RTCState rtc;
> >       PCIIDEState ide;
> >       UHCIState uhci;
> > +    PIIX4PMState pm;
> >       /* Reset Control Register */
> >       MemoryRegion rcr_mem;
> >       uint8_t rcr;
> > @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error
> **errp)
> >           return;
> >       }
> >
> > +    /* ACPI controller */
> > +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
> > +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> > +        return;
> > +    }
> > +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
> > +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm),
> "i2c");
>
> Now that the PM device is QOMified you don't actually need this alias
> anymore (see
> below). In general aliasing parts of contained devices onto the container
> isn't
> recommended, since it starts to blur the line between individual devices
> and then you
> find some wiring gets done to the container, some directly to the
> contained device
> and then it starts to get confusing quite quickly.
>
> >       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
> PIIX_NUM_PIRQS);
> >   }
> >
> > @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
> >       object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
> >       object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
> >       object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
> > +
> > +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
> > +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
> > +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
> >   }
> >
> >   static void piix4_class_init(ObjectClass *klass, void *data)
> > @@ -312,7 +325,7 @@ static void piix4_register_types(void)
> >
> >   type_init(piix4_register_types)
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> > +DeviceState *piix4_create(PCIBus *pci_bus)
> >   {
> >       PCIDevice *pci;
> >       DeviceState *dev;
> > @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus
> **smbus)
> >                                             TYPE_PIIX4_PCI_DEVICE);
> >       dev = DEVICE(pci);
> >
> > -    if (smbus) {
> > -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
> > -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
> > -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
> > -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
> > -        qdev_connect_gpio_out(DEVICE(pci), 0,
> > -                              qdev_get_gpio_in_named(dev, "isa", 9));
> > -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
> > -    }
> > -
> >       return dev;
> >   }
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index e446b25ad0..b0fc84ccbb 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
> >       empty_slot_init("GT64120", 0, 0x20000000);
> >
> >       /* Southbridge */
> > -    dev = piix4_create(pci_bus, &smbus);
> > +    dev = piix4_create(pci_bus);
> >       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> > +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
>
> It should now be possible to do something like this:
>
>      pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>      smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));
>
> whereby we grab the reference to the PIIX4_PM device by resolving the "pm"
> child
> object and then use that to obtain the reference to smbus.
>

Imagining the container device to be an abstraction layer for the
contained functionality I actually prefer the alias. Having it one
doesn't need to know that there is an internal device named "pm" and
one doesn't need to care about how many levels deep it is buried
inside the implementation. This allows for further refactoring the
PIIX4 without breaking its contract to its users.

Also, this reflects how the real hardware works: The Malta board can
wire up the PIIX4 southbridge without worrying about its inner
details. One can look into the datasheets, see the exposed interfaces
and pins, and connect them. By QOM'ifying PIIX4 we now have the
opportunity to mirror the real hardware by exposing it to the Malta
board as a black box which exposes dedicated pins and buses.

Looking further into the QEMU code, there is even the following
comment in sabrelite.c:
        /*
         * TODO: Ideally we would expose the chip select and spi bus on the
         * SoC object using alias properties; then we would not need to
         * directly access the underlying spi device object.
         */
To me this comment seems that the authors think in a similar way.

What do you think?

Best regards,
Bernhard





> >       /* Interrupt controller */
> >       qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> > diff --git a/include/hw/southbridge/piix.h
> b/include/hw/southbridge/piix.h
> > index 0a2ef0c7b6..e1f5d6d5c8 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> >
> >   PIIX3State *piix3_create(PCIBus *pci_bus);
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> > +DeviceState *piix4_create(PCIBus *pci_bus);
> >
> >   #endif
>
>
> ATB,
>
> Mark.
>

Re: [PATCH v3 1/7] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On Sun, May 29, 2022 at 11:05 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 28/05/2022 20:20, Bernhard Beschow wrote:
>
> > TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
> > ones, too.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   hw/isa/piix3.c                | 3 ---
> >   include/hw/isa/isa.h          | 2 --
> >   include/hw/southbridge/piix.h | 4 ++++
> >   3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> > index dab901c9ad..d96ce2b788 100644
> > --- a/hw/isa/piix3.c
> > +++ b/hw/isa/piix3.c
> > @@ -35,9 +35,6 @@
> >
> >   #define XEN_PIIX_NUM_PIRQS      128ULL
> >
> > -#define TYPE_PIIX3_DEVICE "PIIX3"
> > -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> > -
> >   static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> >   {
> >       qemu_set_irq(piix3->pic[pic_irq],
> > diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> > index 034d706ba1..e9fa2f5cea 100644
> > --- a/include/hw/isa/isa.h
> > +++ b/include/hw/isa/isa.h
> > @@ -144,6 +144,4 @@ static inline ISABus *isa_bus_from_device(ISADevice
> *d)
> >       return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
> >   }
> >
> > -#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> > -
> >   #endif
> > diff --git a/include/hw/southbridge/piix.h
> b/include/hw/southbridge/piix.h
> > index 976b4da582..3b97186f75 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -64,6 +64,10 @@ typedef struct PIIXState PIIX3State;
> >   DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> >                            TYPE_PIIX3_PCI_DEVICE)
> >
> > +#define TYPE_PIIX3_DEVICE "PIIX3"
> > +#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> > +#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> > +
> >   PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
> >
> >   DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus
> **smbus);
>
> One tiny nit here: there's a typo in the subject line which I missed when
> reviewing v2.
>
> s/soughbridge/southbridge/
>

Ack. Will fix in v3.

Best regards,
Bernhard

>
>
> ATB,
>
> Mark.
>

[-- Attachment #2: Type: text/html, Size: 3443 bytes --]

Re: [PATCH v3 2/7] hw/isa/piix4: Use object_initialize_child() for embedded struct

[Philippe Mathieu-Daudé via]

On 28/5/22 21:20, Bernhard Beschow wrote:
> Found-by: Peter Maydell <peter.maydell@linaro.org>

I suppose you refer to this thread:
https://lore.kernel.org/qemu-devel/CAFEAcA_y69=iXMH75dHeNkxMa038Z7Xk63GW9fdcAFHJSWS=sA@mail.gmail.com/

I'm going to change the tag to "Reported-by".

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

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 9a6d981037..1d04fb6a55 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -224,7 +224,7 @@ static void piix4_init(Object *obj)
>   {
>       PIIX4State *s = PIIX4_PCI_DEVICE(obj);
>   
> -    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>   }
>   
>   static void piix4_class_init(ObjectClass *klass, void *data)

Re: [PATCH v3 4/7] hw/isa/piix{3, 4}: QOM'ify PCI device creation and wiring

[Philippe Mathieu-Daudé via]

Hi Bernhard,

On 28/5/22 21:20, Bernhard Beschow wrote:
> PCI interrupt wiring and device creation (piix4 only) were performed
> in create() functions which are obsolete. Move these tasks into QOM
> functions to modernize the code.
> 
> In order to avoid duplicate checking for xen_enabled() the piix3 realize
> methods are now split.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix3.c | 67 +++++++++++++++++++++++++++++++++-----------------
>   hw/isa/piix4.c | 30 ++++++++++++++++------
>   2 files changed, 67 insertions(+), 30 deletions(-)

While this is the same chipset family, these models are maintained by
different people... Do you mind splitting?

For PIIX4 part:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 1/7] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Philippe Mathieu-Daudé via]

On 29/5/22 20:09, Bernhard Beschow wrote:
> On Sun, May 29, 2022 at 11:05 AM Mark Cave-Ayland 
> <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>> 
> wrote:
> 
>     On 28/05/2022 20:20, Bernhard Beschow wrote:
> 
>      > TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
>      > ones, too.
>      >
>      > Signed-off-by: Bernhard Beschow <shentey@gmail.com
>     <mailto:shentey@gmail.com>>
>      > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>     <mailto:mark.cave-ayland@ilande.co.uk>>
>      > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>     <mailto:f4bug@amsat.org>>
>      > ---
>      >   hw/isa/piix3.c                | 3 ---
>      >   include/hw/isa/isa.h          | 2 --
>      >   include/hw/southbridge/piix.h | 4 ++++
>      >   3 files changed, 4 insertions(+), 5 deletions(-)

>     One tiny nit here: there's a typo in the subject line which I missed
>     when reviewing v2.
> 
>     s/soughbridge/southbridge/
> 
> 
> Ack. Will fix in v3.

Can do. Also, "include/" in subject is not helpful.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Mark Cave-Ayland]

On 29/05/2022 14:02, Bernhard Beschow wrote:

> On Sun, May 29, 2022 at 12:06 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
> <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
> 
>     On 29/05/2022 10:46, Mark Cave-Ayland wrote:
> 
>      > On 28/05/2022 20:20, Bernhard Beschow wrote:
>      >
>      >> v3:
>      >> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1]
>      >> * Use embedded structs for touched PCI devices (Mark)
>      >> * Fix piix4's rtc embedded struct to be initialized by
>      >>    object_initialize_child() (Peter) [2]
>      >>
>      >> Testing done:
>      >>
>      >> 1)
>      >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
>      >> Result: All pass.
>      >>
>      >> 2)
>      >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
>      >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>      >>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
>      >>
>      >> In both cases the system booted successfully and it was possible to shut down
>      >> the system using the `poweroff` command.
>      >>
>      >>
>      >> v2:
>      >> * Preserve `DeviceState *` as return value of piix4_create() (Mark)
>      >> * Aggregate all type name movements into first commit (Mark)
>      >> * Have piix4 southbridge rather than malta board instantiate piix4 pm (me)
>      >>
>      >> Testing done:
>      >>
>      >> 1)
>      >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu
>      >> Result: All pass.
>      >>
>      >> 2)
>      >> Modify pci_piix3_realize() to start with
>      >>      error_setg(errp, "This is a test");
>      >> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom
>      >> archlinux-2022.05.01-x86_64.iso`.
>      >> Result: qemu-system-x86_64 aborts with: "This is a test"
>      >>
>      >>
>      >> v1:
>      >> The piix3 and piix4 southbridge devices still rely on create() functions which
>      >> are deprecated. This series resolves these functions piece by piece to
>      >> modernize the code.
>      >>
>      >> Both devices are modified in lockstep where possible to provide more context.
>      >>
>      >> Testing done:
>      >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
>      >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda
>      >>    debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
>      >>
>      >> In both cases the system booted successfully and it was possible to shut down
>      >> the system using the `poweroff` command.
>      >>
>      >> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html>
>      >> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html>
>      >>
>      >> Bernhard Beschow (7):
>      >>    include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
>      >>    hw/isa/piix4: Use object_initialize_child() for embedded struct
>      >>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
>      >>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
>      >>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
>      >>    hw/isa/piix4: QOM'ify PIIX4 PM creation
>      >>    hw/isa/piix{3,4}: Inline and remove create() functions
>      >>
>      >>   hw/i386/pc_piix.c             |   7 +-
>      >>   hw/isa/piix3.c                |  98 ++++++++++++++-------------
>      >>   hw/isa/piix4.c                | 120 +++++++++++++++++-----------------
>      >>   hw/mips/malta.c               |   7 +-
>      >>   include/hw/isa/isa.h          |   2 -
>      >>   include/hw/southbridge/piix.h |   6 +-
>      >>   6 files changed, 127 insertions(+), 113 deletions(-)
>      >
>      > Hi Bernhard,
>      >
>      > I've spotted a couple of small things, but once those are fixed this series looks
>      > good to me so I'm happy to give a:
>      >
>      > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>     <mailto:mark.cave-ayland@ilande.co.uk>>
>      >
>      > Thanks for your patience with these series too: you've done some good work here,
>      > however patchsets like this can sometimes take a while to get reviewed because
>     they
>      > both i) touch legacy code/APIs and ii) cut across multiple machines and
>     maintainers.
>      > I'd really like to get this work, along with your RTC updates, merged soon as
>     it is a
>      > great improvement.
>      >
>      > One reason that you may not get many reviews is because you've not added the
>     relevant
>      > maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a
>     few
>      > maintainers will filter based upon their own email address so it is definitely
>     worth
>      > adding them in.
>      >
>      > Fortunately you can easily find the relevant maintainer email addresses by
>     running
>      > "./scripts/get_maintainer.pl <http://get_maintainer.pl>
>     <path-to-git-patch-dir>" on your git format-patch
>      > directory. I'd recommend doing this, and also dropping qemu-trivial since I
>     would say
>      > these patches are now beyond the trivial threshold.
> 
>     Oh wait - I see now it's just the cover letter which is missing the additional
>     maintainer addresses :)  If you could add them into the cover letter for your next
>     revision that would be great, since it gives context for maintainers to help with
>     the
>     review process.
> 
> 
> Hi Mark,
> 
> Thanks for your great work you put into reviews and the useful insights! It seems to 
> me that the time it takes for patches to be queued depends on the subsystem - some 
> are faster, some are slower...
> 
> I've automated my setup as described in [1]. However, it doesn't seem to work for the 
> cover letter which I'd expect to be sent to the union of all reviewers of all 
> patches. Any idea how to fix this?
> 
> Best regards,
> Bernhard
> 
> [1] 
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer 
> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> 

Good question. I tend to do "git format-patch -o /tmp/foo --cover-letter" to generate 
the series, fill in the cover letter, and then use "git send-email /tmp/foo" to send 
out the emails (entering in the results of get_maintainer.pl by hand). I'm not sure 
why the cover letter isn't being generated correctly in your case I'm afraid.


ATB,

Mark.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Philippe Mathieu-Daudé via]

On 30/5/22 21:11, Mark Cave-Ayland wrote:
> On 29/05/2022 14:02, Bernhard Beschow wrote:

>>     Oh wait - I see now it's just the cover letter which is missing 
>> the additional
>>     maintainer addresses :)  If you could add them into the cover 
>> letter for your next
>>     revision that would be great, since it gives context for 
>> maintainers to help with
>>     the
>>     review process.
>>
>>
>> Hi Mark,
>>
>> Thanks for your great work you put into reviews and the useful 
>> insights! It seems to me that the time it takes for patches to be 
>> queued depends on the subsystem - some are faster, some are slower...
>>
>> I've automated my setup as described in [1]. However, it doesn't seem 
>> to work for the cover letter which I'd expect to be sent to the union 
>> of all reviewers of all patches. Any idea how to fix this?
>>
>> Best regards,
>> Bernhard
>>
>> [1] 
>> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer 
>> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> 
> 
> 
> Good question. I tend to do "git format-patch -o /tmp/foo 
> --cover-letter" to generate the series, fill in the cover letter, and 
> then use "git send-email /tmp/foo" to send out the emails (entering in 
> the results of get_maintainer.pl by hand). I'm not sure why the cover 
> letter isn't being generated correctly in your case I'm afraid.

Or try git-publish :) It does a first pass collecting Cc for each patch
(calling get_maintainer.pl) then use that set on the cover.

https://github.com/stefanha/git-publish

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

[Mark Cave-Ayland]

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

> On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland <
> mark.cave-ayland@ilande.co.uk> wrote:
> 
>> On 28/05/2022 20:20, Bernhard Beschow wrote:
>>
>>> Just like the real hardware, create the PIIX4 ACPI controller as part of
>>> the PIIX4 southbridge. This also mirrors how the IDE and USB functions
>>> are already created.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>    hw/isa/piix4.c                | 25 ++++++++++++++-----------
>>>    hw/mips/malta.c               |  3 ++-
>>>    include/hw/southbridge/piix.h |  2 +-
>>>    3 files changed, 17 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>> index 96df21a610..217989227d 100644
>>> --- a/hw/isa/piix4.c
>>> +++ b/hw/isa/piix4.c
>>> @@ -49,6 +49,7 @@ struct PIIX4State {
>>>        RTCState rtc;
>>>        PCIIDEState ide;
>>>        UHCIState uhci;
>>> +    PIIX4PMState pm;
>>>        /* Reset Control Register */
>>>        MemoryRegion rcr_mem;
>>>        uint8_t rcr;
>>> @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error
>> **errp)
>>>            return;
>>>        }
>>>
>>> +    /* ACPI controller */
>>> +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
>>> +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>>> +        return;
>>> +    }
>>> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
>>> +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm),
>> "i2c");
>>
>> Now that the PM device is QOMified you don't actually need this alias
>> anymore (see
>> below). In general aliasing parts of contained devices onto the container
>> isn't
>> recommended, since it starts to blur the line between individual devices
>> and then you
>> find some wiring gets done to the container, some directly to the
>> contained device
>> and then it starts to get confusing quite quickly.
>>
>>>        pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
>> PIIX_NUM_PIRQS);
>>>    }
>>>
>>> @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
>>>        object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>        object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
>>>        object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
>>> +
>>> +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
>>> +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
>>> +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
>>>    }
>>>
>>>    static void piix4_class_init(ObjectClass *klass, void *data)
>>> @@ -312,7 +325,7 @@ static void piix4_register_types(void)
>>>
>>>    type_init(piix4_register_types)
>>>
>>> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>>> +DeviceState *piix4_create(PCIBus *pci_bus)
>>>    {
>>>        PCIDevice *pci;
>>>        DeviceState *dev;
>>> @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus
>> **smbus)
>>>                                              TYPE_PIIX4_PCI_DEVICE);
>>>        dev = DEVICE(pci);
>>>
>>> -    if (smbus) {
>>> -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
>>> -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
>>> -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
>>> -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
>>> -        qdev_connect_gpio_out(DEVICE(pci), 0,
>>> -                              qdev_get_gpio_in_named(dev, "isa", 9));
>>> -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
>>> -    }
>>> -
>>>        return dev;
>>>    }
>>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>>> index e446b25ad0..b0fc84ccbb 100644
>>> --- a/hw/mips/malta.c
>>> +++ b/hw/mips/malta.c
>>> @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
>>>        empty_slot_init("GT64120", 0, 0x20000000);
>>>
>>>        /* Southbridge */
>>> -    dev = piix4_create(pci_bus, &smbus);
>>> +    dev = piix4_create(pci_bus);
>>>        isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>> +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
>>
>> It should now be possible to do something like this:
>>
>>       pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>>       smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));
>>
>> whereby we grab the reference to the PIIX4_PM device by resolving the "pm"
>> child
>> object and then use that to obtain the reference to smbus.
>>
> 
> Imagining the container device to be an abstraction layer for the
> contained functionality I actually prefer the alias. Having it one
> doesn't need to know that there is an internal device named "pm" and
> one doesn't need to care about how many levels deep it is buried
> inside the implementation. This allows for further refactoring the
> PIIX4 without breaking its contract to its users.
> 
> Also, this reflects how the real hardware works: The Malta board can
> wire up the PIIX4 southbridge without worrying about its inner
> details. One can look into the datasheets, see the exposed interfaces
> and pins, and connect them. By QOM'ifying PIIX4 we now have the
> opportunity to mirror the real hardware by exposing it to the Malta
> board as a black box which exposes dedicated pins and buses.

It's a bit trickier in this particular case because here the PIIX4 device acts as 
both a container and PCI device instance - and certainly the distinction can be 
blurred when modelling integrated devices.

The reason for leaning towards referencing the "pm" child object directly is because 
a quick glance at the datasheet suggests that the PM functions and smbus are exposed 
via function 3, so it feels that referencing that PCIDevice instance to find the 
smbus is intuitive.

For me using the "pm" child object is a shorter alternative to using 
pci_find_device() to locate the function 3 PCIDevice function on the bus, which is 
effectively what a guest OS would do. Since the PM device sits directly on the PCI 
bus it would never change depth within the PIIX4 container device, and renaming child 
object properties is an extremely rare event (which would also be easily fixable with 
grep).

> Looking further into the QEMU code, there is even the following
> comment in sabrelite.c:
>          /*
>           * TODO: Ideally we would expose the chip select and spi bus on the
>           * SoC object using alias properties; then we would not need to
>           * directly access the underlying spi device object.
>           */
> To me this comment seems that the authors think in a similar way.
> 
> What do you think?

It's difficult for me to know without having much knowledge of ARM devices, but I can 
see how this might be useful given that SoCs can integrate a lot of different devices 
into a single address space. Another thing to note is that the comment above was 
originally written in 2016, and the qdev/QOM APIs (and indeed our knowledge as to how 
best to use them) have improved considerably since then.


ATB,

Mark.

Re: [PATCH v3 4/7] hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring

[Bernhard Beschow]

Am 30. Mai 2022 13:17:12 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>Hi Bernhard,

Hi Philippe,

>On 28/5/22 21:20, Bernhard Beschow wrote:
>> PCI interrupt wiring and device creation (piix4 only) were performed
>> in create() functions which are obsolete. Move these tasks into QOM
>> functions to modernize the code.
>> 
>> In order to avoid duplicate checking for xen_enabled() the piix3 realize
>> methods are now split.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/piix3.c | 67 +++++++++++++++++++++++++++++++++-----------------
>>   hw/isa/piix4.c | 30 ++++++++++++++++------
>>   2 files changed, 67 insertions(+), 30 deletions(-)
>
>While this is the same chipset family, these models are maintained by
>different people... Do you mind splitting?

Will do. I'd split the whole series then.

>For PIIX4 part:
>Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>

Re: [PATCH v3 1/7] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Bernhard Beschow]

Am 30. Mai 2022 13:19:33 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 29/5/22 20:09, Bernhard Beschow wrote:
>> On Sun, May 29, 2022 at 11:05 AM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
>> 
>>     On 28/05/2022 20:20, Bernhard Beschow wrote:
>> 
>>      > TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
>>      > ones, too.
>>      >
>>      > Signed-off-by: Bernhard Beschow <shentey@gmail.com
>>     <mailto:shentey@gmail.com>>
>>      > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>>     <mailto:mark.cave-ayland@ilande.co.uk>>
>>      > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>>     <mailto:f4bug@amsat.org>>
>>      > ---
>>      >   hw/isa/piix3.c                | 3 ---
>>      >   include/hw/isa/isa.h          | 2 --
>>      >   include/hw/southbridge/piix.h | 4 ++++
>>      >   3 files changed, 4 insertions(+), 5 deletions(-)
>
>>     One tiny nit here: there's a typo in the subject line which I missed
>>     when reviewing v2.
>> 
>>     s/soughbridge/southbridge/
>> 
>> 
>> Ack. Will fix in v3.
>
>Can do. Also, "include/" in subject is not helpful.

I'll take care of this in v4.

Thanks,
Bernhard

Re: [PATCH v3 2/7] hw/isa/piix4: Use object_initialize_child() for embedded struct

[Bernhard Beschow]

Am 30. Mai 2022 11:38:30 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 28/5/22 21:20, Bernhard Beschow wrote:
>> Found-by: Peter Maydell <peter.maydell@linaro.org>
>
>I suppose you refer to this thread:
>https://lore.kernel.org/qemu-devel/CAFEAcA_y69=iXMH75dHeNkxMa038Z7Xk63GW9fdcAFHJSWS=sA@mail.gmail.com/

Yes, correct.

>I'm going to change the tag to "Reported-by".

I'll take care of this in v4.

>Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/piix4.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 9a6d981037..1d04fb6a55 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -224,7 +224,7 @@ static void piix4_init(Object *obj)
>>   {
>>       PIIX4State *s = PIIX4_PCI_DEVICE(obj);
>>   -    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>   }
>>     static void piix4_class_init(ObjectClass *klass, void *data)

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

[Bernhard Beschow]

Am 30. Mai 2022 19:58:37 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 29/05/2022 19:05, Bernhard Beschow wrote:
>
>> On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland <
>> mark.cave-ayland@ilande.co.uk> wrote:
>> 
>>> On 28/05/2022 20:20, Bernhard Beschow wrote:
>>> 
>>>> Just like the real hardware, create the PIIX4 ACPI controller as part of
>>>> the PIIX4 southbridge. This also mirrors how the IDE and USB functions
>>>> are already created.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>    hw/isa/piix4.c                | 25 ++++++++++++++-----------
>>>>    hw/mips/malta.c               |  3 ++-
>>>>    include/hw/southbridge/piix.h |  2 +-
>>>>    3 files changed, 17 insertions(+), 13 deletions(-)
>>>> 
>>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>>> index 96df21a610..217989227d 100644
>>>> --- a/hw/isa/piix4.c
>>>> +++ b/hw/isa/piix4.c
>>>> @@ -49,6 +49,7 @@ struct PIIX4State {
>>>>        RTCState rtc;
>>>>        PCIIDEState ide;
>>>>        UHCIState uhci;
>>>> +    PIIX4PMState pm;
>>>>        /* Reset Control Register */
>>>>        MemoryRegion rcr_mem;
>>>>        uint8_t rcr;
>>>> @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error
>>> **errp)
>>>>            return;
>>>>        }
>>>> 
>>>> +    /* ACPI controller */
>>>> +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
>>>> +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>>>> +        return;
>>>> +    }
>>>> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
>>>> +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm),
>>> "i2c");
>>> 
>>> Now that the PM device is QOMified you don't actually need this alias
>>> anymore (see
>>> below). In general aliasing parts of contained devices onto the container
>>> isn't
>>> recommended, since it starts to blur the line between individual devices
>>> and then you
>>> find some wiring gets done to the container, some directly to the
>>> contained device
>>> and then it starts to get confusing quite quickly.
>>> 
>>>>        pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
>>> PIIX_NUM_PIRQS);
>>>>    }
>>>> 
>>>> @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
>>>>        object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>        object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
>>>>        object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
>>>> +
>>>> +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
>>>> +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
>>>> +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
>>>>    }
>>>> 
>>>>    static void piix4_class_init(ObjectClass *klass, void *data)
>>>> @@ -312,7 +325,7 @@ static void piix4_register_types(void)
>>>> 
>>>>    type_init(piix4_register_types)
>>>> 
>>>> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>>>> +DeviceState *piix4_create(PCIBus *pci_bus)
>>>>    {
>>>>        PCIDevice *pci;
>>>>        DeviceState *dev;
>>>> @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus
>>> **smbus)
>>>>                                              TYPE_PIIX4_PCI_DEVICE);
>>>>        dev = DEVICE(pci);
>>>> 
>>>> -    if (smbus) {
>>>> -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
>>>> -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
>>>> -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
>>>> -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
>>>> -        qdev_connect_gpio_out(DEVICE(pci), 0,
>>>> -                              qdev_get_gpio_in_named(dev, "isa", 9));
>>>> -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
>>>> -    }
>>>> -
>>>>        return dev;
>>>>    }
>>>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>>>> index e446b25ad0..b0fc84ccbb 100644
>>>> --- a/hw/mips/malta.c
>>>> +++ b/hw/mips/malta.c
>>>> @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
>>>>        empty_slot_init("GT64120", 0, 0x20000000);
>>>> 
>>>>        /* Southbridge */
>>>> -    dev = piix4_create(pci_bus, &smbus);
>>>> +    dev = piix4_create(pci_bus);
>>>>        isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>>> +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
>>> 
>>> It should now be possible to do something like this:
>>> 
>>>       pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>>>       smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));
>>> 
>>> whereby we grab the reference to the PIIX4_PM device by resolving the "pm"
>>> child
>>> object and then use that to obtain the reference to smbus.
>>> 
>> 
>> Imagining the container device to be an abstraction layer for the
>> contained functionality I actually prefer the alias. Having it one
>> doesn't need to know that there is an internal device named "pm" and
>> one doesn't need to care about how many levels deep it is buried
>> inside the implementation. This allows for further refactoring the
>> PIIX4 without breaking its contract to its users.
>> 
>> Also, this reflects how the real hardware works: The Malta board can
>> wire up the PIIX4 southbridge without worrying about its inner
>> details. One can look into the datasheets, see the exposed interfaces
>> and pins, and connect them. By QOM'ifying PIIX4 we now have the
>> opportunity to mirror the real hardware by exposing it to the Malta
>> board as a black box which exposes dedicated pins and buses.
>
>It's a bit trickier in this particular case because here the PIIX4 device acts as both a container and PCI device instance - and certainly the distinction can be blurred when modelling integrated devices.
>
>The reason for leaning towards referencing the "pm" child object directly is because a quick glance at the datasheet suggests that the PM functions and smbus are exposed via function 3, so it feels that referencing that PCIDevice instance to find the smbus is intuitive.
>
>For me using the "pm" child object is a shorter alternative to using pci_find_device() to locate the function 3 PCIDevice function on the bus, which is effectively what a guest OS would do. Since the PM device sits directly on the PCI bus it would never change depth within the PIIX4 container device, and renaming child object properties is an extremely rare event (which would also be easily fixable with grep).

Alright, I'll implement it like this in v4.

Best regards,
Bernhard

>> Looking further into the QEMU code, there is even the following
>> comment in sabrelite.c:
>>          /*
>>           * TODO: Ideally we would expose the chip select and spi bus on the
>>           * SoC object using alias properties; then we would not need to
>>           * directly access the underlying spi device object.
>>           */
>> To me this comment seems that the authors think in a similar way.
>> 
>> What do you think?
>
>It's difficult for me to know without having much knowledge of ARM devices, but I can see how this might be useful given that SoCs can integrate a lot of different devices into a single address space. Another thing to note is that the comment above was originally written in 2016, and the qdev/QOM APIs (and indeed our knowledge as to how best to use them) have improved considerably since then.
>
>
>ATB,
>
>Mark.

Re: [PATCH v3 0/7] QOM'ify PIIX southbridge creation

[Bernhard Beschow]

Am 30. Mai 2022 19:45:26 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 30/5/22 21:11, Mark Cave-Ayland wrote:
>> On 29/05/2022 14:02, Bernhard Beschow wrote:
>
>>>     Oh wait - I see now it's just the cover letter which is missing the additional
>>>     maintainer addresses :)  If you could add them into the cover letter for your next
>>>     revision that would be great, since it gives context for maintainers to help with
>>>     the
>>>     review process.
>>> 
>>> 
>>> Hi Mark,
>>> 
>>> Thanks for your great work you put into reviews and the useful insights! It seems to me that the time it takes for patches to be queued depends on the subsystem - some are faster, some are slower...
>>> 
>>> I've automated my setup as described in [1]. However, it doesn't seem to work for the cover letter which I'd expect to be sent to the union of all reviewers of all patches. Any idea how to fix this?
>>> 
>>> Best regards,
>>> Bernhard
>>> 
>>> [1] https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> 
>> 
>> 
>> Good question. I tend to do "git format-patch -o /tmp/foo --cover-letter" to generate the series, fill in the cover letter, and then use "git send-email /tmp/foo" to send out the emails (entering in the results of get_maintainer.pl by hand). I'm not sure why the cover letter isn't being generated correctly in your case I'm afraid.
>
>Or try git-publish :) It does a first pass collecting Cc for each patch
>(calling get_maintainer.pl) then use that set on the cover.
>
>https://github.com/stefanha/git-publish

Yes, that worked. What an improvement!

Best regards,
Bernhard