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

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

[Bernhard Beschow]

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.

Bernhard Beschow (6):
  include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
  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                | 97 +++++++++++++++-------------------
 hw/mips/malta.c               |  7 ++-
 include/hw/isa/isa.h          |  2 -
 include/hw/southbridge/piix.h |  6 +--
 6 files changed, 109 insertions(+), 108 deletions(-)

-- 
2.36.1

[PATCH v2 1/6] 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>
---
 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 f63f83e5c6..4d17400dfd 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -70,6 +70,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

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

[BALATON Zoltan]

On Sun, 22 May 2022, 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>
> ---
> 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 f63f83e5c6..4d17400dfd 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,6 +70,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"

This naming seems to go back to 9b74b190d6c so it's not directly related 
to this series but there seems to be a bit of a confusion here that I 
wonder could be cleaned up now. We have:

#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
and
#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"

and both of these have mismatched #define and device name. These are not 
user creatable so so the device names don't appear anywhere (except maybe 
in info qtree output) but this could still be confusing as normally type 
defines should match device names. If these are the ISA bridges then maybe 
piix?-isa is a good name so maybe we should have

#define TYPE_PIIX3_ISA "piix3-isa"
#defint TYPE_PIIX4_ISA "piix4-isa"

(then also matching e.g. "via-isa") but I'm not sure changing "pci-piix3" 
to "piix3-isa" could break anything (I don't expect changing the defines 
themselces could cause any problem).

It's just something I've noticed and brought up for consideration but I 
have no strong opinion on it so up to you what you do with it.

Regards,
BALATON Zoltan

> +
> PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>
> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
>

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

[Bernhard Beschow]

Am 22. Mai 2022 22:32:06 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 22 May 2022, 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>
>> ---
>> 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 f63f83e5c6..4d17400dfd 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -70,6 +70,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"
>
>This naming seems to go back to 9b74b190d6c so it's not directly related to this series but there seems to be a bit of a confusion here that I wonder could be cleaned up now. We have:
>
>#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
>and
>#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>
>and both of these have mismatched #define and device name. These are not user creatable so so the device names don't appear anywhere (except maybe in info qtree output) but this could still be confusing as normally type defines should match device names. If these are the ISA bridges then maybe piix?-isa is a good name so maybe we should have
>
>#define TYPE_PIIX3_ISA "piix3-isa"
>#defint TYPE_PIIX4_ISA "piix4-isa"
>
>(then also matching e.g. "via-isa") but I'm not sure changing "pci-piix3" to "piix3-isa" could break anything (I don't expect changing the defines themselces could cause any problem).
>
>It's just something I've noticed and brought up for consideration but I have no strong opinion on it so up to you what you do with it.

Hi Zoltan,

yeah, I noticed the naming when moving the defines. I couldn't come up
with better names which were worth the effort because I can imagine
where the names are coming from. I also didn't want to go down the
rabbithole of backwards compatibility (which is a topic I haven't
covered yet). I think that *-isa is too narrow a name for the
container device since it bridges a couple of buses to PCI, but other
than that I don't have strong opinions about the names.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> +
>> PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>>
>> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
>>

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

[Mark Cave-Ayland]

On 29/05/2022 10:23, Bernhard Beschow wrote:

> Am 22. Mai 2022 22:32:06 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 22 May 2022, 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>
>>> ---
>>> 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 f63f83e5c6..4d17400dfd 100644
>>> --- a/include/hw/southbridge/piix.h
>>> +++ b/include/hw/southbridge/piix.h
>>> @@ -70,6 +70,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"
>>
>> This naming seems to go back to 9b74b190d6c so it's not directly related to this series but there seems to be a bit of a confusion here that I wonder could be cleaned up now. We have:
>>
>> #define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
>> and
>> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>>
>> and both of these have mismatched #define and device name. These are not user creatable so so the device names don't appear anywhere (except maybe in info qtree output) but this could still be confusing as normally type defines should match device names. If these are the ISA bridges then maybe piix?-isa is a good name so maybe we should have
>>
>> #define TYPE_PIIX3_ISA "piix3-isa"
>> #defint TYPE_PIIX4_ISA "piix4-isa"
>>
>> (then also matching e.g. "via-isa") but I'm not sure changing "pci-piix3" to "piix3-isa" could break anything (I don't expect changing the defines themselces could cause any problem).
>>
>> It's just something I've noticed and brought up for consideration but I have no strong opinion on it so up to you what you do with it.
> 
> Hi Zoltan,
> 
> yeah, I noticed the naming when moving the defines. I couldn't come up
> with better names which were worth the effort because I can imagine
> where the names are coming from. I also didn't want to go down the
> rabbithole of backwards compatibility (which is a topic I haven't
> covered yet). I think that *-isa is too narrow a name for the
> container device since it bridges a couple of buses to PCI, but other
> than that I don't have strong opinions about the names.

Agreed. They certainly aren't the best of names, but my worry would be that someone 
has done some magic with -global to override the defaults and so renaming them would 
cause something to break.

Certainly this could be discussed as a separate topic with the PC machine maintainers 
if someone feels strongly about it, but I wouldn't want this to hold up the patch 
series unnecessarily.


ATB,

Mark.

[PATCH v2 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 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

[PATCH v2 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 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 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

[PATCH v2 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 578e537b35..4d4b83a27d 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 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 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 4d17400dfd..0bec7f8ca3 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -74,8 +74,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 v2 5/6] 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                | 14 +++++++-------
 hw/mips/malta.c               |  3 ++-
 include/hw/southbridge/piix.h |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 4968c69da9..1645f63450 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -206,6 +206,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
     PCIDevice *pci;
     PCIBus *pci_bus = pci_get_bus(dev);
+    I2CBus *smbus;
     ISABus *isa_bus;
     qemu_irq *i8259_out_irq;
 
@@ -252,6 +253,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
     /* USB */
     pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
 
+    /* ACPI controller */
+    smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100, s->isa[9],
+                          NULL, 0, NULL);
+    object_property_add_const_link(OBJECT(s), "smbus", OBJECT(smbus));
+
     pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 }
 
@@ -301,7 +307,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;
@@ -311,11 +317,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
                                           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;
 }
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 0bec7f8ca3..2c21359efa 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -76,6 +76,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

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

[Mark Cave-Ayland]

On 22/05/2022 22:24, 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                | 14 +++++++-------
>   hw/mips/malta.c               |  3 ++-
>   include/hw/southbridge/piix.h |  2 +-
>   3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 4968c69da9..1645f63450 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -206,6 +206,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
>       PCIDevice *pci;
>       PCIBus *pci_bus = pci_get_bus(dev);
> +    I2CBus *smbus;
>       ISABus *isa_bus;
>       qemu_irq *i8259_out_irq;
>   
> @@ -252,6 +253,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>       /* USB */
>       pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
>   
> +    /* ACPI controller */
> +    smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100, s->isa[9],
> +                          NULL, 0, NULL);
> +    object_property_add_const_link(OBJECT(s), "smbus", OBJECT(smbus));
> +

Interesting hack here to expose the smbus so it is available to qdev_get_child_bus(), 
but really this is still really working around the fact that piix4_pm_init() itself 
should be removed first. Once that is done, you can then use a standard QOM pattern 
to initialise the "internal" PCI devices via object_initialize_child() and realize 
them in piix4_realize() instead of using pci_create_simple().

Is that something you could take a look at? If not, I may be able to put something 
together towards the end of the week. Other than that I think the rest of the series 
looks good.

>       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>   }
>   
> @@ -301,7 +307,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;
> @@ -311,11 +317,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>                                             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;
>   }
> 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 0bec7f8ca3..2c21359efa 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -76,6 +76,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 v2 5/6] hw/isa/piix4: QOM'ify PIIX4 PM creation

[Mark Cave-Ayland]

On 25/05/2022 19:09, Mark Cave-Ayland wrote:

> On 22/05/2022 22:24, 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                | 14 +++++++-------
>>   hw/mips/malta.c               |  3 ++-
>>   include/hw/southbridge/piix.h |  2 +-
>>   3 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 4968c69da9..1645f63450 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -206,6 +206,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>>       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
>>       PCIDevice *pci;
>>       PCIBus *pci_bus = pci_get_bus(dev);
>> +    I2CBus *smbus;
>>       ISABus *isa_bus;
>>       qemu_irq *i8259_out_irq;
>> @@ -252,6 +253,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>>       /* USB */
>>       pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
>> +    /* ACPI controller */
>> +    smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100, s->isa[9],
>> +                          NULL, 0, NULL);
>> +    object_property_add_const_link(OBJECT(s), "smbus", OBJECT(smbus));
>> +
> 
> Interesting hack here to expose the smbus so it is available to qdev_get_child_bus(), 
> but really this is still really working around the fact that piix4_pm_init() itself 
> should be removed first. Once that is done, you can then use a standard QOM pattern 
> to initialise the "internal" PCI devices via object_initialize_child() and realize 
> them in piix4_realize() instead of using pci_create_simple().
> 
> Is that something you could take a look at? If not, I may be able to put something 
> together towards the end of the week. Other than that I think the rest of the series 
> looks good.

Hi Bernhard,

I've just sent through the series for eliminating the piix4_pm_init() which should 
allow you to improve the QOMifcation done as part of this series.

A bit of background as to why this is necessary: when building a container device 
such as the PIIX southbridge, there should still be 2 distinct init and realize 
phases. In effect this needs to be done depth-first so when you init the PIIX4 
southbridge, the instance init function should also init the contained PCI devices 
such as IDE and USB. Similarly when the PIIX4 device is realized, its realize 
function should then realize the contained PCI devices using qdev_realize().

This is one of the main reasons why legacy global device init functions aren't 
recommended, since they both init *and* realize the device before returning it which 
immediately breaks this contract.

The normal way to initialise a contained device is to use object_initialize_child() 
in the container's instance init function and to store the the child device instance 
within the container. But what this also means is that you shouldn't use any _new() 
functions like pci_new() or pci_create_simple() to instantiated contained devices in 
a container realize function either. So the next question: how is this done?

Fortunately there is an existing example of this: take a look at how this is 
currently done in hw/pci-host/q35.c's q35_host_initfn() and q35_host_realize() for 
the MCH_PCI_DEVICE device.

I hope this gives you all the information you need to produce a v3 of the series: if 
you have any further questions, do let me know and I will do my best to answer them.

>>       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>>   }
>> @@ -301,7 +307,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;
>> @@ -311,11 +317,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>>                                             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;
>>   }
>> 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 0bec7f8ca3..2c21359efa 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -76,6 +76,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.

[PATCH v2 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                | 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 4d4b83a27d..989e101ed6 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 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 1645f63450..aabbda3e91 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -306,16 +306,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 2c21359efa..2d55dbdef7 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -74,8 +74,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 v2 0/6] QOM'ify PIIX southbridge creation

[Philippe Mathieu-Daudé via]

On 22/5/22 23:24, Bernhard Beschow wrote:

> Bernhard Beschow (6):
>    include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names
>    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

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