[PATCH 0/6] QOM'ify PIIX southbridge creation

[PATCH 0/6] QOM'ify PIIX southbridge creation

[Bernhard Beschow]

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.

Bernhard Beschow (6):
  include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
  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: Factor out SM bus initialization from create() function
  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                | 91 +++++++++++++-------------------
 hw/mips/malta.c               |  9 +++-
 include/hw/isa/isa.h          |  2 -
 include/hw/southbridge/piix.h |  6 +--
 6 files changed, 105 insertions(+), 108 deletions(-)

-- 
2.36.1

[PATCH 1/6] include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h

[Bernhard Beschow]

TYPE_PIIX3_PCI_DEVICE resides there as well.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/isa/isa.h          | 2 --
 include/hw/southbridge/piix.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

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 f63f83e5c6..a304fc6041 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -70,6 +70,8 @@ typedef struct PIIXState PIIX3State;
 DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
                          TYPE_PIIX3_PCI_DEVICE)
 
+#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

Re: [PATCH 1/6] include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> TYPE_PIIX3_PCI_DEVICE resides there as well.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/isa/isa.h          | 2 --
>   include/hw/southbridge/piix.h | 2 ++
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 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 f63f83e5c6..a304fc6041 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,6 +70,8 @@ typedef struct PIIXState PIIX3State;
>   DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>                            TYPE_PIIX3_PCI_DEVICE)
>   
> +#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);

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


ATB,

Mark.

[PATCH 2/6] 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>
---
 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 dab901c9ad..7d69420967 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -81,6 +81,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;
@@ -353,17 +364,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 8607e0ac36..a223b69e24 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -73,6 +73,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);
@@ -265,31 +290,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

Re: [PATCH 2/6] hw/isa/piix{3, 4}: Move pci_map_irq_fn's near pci_set_irq_fn's

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> 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>
> ---
>   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 dab901c9ad..7d69420967 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -81,6 +81,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;
> @@ -353,17 +364,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 8607e0ac36..a223b69e24 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -73,6 +73,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);
> @@ -265,31 +290,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;

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


ATB,

Mark.

[PATCH 3/6] 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 | 20 +++++++++------
 2 files changed, 57 insertions(+), 30 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 7d69420967..d15117a7c7 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"
@@ -280,7 +281,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);
 
@@ -305,7 +306,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;
@@ -329,11 +329,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 = {
@@ -342,11 +359,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 = {
@@ -368,27 +407,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 a223b69e24..134d23aea7 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -204,6 +204,8 @@ static const MemoryRegionOps piix4_rcr_ops = {
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    PCIDevice *pci;
+    PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
     qemu_irq *i8259_out_irq;
 
@@ -242,6 +244,15 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
     s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
+
+    /* IDE */
+    pci = pci_create_simple(pci_bus, dev->devfn + 1, "piix4-ide");
+    pci_ide_create_devs(pci);
+
+    /* USB */
+    pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
+
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 }
 
 static void piix4_init(Object *obj)
@@ -292,7 +303,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);
@@ -300,22 +310,16 @@ 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) {
         *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
                                qdev_get_gpio_in_named(dev, "isa", 9),
                                NULL, 0, NULL);
     }
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
-
     return dev;
 }
-- 
2.36.1

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

[Mark Cave-Ayland]

On 13/05/2022 18:54, 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 | 20 +++++++++------
>   2 files changed, 57 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index 7d69420967..d15117a7c7 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"
> @@ -280,7 +281,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);
>   
> @@ -305,7 +306,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;
> @@ -329,11 +329,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);
> +};
> +

Oooof. So the reason this looks a bit odd is because we don't have an equivalent of 
device_class_set_parent_realize() for PCIDevice realize(). Having had a look in pci.c 
this looks like a similar approach for handling errors, execpt that here errp is 
accessed directly.

I think this is probably the best we can do for now though. Have you tried forcing 
pci_piix3_realize() to throw an error during testing to make sure this works as expected?

>   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 = {
> @@ -342,11 +359,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 = {
> @@ -368,27 +407,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 a223b69e24..134d23aea7 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -204,6 +204,8 @@ static const MemoryRegionOps piix4_rcr_ops = {
>   static void piix4_realize(PCIDevice *dev, Error **errp)
>   {
>       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> +    PCIDevice *pci;
> +    PCIBus *pci_bus = pci_get_bus(dev);
>       ISABus *isa_bus;
>       qemu_irq *i8259_out_irq;
>   
> @@ -242,6 +244,15 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>           return;
>       }
>       s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
> +
> +    /* IDE */
> +    pci = pci_create_simple(pci_bus, dev->devfn + 1, "piix4-ide");
> +    pci_ide_create_devs(pci);
> +
> +    /* USB */
> +    pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
> +
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>   }
>   
>   static void piix4_init(Object *obj)
> @@ -292,7 +303,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);
> @@ -300,22 +310,16 @@ 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) {
>           *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>                                  qdev_get_gpio_in_named(dev, "isa", 9),
>                                  NULL, 0, NULL);
>       }
>   
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
> -
>       return dev;
>   }

As long as the error handling works as required:

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


ATB,

Mark.

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

[Bernhard Beschow]

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

On Sat, May 21, 2022 at 10:27 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 13/05/2022 18:54, 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 | 20 +++++++++------
> >   2 files changed, 57 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> > index 7d69420967..d15117a7c7 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"
> > @@ -280,7 +281,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);
> >
> > @@ -305,7 +306,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;
> > @@ -329,11 +329,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);
> > +};
> > +
>
> Oooof. So the reason this looks a bit odd is because we don't have an
> equivalent of
> device_class_set_parent_realize() for PCIDevice realize(). Having had a
> look in pci.c
> this looks like a similar approach for handling errors, execpt that here
> errp is
> accessed directly.
>

Yes, I was surprised by this as well. So I took inspiration from
sdhci_common_realize().

>
> I think this is probably the best we can do for now though. Have you tried
> forcing
> pci_piix3_realize() to throw an error during testing to make sure this
> works as expected?
>

I'll post the results in the cover letter of v2.

>
> >   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 = {
> > @@ -342,11 +359,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 = {
> > @@ -368,27 +407,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 a223b69e24..134d23aea7 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -204,6 +204,8 @@ static const MemoryRegionOps piix4_rcr_ops = {
> >   static void piix4_realize(PCIDevice *dev, Error **errp)
> >   {
> >       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> > +    PCIDevice *pci;
> > +    PCIBus *pci_bus = pci_get_bus(dev);
> >       ISABus *isa_bus;
> >       qemu_irq *i8259_out_irq;
> >
> > @@ -242,6 +244,15 @@ static void piix4_realize(PCIDevice *dev, Error
> **errp)
> >           return;
> >       }
> >       s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
> > +
> > +    /* IDE */
> > +    pci = pci_create_simple(pci_bus, dev->devfn + 1, "piix4-ide");
> > +    pci_ide_create_devs(pci);
> > +
> > +    /* USB */
> > +    pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
> > +
> > +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
> PIIX_NUM_PIRQS);
> >   }
> >
> >   static void piix4_init(Object *obj)
> > @@ -292,7 +303,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);
> > @@ -300,22 +310,16 @@ 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) {
> >           *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> >                                  qdev_get_gpio_in_named(dev, "isa", 9),
> >                                  NULL, 0, NULL);
> >       }
> >
> > -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
> PIIX_NUM_PIRQS);
> > -
> >       return dev;
> >   }
>
> As long as the error handling works as required:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>
> ATB,
>
> Mark.
>

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

[PATCH 4/6] 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>
---
 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 f843dd906f..47932448fd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -206,9 +206,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 d15117a7c7..6eacb22dd0 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -403,7 +403,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;
@@ -412,7 +412,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 134d23aea7..4968c69da9 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -301,7 +301,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;
@@ -311,10 +311,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) {
         *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
                                qdev_get_gpio_in_named(dev, "isa", 9),
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 a304fc6041..b768109f30 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -72,8 +72,8 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
 
 #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

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

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> Modernizes the code and even saves a few lines.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   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 f843dd906f..47932448fd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -206,9 +206,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 d15117a7c7..6eacb22dd0 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -403,7 +403,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;
> @@ -412,7 +412,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 134d23aea7..4968c69da9 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -301,7 +301,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;
> @@ -311,10 +311,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) {
>           *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>                                  qdev_get_gpio_in_named(dev, "isa", 9),
> 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 a304fc6041..b768109f30 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -72,8 +72,8 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>   
>   #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

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


ATB,

Mark.

[PATCH 5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

[Bernhard Beschow]

Initialize the SM bus just like is done for piix3 which modernizes the
code.

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

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 4968c69da9..852e5c4db1 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -301,21 +301,12 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
+PCIDevice *piix4_create(PCIBus *pci_bus)
 {
     PCIDevice *pci;
-    DeviceState *dev;
-    int devfn = PCI_DEVFN(10, 0);
 
-    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
                                           TYPE_PIIX4_PCI_DEVICE);
-    dev = DEVICE(pci);
 
-    if (smbus) {
-        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
-                               qdev_get_gpio_in_named(dev, "isa", 9),
-                               NULL, 0, NULL);
-    }
-
-    return dev;
+    return pci;
 }
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index e446b25ad0..d4bd3549d0 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,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &smbus);
+    piix4 = piix4_create(pci_bus);
+    dev = DEVICE(piix4);
     isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
+                          qdev_get_gpio_in_named(dev, "isa", 9),
+                          NULL, 0, NULL);
 
     /* 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 b768109f30..bea3b44551 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
 
 PIIX3State *piix3_create(PCIBus *pci_bus);
 
-DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
+PCIDevice *piix4_create(PCIBus *pci_bus);
 
 #endif
-- 
2.36.1

Re: [PATCH 5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> Initialize the SM bus just like is done for piix3 which modernizes the
> code.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c                | 15 +++------------
>   hw/mips/malta.c               |  7 ++++++-
>   include/hw/southbridge/piix.h |  2 +-
>   3 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 4968c69da9..852e5c4db1 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -301,21 +301,12 @@ static void piix4_register_types(void)
>   
>   type_init(piix4_register_types)
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> +PCIDevice *piix4_create(PCIBus *pci_bus)
>   {
>       PCIDevice *pci;
> -    DeviceState *dev;
> -    int devfn = PCI_DEVFN(10, 0);
>   
> -    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
> +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
>                                             TYPE_PIIX4_PCI_DEVICE);
> -    dev = DEVICE(pci);
>   
> -    if (smbus) {
> -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> -                               qdev_get_gpio_in_named(dev, "isa", 9),
> -                               NULL, 0, NULL);
> -    }
> -
> -    return dev;
> +    return pci;
>   }

I don't think it makes sense to return PCIDevice here: when returning a QOM object 
from a function, the general expectation is that for a device you would return a 
DeviceState since then it can natively be used by the qdev API. So please keep the 
original return type above.

> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index e446b25ad0..d4bd3549d0 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,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
>       empty_slot_init("GT64120", 0, 0x20000000);
>   
>       /* Southbridge */
> -    dev = piix4_create(pci_bus, &smbus);
> +    piix4 = piix4_create(pci_bus);
> +    dev = DEVICE(piix4);
>       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> +    smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> +                          qdev_get_gpio_in_named(dev, "isa", 9),
> +                          NULL, 0, NULL);

... then here you can do either "piix4 = PCI_DEVICE(dev)" or perhaps even inline it 
directly as PCI_DEVICE(dev)->devfn if it isn't used elsewhere.

>       /* 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 b768109f30..bea3b44551 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>   
>   PIIX3State *piix3_create(PCIBus *pci_bus);
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> +PCIDevice *piix4_create(PCIBus *pci_bus);
>   
>   #endif


ATB,

Mark.

Re: [PATCH 5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

[Bernhard Beschow]

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

On Sat, May 21, 2022 at 10:39 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 13/05/2022 18:54, Bernhard Beschow wrote:
>
> > Initialize the SM bus just like is done for piix3 which modernizes the
> > code.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> >   hw/isa/piix4.c                | 15 +++------------
> >   hw/mips/malta.c               |  7 ++++++-
> >   include/hw/southbridge/piix.h |  2 +-
> >   3 files changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 4968c69da9..852e5c4db1 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -301,21 +301,12 @@ static void piix4_register_types(void)
> >
> >   type_init(piix4_register_types)
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> > +PCIDevice *piix4_create(PCIBus *pci_bus)
> >   {
> >       PCIDevice *pci;
> > -    DeviceState *dev;
> > -    int devfn = PCI_DEVFN(10, 0);
> >
> > -    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
> > +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
> true,
> >                                             TYPE_PIIX4_PCI_DEVICE);
> > -    dev = DEVICE(pci);
> >
> > -    if (smbus) {
> > -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> > -                               qdev_get_gpio_in_named(dev, "isa", 9),
> > -                               NULL, 0, NULL);
> > -    }
> > -
> > -    return dev;
> > +    return pci;
> >   }
>
> I don't think it makes sense to return PCIDevice here: when returning a
> QOM object
> from a function, the general expectation is that for a device you would
> return a
> DeviceState since then it can natively be used by the qdev API. So please
> keep the
> original return type above.
>

Okay, will do.

I've been toying with moving piix4_pm_init() into piix4_realize(), such
that it is created as part of TYPE_PIIX4_PCI_DEVICE - just as the real
hardware. I think I like this solution much better.

>
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index e446b25ad0..d4bd3549d0 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,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
> >       empty_slot_init("GT64120", 0, 0x20000000);
> >
> >       /* Southbridge */
> > -    dev = piix4_create(pci_bus, &smbus);
> > +    piix4 = piix4_create(pci_bus);
> > +    dev = DEVICE(piix4);
> >       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> > +    smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> > +                          qdev_get_gpio_in_named(dev, "isa", 9),
> > +                          NULL, 0, NULL);
>
> ... then here you can do either "piix4 = PCI_DEVICE(dev)" or perhaps even
> inline it
> directly as PCI_DEVICE(dev)->devfn if it isn't used elsewhere.
>

When instantiating the pm in TYPE_PIIX4_PCI_DEVICE this problem just
disappears magically. So I'd roll with this in v2.

>
> >       /* 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 b768109f30..bea3b44551 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> >
> >   PIIX3State *piix3_create(PCIBus *pci_bus);
> >
> > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> > +PCIDevice *piix4_create(PCIBus *pci_bus);
> >
> >   #endif
>
>
> ATB,
>
> Mark.
>

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

Re: [PATCH 5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

[Mark Cave-Ayland]

On 22/05/2022 10:21, Bernhard Beschow wrote:

> On Sat, May 21, 2022 at 10:39 AM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
> <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
> 
>     On 13/05/2022 18:54, Bernhard Beschow wrote:
> 
>      > Initialize the SM bus just like is done for piix3 which modernizes the
>      > code.
>      >
>      > Signed-off-by: Bernhard Beschow <shentey@gmail.com <mailto:shentey@gmail.com>>
>      > ---
>      >   hw/isa/piix4.c                | 15 +++------------
>      >   hw/mips/malta.c               |  7 ++++++-
>      >   include/hw/southbridge/piix.h |  2 +-
>      >   3 files changed, 10 insertions(+), 14 deletions(-)
>      >
>      > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>      > index 4968c69da9..852e5c4db1 100644
>      > --- a/hw/isa/piix4.c
>      > +++ b/hw/isa/piix4.c
>      > @@ -301,21 +301,12 @@ static void piix4_register_types(void)
>      >
>      >   type_init(piix4_register_types)
>      >
>      > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>      > +PCIDevice *piix4_create(PCIBus *pci_bus)
>      >   {
>      >       PCIDevice *pci;
>      > -    DeviceState *dev;
>      > -    int devfn = PCI_DEVFN(10, 0);
>      >
>      > -    pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
>      > +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
>      >                                             TYPE_PIIX4_PCI_DEVICE);
>      > -    dev = DEVICE(pci);
>      >
>      > -    if (smbus) {
>      > -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
>      > -                               qdev_get_gpio_in_named(dev, "isa", 9),
>      > -                               NULL, 0, NULL);
>      > -    }
>      > -
>      > -    return dev;
>      > +    return pci;
>      >   }
> 
>     I don't think it makes sense to return PCIDevice here: when returning a QOM object
>     from a function, the general expectation is that for a device you would return a
>     DeviceState since then it can natively be used by the qdev API. So please keep the
>     original return type above.
> 
> 
> Okay, will do.
> 
> I've been toying with moving piix4_pm_init() into piix4_realize(), such that it is 
> created as part of TYPE_PIIX4_PCI_DEVICE - just as the real hardware. I think I like 
> this solution much better.
> 
> 
>      > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>      > index e446b25ad0..d4bd3549d0 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,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
>      >       empty_slot_init("GT64120", 0, 0x20000000);
>      >
>      >       /* Southbridge */
>      > -    dev = piix4_create(pci_bus, &smbus);
>      > +    piix4 = piix4_create(pci_bus);
>      > +    dev = DEVICE(piix4);
>      >       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>      > +    smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
>      > +                          qdev_get_gpio_in_named(dev, "isa", 9),
>      > +                          NULL, 0, NULL);
> 
>     ... then here you can do either "piix4 = PCI_DEVICE(dev)" or perhaps even inline it
>     directly as PCI_DEVICE(dev)->devfn if it isn't used elsewhere.
> 
> 
> When instantiating the pm in TYPE_PIIX4_PCI_DEVICE this problem just disappears 
> magically. So I'd roll with this in v2.

(goes and looks)

Yes, that makes complete sense to me since it mirrors exactly how this is already 
done with the integrated IDE and USB devices. At some point I can see that 
piix4_pm_init() should also disappear but let's go one step at a time :)

>      >       /* 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 b768109f30..bea3b44551 100644
>      > --- a/include/hw/southbridge/piix.h
>      > +++ b/include/hw/southbridge/piix.h
>      > @@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>      >
>      >   PIIX3State *piix3_create(PCIBus *pci_bus);
>      >
>      > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
>      > +PCIDevice *piix4_create(PCIBus *pci_bus);
>      >
>      >   #endif

ATB,

Mark.

[PATCH 6/6] 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>
---
 hw/i386/pc_piix.c             |  6 +++++-
 hw/isa/piix3.c                | 16 ----------------
 hw/isa/piix4.c                | 10 ----------
 hw/mips/malta.c               |  3 ++-
 include/hw/southbridge/piix.h |  6 ++----
 5 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 47932448fd..82c7941958 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -196,6 +196,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,
@@ -206,7 +209,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 6eacb22dd0..01c376b39a 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -36,9 +36,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],
@@ -402,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 852e5c4db1..a70063bc77 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -300,13 +300,3 @@ static void piix4_register_types(void)
 }
 
 type_init(piix4_register_types)
-
-PCIDevice *piix4_create(PCIBus *pci_bus)
-{
-    PCIDevice *pci;
-
-    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
-                                          TYPE_PIIX4_PCI_DEVICE);
-
-    return pci;
-}
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index d4bd3549d0..57b5eddc74 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1400,7 +1400,8 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    piix4 = 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 = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index bea3b44551..2d55dbdef7 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -70,10 +70,8 @@ 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);
-
-PCIDevice *piix4_create(PCIBus *pci_bus);
-
 #endif
-- 
2.36.1

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

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> 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>
> ---
>   hw/i386/pc_piix.c             |  6 +++++-
>   hw/isa/piix3.c                | 16 ----------------
>   hw/isa/piix4.c                | 10 ----------
>   hw/mips/malta.c               |  3 ++-
>   include/hw/southbridge/piix.h |  6 ++----
>   5 files changed, 9 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 47932448fd..82c7941958 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -196,6 +196,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,
> @@ -206,7 +209,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 6eacb22dd0..01c376b39a 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -36,9 +36,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],
> @@ -402,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 852e5c4db1..a70063bc77 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -300,13 +300,3 @@ static void piix4_register_types(void)
>   }
>   
>   type_init(piix4_register_types)
> -
> -PCIDevice *piix4_create(PCIBus *pci_bus)
> -{
> -    PCIDevice *pci;
> -
> -    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
> -                                          TYPE_PIIX4_PCI_DEVICE);
> -
> -    return pci;
> -}
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index d4bd3549d0..57b5eddc74 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1400,7 +1400,8 @@ void mips_malta_init(MachineState *machine)
>       empty_slot_init("GT64120", 0, 0x20000000);
>   
>       /* Southbridge */
> -    piix4 = 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 = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index bea3b44551..2d55dbdef7 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,10 +70,8 @@ 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"

I think it would make sense for the movement of these types to be included in patch 1 
in a single place.

> -PIIX3State *piix3_create(PCIBus *pci_bus);
> -
> -PCIDevice *piix4_create(PCIBus *pci_bus);
> -
>   #endif

Otherwise:

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


ATB,

Mark.

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

[Bernhard Beschow]

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

On Sat, May 21, 2022 at 10:43 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 13/05/2022 18:54, Bernhard Beschow wrote:
>
> > 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>
> > ---
> >   hw/i386/pc_piix.c             |  6 +++++-
> >   hw/isa/piix3.c                | 16 ----------------
> >   hw/isa/piix4.c                | 10 ----------
> >   hw/mips/malta.c               |  3 ++-
> >   include/hw/southbridge/piix.h |  6 ++----
> >   5 files changed, 9 insertions(+), 32 deletions(-)
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 47932448fd..82c7941958 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -196,6 +196,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,
> > @@ -206,7 +209,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 6eacb22dd0..01c376b39a 100644
> > --- a/hw/isa/piix3.c
> > +++ b/hw/isa/piix3.c
> > @@ -36,9 +36,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],
> > @@ -402,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 852e5c4db1..a70063bc77 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -300,13 +300,3 @@ static void piix4_register_types(void)
> >   }
> >
> >   type_init(piix4_register_types)
> > -
> > -PCIDevice *piix4_create(PCIBus *pci_bus)
> > -{
> > -    PCIDevice *pci;
> > -
> > -    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
> true,
> > -                                          TYPE_PIIX4_PCI_DEVICE);
> > -
> > -    return pci;
> > -}
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index d4bd3549d0..57b5eddc74 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1400,7 +1400,8 @@ void mips_malta_init(MachineState *machine)
> >       empty_slot_init("GT64120", 0, 0x20000000);
> >
> >       /* Southbridge */
> > -    piix4 = 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 = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> > diff --git a/include/hw/southbridge/piix.h
> b/include/hw/southbridge/piix.h
> > index bea3b44551..2d55dbdef7 100644
> > --- a/include/hw/southbridge/piix.h
> > +++ b/include/hw/southbridge/piix.h
> > @@ -70,10 +70,8 @@ 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"
>
> I think it would make sense for the movement of these types to be included
> in patch 1
> in a single place.
>

This makes sense indeed. I'll change it.

>
> > -PIIX3State *piix3_create(PCIBus *pci_bus);
> > -
> > -PCIDevice *piix4_create(PCIBus *pci_bus);
> > -
> >   #endif
>
> Otherwise:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>
> ATB,
>
> Mark.
>

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

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

[Mark Cave-Ayland]

On 13/05/2022 18:54, Bernhard Beschow wrote:

> 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.
> 
> Bernhard Beschow (6):
>    include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
>    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: Factor out SM bus initialization from create() function
>    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                | 91 +++++++++++++-------------------
>   hw/mips/malta.c               |  9 +++-
>   include/hw/isa/isa.h          |  2 -
>   include/hw/southbridge/piix.h |  6 +--
>   6 files changed, 105 insertions(+), 108 deletions(-)

I've just reviewed these, and other than a couple of minor issues these look good to 
me and definitely help to improve the code.

One thing reading over this patches has made me realise is that there is quite a 
model violation here in that the PIIX3 and PIIX4 devices (which are the PCI-ISA 
bridges) are actually setting the PCI host bridge IRQs(!). What should happen is that 
the PCI bus IRQs and routing should be done in the PCI host bridge, and gpios used in 
the PCI-ISA bridges to pass them up. But that's definitely something outside the 
scope of these improvements.


ATB,

Mark.

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

[Philippe Mathieu-Daudé via]

On 21/5/22 10:48, Mark Cave-Ayland wrote:
> On 13/05/2022 18:54, Bernhard Beschow wrote:

>> Bernhard Beschow (6):
>>    include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
>>    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: Factor out SM bus initialization from create() function
>>    hw/isa/piix{3,4}: Inline and remove create() functions

> I've just reviewed these, and other than a couple of minor issues these 
> look good to me and definitely help to improve the code.
> 
> One thing reading over this patches has made me realise is that there is 
> quite a model violation here in that the PIIX3 and PIIX4 devices (which 
> are the PCI-ISA bridges) are actually setting the PCI host bridge 
> IRQs(!). What should happen is that the PCI bus IRQs and routing should 
> be done in the PCI host bridge, and gpios used in the PCI-ISA bridges to 
> pass them up. But that's definitely something outside the scope of these 
> improvements.

Yes. Do you mind opening a GitLab issue?