[PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Bernhard Beschow]

A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
qemu_irqs aren't initialized at that time yet. This series provides a fix by
initializing the qemu_irq of the respective south bridges before they
are passed to i2859_init().

Furthermore -- as an optional extension -- this series also fixes some usability
issues in the API for creating multifunction PCI devices.

The series is structured as follows: The first three commits fix the
regressions, the last two fix the public API for creating multifunction PCI
devices.

[1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/

Bernhard Beschow (5):
  hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
  hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
  hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
  hw/pci/pci: Remove multifunction parameter from
    pci_create_simple_multifunction()
  hw/pci/pci: Remove multifunction parameter from
    pci_new_multifunction()

 include/hw/pci/pci.h |  4 +---
 hw/alpha/dp264.c     |  8 +++++---
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     | 10 +++++-----
 hw/isa/vt82c686.c    |  3 ++-
 hw/mips/boston.c     |  3 +--
 hw/mips/fuloong2e.c  |  9 +++++----
 hw/mips/malta.c      |  2 +-
 hw/pci-host/sabre.c  |  6 ++----
 hw/pci/pci.c         | 18 ++++++++++++------
 hw/ppc/pegasos2.c    |  9 +++++----
 hw/ppc/prep.c        |  4 +++-
 hw/sparc64/sun4u.c   |  5 ++---
 13 files changed, 45 insertions(+), 38 deletions(-)

-- 
2.39.2

[PATCH 1/5] hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt

[Bernhard Beschow]

Commit bb98e0f59cde ("hw/isa/vt82c686: Remove intermediate IRQ forwarder")
passes s->cpu_intr to i8259_init() in via_isa_realize() directly. However,
s->cpu_intr isn't initialized yet since that happens after the south
bridge's pci_realize_and_unref() in board code. Fix this by initializing s-
>cpu_intr before realizing the south bridge.

Fixes: bb98e0f59cde ("hw/isa/vt82c686: Remove intermediate IRQ forwarder")
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   |  3 ++-
 hw/mips/fuloong2e.c |  9 +++++----
 hw/ppc/pegasos2.c   | 10 ++++++----
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f4c40965cd..8900d87f59 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -578,6 +578,8 @@ static void via_isa_init(Object *obj)
     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
+
+    qdev_init_gpio_out(DEVICE(obj), &s->cpu_intr, 1);
 }
 
 static const TypeInfo via_isa_info = {
@@ -606,7 +608,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     ISABus *isa_bus;
     int i;
 
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index cfc8ca6ae4..30944f8fe7 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -295,14 +295,15 @@ static void mips_fuloong2e_init(MachineState *machine)
     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
 
     /* South bridge -> IP5 */
-    pci_dev = pci_create_simple_multifunction(pci_bus,
-                                              PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
-                                              true, TYPE_VT82C686B_ISA);
+    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), true,
+                                    TYPE_VT82C686B_ISA);
+    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
+    pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
+
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(OBJECT(pci_dev),
                                                             "rtc"),
                               "date");
-    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
 
     dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
     pci_ide_create_devs(PCI_DEVICE(dev));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 7cc375df05..b0ada9c963 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -159,13 +159,15 @@ static void pegasos2_init(MachineState *machine)
     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
 
     /* VIA VT8231 South Bridge (multifunction PCI device) */
-    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
-                                                 true, TYPE_VT8231_ISA));
+    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
+                                       TYPE_VT8231_ISA));
+    qdev_connect_gpio_out(DEVICE(via), 0,
+                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
+    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
+
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(via, "rtc"),
                               "date");
-    qdev_connect_gpio_out(DEVICE(via), 0,
-                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
     pci_ide_create_devs(dev);
-- 
2.39.2

Re: [PATCH 1/5] hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt

[BALATON Zoltan]

On Sat, 4 Mar 2023, Bernhard Beschow wrote:
> Commit bb98e0f59cde ("hw/isa/vt82c686: Remove intermediate IRQ forwarder")
> passes s->cpu_intr to i8259_init() in via_isa_realize() directly. However,
> s->cpu_intr isn't initialized yet since that happens after the south
> bridge's pci_realize_and_unref() in board code. Fix this by initializing s-
>> cpu_intr before realizing the south bridge.
>
> Fixes: bb98e0f59cde ("hw/isa/vt82c686: Remove intermediate IRQ forwarder")
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c   |  3 ++-
> hw/mips/fuloong2e.c |  9 +++++----
> hw/ppc/pegasos2.c   | 10 ++++++----
> 3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f4c40965cd..8900d87f59 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj)
>     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
>     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> +
> +    qdev_init_gpio_out(DEVICE(obj), &s->cpu_intr, 1);
> }
>
> static const TypeInfo via_isa_info = {
> @@ -606,7 +608,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     ISABus *isa_bus;
>     int i;
>
> -    qdev_init_gpio_out(dev, &s->cpu_intr, 1);

I'm not a fan of useless asserts but in this case it's not useless and 
you'd need an assert here to make sure board code already connected the 
intertupt if that's required for this to work.

Regards,
BALATON Zoltan

>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
>
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index cfc8ca6ae4..30944f8fe7 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -295,14 +295,15 @@ static void mips_fuloong2e_init(MachineState *machine)
>     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
>
>     /* South bridge -> IP5 */
> -    pci_dev = pci_create_simple_multifunction(pci_bus,
> -                                              PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> -                                              true, TYPE_VT82C686B_ISA);
> +    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), true,
> +                                    TYPE_VT82C686B_ISA);
> +    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
> +    pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
> +
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(OBJECT(pci_dev),
>                                                             "rtc"),
>                               "date");
> -    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
>
>     dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>     pci_ide_create_devs(PCI_DEVICE(dev));
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 7cc375df05..b0ada9c963 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -159,13 +159,15 @@ static void pegasos2_init(MachineState *machine)
>     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
>
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
> -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
> -                                                 true, TYPE_VT8231_ISA));
> +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
> +                                       TYPE_VT8231_ISA));
> +    qdev_connect_gpio_out(DEVICE(via), 0,
> +                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
> +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
> +
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(via, "rtc"),
>                               "date");
> -    qdev_connect_gpio_out(DEVICE(via), 0,
> -                          qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
>     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
>     pci_ide_create_devs(dev);
>

[PATCH 2/5] hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt

[Bernhard Beschow]

Commit cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
passes s->cpu_intr to i8259_init() in i82378_realize() directly. However, s-
>cpu_intr isn't initialized yet since that happens after the south bridge's
pci_realize_and_unref() in board code. Fix this by initializing s->cpu_intr
before realizing the south bridge.

Fixes: cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/alpha/dp264.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 4161f559a7..e92295ac86 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -16,6 +16,7 @@
 #include "hw/ide/pci.h"
 #include "hw/isa/superio.h"
 #include "net/net.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/datadir.h"
 
@@ -110,11 +111,12 @@ static void clipper_init(MachineState *machine)
      * Importantly, we need to provide a PCI device node for it, otherwise
      * some operating systems won't notice there's an ISA bus to configure.
      */
-    i82378_dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(7, 0), "i82378"));
-    isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
-
+    i82378_dev = DEVICE(pci_new(PCI_DEVFN(7, 0), "i82378"));
     /* Connect the ISA PIC to the Typhoon IRQ used for ISA interrupts. */
     qdev_connect_gpio_out(i82378_dev, 0, isa_irq);
+    qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
+
+    isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
 
     /* Since we have an SRM-compatible PALcode, use the SRM epoch.  */
     mc146818_rtc_init(isa_bus, 1900, rtc_irq);
-- 
2.39.2

[PATCH 3/5] hw/ppc/prep: Fix wiring of PIC -> CPU interrupt

[Bernhard Beschow]

Commit cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
passes s->cpu_intr to i8259_init() in i82378_realize() directly. However, s-
>cpu_intr isn't initialized yet since that happens after the south bridge's
pci_realize_and_unref() in board code. Fix this by initializing s->cpu_intr
before realizing the south bridge.

Fixes: cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/prep.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index d00280c0f8..cfa47c1e44 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -270,9 +270,11 @@ static void ibm_40p_init(MachineState *machine)
     }
 
     /* PCI -> ISA bridge */
-    i82378_dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
+    i82378_dev = DEVICE(pci_new(PCI_DEVFN(11, 0), "i82378"));
     qdev_connect_gpio_out(i82378_dev, 0,
                           qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
+    qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
+
     sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
     isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
 
-- 
2.39.2

Re: [PATCH 3/5] hw/ppc/prep: Fix wiring of PIC -> CPU interrupt

[Daniel Henrique Barboza]

On 3/4/23 08:40, Bernhard Beschow wrote:
> Commit cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
> passes s->cpu_intr to i8259_init() in i82378_realize() directly. However, s-
>> cpu_intr isn't initialized yet since that happens after the south bridge's
> pci_realize_and_unref() in board code. Fix this by initializing s->cpu_intr
> before realizing the south bridge.
> 
> Fixes: cef2e7148e32 ("hw/isa/i82378: Remove intermediate IRQ forwarder")
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

And queued in ppc-next. Thanks,


Daniel



>   hw/ppc/prep.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index d00280c0f8..cfa47c1e44 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -270,9 +270,11 @@ static void ibm_40p_init(MachineState *machine)
>       }
>   
>       /* PCI -> ISA bridge */
> -    i82378_dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
> +    i82378_dev = DEVICE(pci_new(PCI_DEVFN(11, 0), "i82378"));
>       qdev_connect_gpio_out(i82378_dev, 0,
>                             qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
> +    qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
> +
>       sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
>       isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
>   

[PATCH 4/5] hw/pci/pci: Remove multifunction parameter from pci_create_simple_multifunction()

[Bernhard Beschow]

There is also pci_create_simple() which creates non-multifunction PCI
devices. Accordingly the parameter is always set to true when a multi
function PCI device is to be created.

The reason for the parameter's existence seems to be that it is used in the
internal PCI code as well which is the only location where it gets set to
false. This one usage can be replaced by trivial code.

Remove this redundant, error-prone parameter.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci/pci.h | 1 -
 hw/i386/pc_piix.c    | 2 +-
 hw/i386/pc_q35.c     | 4 ++--
 hw/mips/boston.c     | 3 +--
 hw/mips/malta.c      | 2 +-
 hw/pci/pci.c         | 7 ++++---
 6 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d5a40cd058..830407a5b9 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -578,7 +578,6 @@ PCIDevice *pci_new(int devfn, const char *name);
 bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp);
 
 PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
-                                           bool multifunction,
                                            const char *name);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2f16011bab..0cd430ccc5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -235,7 +235,7 @@ static void pc_init1(MachineState *machine,
                                        : pc_pci_slot_get_pirq);
         pcms->bus = pci_bus;
 
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+        pci_dev = pci_create_simple_multifunction(pci_bus, -1, type);
         piix3 = PIIX3_PCI_DEVICE(pci_dev);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 797ba347fd..65a862b66d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -299,7 +299,7 @@ static void pc_q35_init(MachineState *machine)
         ahci = pci_create_simple_multifunction(host_bus,
                                                PCI_DEVFN(ICH9_SATA1_DEV,
                                                          ICH9_SATA1_FUNC),
-                                               true, "ich9-ahci");
+                                               "ich9-ahci");
         idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
         g_assert(MAX_SATA_PORTS == ahci_get_num_ports(ahci));
@@ -321,7 +321,7 @@ static void pc_q35_init(MachineState *machine)
         smb = pci_create_simple_multifunction(host_bus,
                                               PCI_DEVFN(ICH9_SMB_DEV,
                                                         ICH9_SMB_FUNC),
-                                              true, TYPE_ICH9_SMB_DEVICE);
+                                              TYPE_ICH9_SMB_DEVICE);
         pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(smb), "i2c"));
 
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index a9d87f3437..2539606549 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -770,8 +770,7 @@ static void boston_mach_init(MachineState *machine)
                              boston_lcd_event, NULL, s, NULL, true);
 
     ahci = pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
-                                           PCI_DEVFN(0, 0),
-                                           true, TYPE_ICH9_AHCI);
+                                           PCI_DEVFN(0, 0), TYPE_ICH9_AHCI);
     g_assert(ARRAY_SIZE(hd) == ahci_get_num_ports(ahci));
     ide_drive_get(hd, ahci_get_num_ports(ahci));
     ahci_ide_create_devs(ahci, hd);
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ec172b111a..d4c79d263d 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1254,7 +1254,7 @@ void mips_malta_init(MachineState *machine)
     pci_bus_map_irqs(pci_bus, malta_pci_slot_get_pirq);
 
     /* Southbridge */
-    piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
+    piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN,
                                             TYPE_PIIX4_PCI_DEVICE);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 034fe49e9a..c2e14f000e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2111,17 +2111,18 @@ bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp)
 }
 
 PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
-                                           bool multifunction,
                                            const char *name)
 {
-    PCIDevice *dev = pci_new_multifunction(devfn, multifunction, name);
+    PCIDevice *dev = pci_new_multifunction(devfn, true, name);
     pci_realize_and_unref(dev, bus, &error_fatal);
     return dev;
 }
 
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 {
-    return pci_create_simple_multifunction(bus, devfn, false, name);
+    PCIDevice *dev = pci_new(devfn, name);
+    pci_realize_and_unref(dev, bus, &error_fatal);
+    return dev;
 }
 
 static uint8_t pci_find_space(PCIDevice *pdev, uint8_t size)
-- 
2.39.2

[PATCH 5/5] hw/pci/pci: Remove multifunction parameter from pci_new_multifunction()

[Bernhard Beschow]

There is also pci_new() which creates non-multifunction PCI devices.
Accordingly the parameter is always set to true when a multi function PCI
device is to be created.

The reason for the parameter's existence seems to be that it is used in the
internal PCI code as well which is the only location where it gets set to
false. This one usage can be resolved by factoring out an internal helper
function.

Remove this redundant, error-prone parameter.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci/pci.h |  3 +--
 hw/i386/pc_q35.c     |  6 +++---
 hw/mips/fuloong2e.c  |  2 +-
 hw/pci-host/sabre.c  |  6 ++----
 hw/pci/pci.c         | 13 +++++++++----
 hw/ppc/pegasos2.c    |  3 +--
 hw/sparc64/sun4u.c   |  5 ++---
 7 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 830407a5b9..cbf3ebea4e 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -572,8 +572,7 @@ pci_set_quad_by_mask(uint8_t *config, uint64_t mask, uint64_t reg)
     pci_set_quad(config, (~mask & val) | (mask & rval));
 }
 
-PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
-                                    const char *name);
+PCIDevice *pci_new_multifunction(int devfn, const char *name);
 PCIDevice *pci_new(int devfn, const char *name);
 bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 65a862b66d..b41fc2b879 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -99,12 +99,12 @@ static int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
         return -1;
     }
 
-    ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), true, name);
+    ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), name);
     pci_realize_and_unref(ehci, bus, &error_fatal);
     usbbus = QLIST_FIRST(&ehci->qdev.child_bus);
 
     for (i = 0; i < 3; i++) {
-        uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func), true,
+        uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func),
                                      comp[i].name);
         qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name);
         qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port);
@@ -236,7 +236,7 @@ static void pc_q35_init(MachineState *machine)
     phb = PCI_HOST_BRIDGE(q35_host);
     host_bus = phb->bus;
     /* create ISA bus */
-    lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true,
+    lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
                                 TYPE_ICH9_LPC_DEVICE);
     qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
                       x86_machine_is_smm_enabled(x86ms));
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 30944f8fe7..0d4a45bd4f 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -295,7 +295,7 @@ static void mips_fuloong2e_init(MachineState *machine)
     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
 
     /* South bridge -> IP5 */
-    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0), true,
+    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
                                     TYPE_VT82C686B_ISA);
     qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
     pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 949ecc21f2..dcb2e230b6 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -387,14 +387,12 @@ static void sabre_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
 
     /* APB secondary busses */
-    pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
-                                    TYPE_SIMBA_PCI_BRIDGE);
+    pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_SIMBA_PCI_BRIDGE);
     s->bridgeB = PCI_BRIDGE(pci_dev);
     pci_bridge_map_irq(s->bridgeB, "pciB", pci_simbaB_map_irq);
     pci_realize_and_unref(pci_dev, phb->bus, &error_fatal);
 
-    pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), true,
-                                    TYPE_SIMBA_PCI_BRIDGE);
+    pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), TYPE_SIMBA_PCI_BRIDGE);
     s->bridgeA = PCI_BRIDGE(pci_dev);
     pci_bridge_map_irq(s->bridgeA, "pciA", pci_simbaA_map_irq);
     pci_realize_and_unref(pci_dev, phb->bus, &error_fatal);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c2e14f000e..09907c966e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2089,8 +2089,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     pci_dev->msi_trigger = pci_msi_trigger;
 }
 
-PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
-                                 const char *name)
+static PCIDevice *pci_new_internal(int devfn, bool multifunction,
+                                   const char *name)
 {
     DeviceState *dev;
 
@@ -2100,9 +2100,14 @@ PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
     return PCI_DEVICE(dev);
 }
 
+PCIDevice *pci_new_multifunction(int devfn, const char *name)
+{
+    return pci_new_internal(devfn, true, name);
+}
+
 PCIDevice *pci_new(int devfn, const char *name)
 {
-    return pci_new_multifunction(devfn, false, name);
+    return pci_new_internal(devfn, false, name);
 }
 
 bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp)
@@ -2113,7 +2118,7 @@ bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp)
 PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
                                            const char *name)
 {
-    PCIDevice *dev = pci_new_multifunction(devfn, true, name);
+    PCIDevice *dev = pci_new_multifunction(devfn, name);
     pci_realize_and_unref(dev, bus, &error_fatal);
     return dev;
 }
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index b0ada9c963..b8c8a60804 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -159,8 +159,7 @@ static void pegasos2_init(MachineState *machine)
     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
 
     /* VIA VT8231 South Bridge (multifunction PCI device) */
-    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
-                                       TYPE_VT8231_ISA));
+    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
     qdev_connect_gpio_out(DEVICE(via), 0,
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
     pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index a25e951f9d..b5d5a82c50 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -612,7 +612,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     pci_busA->slot_reserved_mask = 0xfffffff1;
     pci_busB->slot_reserved_mask = 0xfffffff0;
 
-    ebus = pci_new_multifunction(PCI_DEVFN(1, 0), true, TYPE_EBUS);
+    ebus = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_EBUS);
     qdev_prop_set_uint64(DEVICE(ebus), "console-serial-base",
                          hwdef->console_serial_base);
     pci_realize_and_unref(ebus, pci_busA, &error_fatal);
@@ -648,8 +648,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
 
         if (!nd->model || strcmp(nd->model, "sunhme") == 0) {
             if (!onboard_nic) {
-                pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1),
-                                                   true, "sunhme");
+                pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), "sunhme");
                 bus = pci_busA;
                 memcpy(&macaddr, &nd->macaddr.a, sizeof(MACAddr));
                 onboard_nic = true;
-- 
2.39.2

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Bernhard Beschow]

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

On Sat, Mar 4, 2023 at 12:40 PM Bernhard Beschow <shentey@gmail.com> wrote:

> A recent series [1] attempted to remove some PIC -> CPU interrupt
> indirections.
> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because
> the
> qemu_irqs aren't initialized at that time yet. This series provides a fix
> by
> initializing the qemu_irq of the respective south bridges before they
> are passed to i2859_init().
>
> Furthermore -- as an optional extension -- this series also fixes some
> usability
> issues in the API for creating multifunction PCI devices.
>
> The series is structured as follows: The first three commits fix the
> regressions, the last two fix the public API for creating multifunction PCI
> devices.
>

Testing done:
* `make check`
* `make check-avocado`
* Start MorphOS ISO

>
> [1]
> https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
>
> Bernhard Beschow (5):
>   hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>   hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>   hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>   hw/pci/pci: Remove multifunction parameter from
>     pci_create_simple_multifunction()
>   hw/pci/pci: Remove multifunction parameter from
>     pci_new_multifunction()
>
>  include/hw/pci/pci.h |  4 +---
>  hw/alpha/dp264.c     |  8 +++++---
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 10 +++++-----
>  hw/isa/vt82c686.c    |  3 ++-
>  hw/mips/boston.c     |  3 +--
>  hw/mips/fuloong2e.c  |  9 +++++----
>  hw/mips/malta.c      |  2 +-
>  hw/pci-host/sabre.c  |  6 ++----
>  hw/pci/pci.c         | 18 ++++++++++++------
>  hw/ppc/pegasos2.c    |  9 +++++----
>  hw/ppc/prep.c        |  4 +++-
>  hw/sparc64/sun4u.c   |  5 ++---
>  13 files changed, 45 insertions(+), 38 deletions(-)
>
> --
> 2.39.2
>
>

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

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[BALATON Zoltan]

On Sat, 4 Mar 2023, Bernhard Beschow wrote:
> A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
> qemu_irqs aren't initialized at that time yet. This series provides a fix by
> initializing the qemu_irq of the respective south bridges before they
> are passed to i2859_init().
>
> Furthermore -- as an optional extension -- this series also fixes some usability
> issues in the API for creating multifunction PCI devices.
>
> The series is structured as follows: The first three commits fix the
> regressions, the last two fix the public API for creating multifunction PCI
> devices.
>
> [1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
>
> Bernhard Beschow (5):
>  hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>  hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>  hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>  hw/pci/pci: Remove multifunction parameter from
>    pci_create_simple_multifunction()
>  hw/pci/pci: Remove multifunction parameter from
>    pci_new_multifunction()

I'd postopne the last two API change patches to the next release. Ideally 
the device itself should know if it's multifunction or not and the board 
instantiating it should not do anything different than instantiating a 
single function device so we's only need pci_new or pci_create_simple 
without multifunction parameter or variant. So my question is why do we 
need these at all and could this be simplified more? But there's not 
enough time to answer that now so I'd ask to leave these alone for now and 
come back to this in next devel cycle.

The other 3 patches fix a breakaga in current master so can be considered 
but I'd need to know a decision if this will be taken or a revert as I 
need to rebase my pending patches accordingly. A maintainer please speak 
up here.

Regards,
BALATON Zoltan

> include/hw/pci/pci.h |  4 +---
> hw/alpha/dp264.c     |  8 +++++---
> hw/i386/pc_piix.c    |  2 +-
> hw/i386/pc_q35.c     | 10 +++++-----
> hw/isa/vt82c686.c    |  3 ++-
> hw/mips/boston.c     |  3 +--
> hw/mips/fuloong2e.c  |  9 +++++----
> hw/mips/malta.c      |  2 +-
> hw/pci-host/sabre.c  |  6 ++----
> hw/pci/pci.c         | 18 ++++++++++++------
> hw/ppc/pegasos2.c    |  9 +++++----
> hw/ppc/prep.c        |  4 +++-
> hw/sparc64/sun4u.c   |  5 ++---
> 13 files changed, 45 insertions(+), 38 deletions(-)
>
> --
> 2.39.2
>
>
>

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Peter Maydell]

On Sat, 4 Mar 2023 at 13:30, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 4 Mar 2023, Bernhard Beschow wrote:
> > A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
> > This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
> > qemu_irqs aren't initialized at that time yet. This series provides a fix by
> > initializing the qemu_irq of the respective south bridges before they
> > are passed to i2859_init().
> >
> > Furthermore -- as an optional extension -- this series also fixes some usability
> > issues in the API for creating multifunction PCI devices.
> >
> > The series is structured as follows: The first three commits fix the
> > regressions, the last two fix the public API for creating multifunction PCI
> > devices.
> >
> > [1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
> >
> > Bernhard Beschow (5):
> >  hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
> >  hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
> >  hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
> >  hw/pci/pci: Remove multifunction parameter from
> >    pci_create_simple_multifunction()
> >  hw/pci/pci: Remove multifunction parameter from
> >    pci_new_multifunction()
>
> I'd postopne the last two API change patches to the next release. Ideally
> the device itself should know if it's multifunction or not and the board
> instantiating it should not do anything different than instantiating a
> single function device so we's only need pci_new or pci_create_simple
> without multifunction parameter or variant. So my question is why do we
> need these at all and could this be simplified more? But there's not
> enough time to answer that now so I'd ask to leave these alone for now and
> come back to this in next devel cycle.
>
> The other 3 patches fix a breakaga in current master so can be considered
> but I'd need to know a decision if this will be taken or a revert as I
> need to rebase my pending patches accordingly. A maintainer please speak
> up here.

If we're happy that patches 1-3 fix the regressions and look OK
code-wise then applying them is probably the simplest thing.

thanks
-- PMM

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Michael S. Tsirkin]

On Sat, Mar 04, 2023 at 12:40:38PM +0100, Bernhard Beschow wrote:
> A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
> qemu_irqs aren't initialized at that time yet. This series provides a fix by
> initializing the qemu_irq of the respective south bridges before they
> are passed to i2859_init().
> 
> Furthermore -- as an optional extension -- this series also fixes some usability
> issues in the API for creating multifunction PCI devices.
> 
> The series is structured as follows: The first three commits fix the
> regressions, the last two fix the public API for creating multifunction PCI
> devices.

"We pulled your tooth oh and we removed your appendix too while we were
at it".
Pls don't do this kind of thing unless strictly necessary.


> [1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
> 
> Bernhard Beschow (5):
>   hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>   hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>   hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>   hw/pci/pci: Remove multifunction parameter from
>     pci_create_simple_multifunction()
>   hw/pci/pci: Remove multifunction parameter from
>     pci_new_multifunction()
> 
>  include/hw/pci/pci.h |  4 +---
>  hw/alpha/dp264.c     |  8 +++++---
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 10 +++++-----
>  hw/isa/vt82c686.c    |  3 ++-
>  hw/mips/boston.c     |  3 +--
>  hw/mips/fuloong2e.c  |  9 +++++----
>  hw/mips/malta.c      |  2 +-
>  hw/pci-host/sabre.c  |  6 ++----
>  hw/pci/pci.c         | 18 ++++++++++++------
>  hw/ppc/pegasos2.c    |  9 +++++----
>  hw/ppc/prep.c        |  4 +++-
>  hw/sparc64/sun4u.c   |  5 ++---
>  13 files changed, 45 insertions(+), 38 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Mark Cave-Ayland]

On 04/03/2023 11:40, Bernhard Beschow wrote:

> A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
> qemu_irqs aren't initialized at that time yet. This series provides a fix by
> initializing the qemu_irq of the respective south bridges before they
> are passed to i2859_init().
> 
> Furthermore -- as an optional extension -- this series also fixes some usability
> issues in the API for creating multifunction PCI devices.
> 
> The series is structured as follows: The first three commits fix the
> regressions, the last two fix the public API for creating multifunction PCI
> devices.
> 
> [1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
> 
> Bernhard Beschow (5):
>    hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>    hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>    hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>    hw/pci/pci: Remove multifunction parameter from
>      pci_create_simple_multifunction()
>    hw/pci/pci: Remove multifunction parameter from
>      pci_new_multifunction()
> 
>   include/hw/pci/pci.h |  4 +---
>   hw/alpha/dp264.c     |  8 +++++---
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     | 10 +++++-----
>   hw/isa/vt82c686.c    |  3 ++-
>   hw/mips/boston.c     |  3 +--
>   hw/mips/fuloong2e.c  |  9 +++++----
>   hw/mips/malta.c      |  2 +-
>   hw/pci-host/sabre.c  |  6 ++----
>   hw/pci/pci.c         | 18 ++++++++++++------
>   hw/ppc/pegasos2.c    |  9 +++++----
>   hw/ppc/prep.c        |  4 +++-
>   hw/sparc64/sun4u.c   |  5 ++---
>   13 files changed, 45 insertions(+), 38 deletions(-)

Thanks for doing this! The patches basically look good, the only minor niggle is that 
normally wiring of gpios is done *after* realize() for consistency because some 
qdev_init_gpio_*() functions may use a property to define the gpio array size.

Having said that it is a nice tidy-up, so I'd be okay with patches 1-3 if you added a 
small comment above the qdev_connect_gpio_out() lines pointing out that this is a 
temporary solution (hack?) until the 8259 device is converted to use gpios.

However given that Phil wrote the patches I'd wait for him to decide whether he'd 
prefer a plain revert over the changes in this series before going ahead with a v2.

Finally I think patches 4-5 are good, however given how close we are to freeze I'd 
drop them for now and present them as a separate series at the start of the 8.1 dev 
cycle when everyone has a bit more breathing space.


ATB,

Mark.

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Philippe Mathieu-Daudé]

Hi Bernhard, Mark,

On 7/3/23 00:59, Mark Cave-Ayland wrote:
> On 04/03/2023 11:40, Bernhard Beschow wrote:
> 
>> A recent series [1] attempted to remove some PIC -> CPU interrupt 
>> indirections.
>> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 
>> because the
>> qemu_irqs aren't initialized at that time yet. This series provides a 
>> fix by
>> initializing the qemu_irq of the respective south bridges before they
>> are passed to i2859_init().
>>
>> Furthermore -- as an optional extension -- this series also fixes some 
>> usability
>> issues in the API for creating multifunction PCI devices.
>>
>> The series is structured as follows: The first three commits fix the
>> regressions, the last two fix the public API for creating 
>> multifunction PCI
>> devices.
>>
>> [1] 
>> https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/
>>
>> Bernhard Beschow (5):
>>    hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>>    hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>>    hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>>    hw/pci/pci: Remove multifunction parameter from
>>      pci_create_simple_multifunction()
>>    hw/pci/pci: Remove multifunction parameter from
>>      pci_new_multifunction()
>>
>>   include/hw/pci/pci.h |  4 +---
>>   hw/alpha/dp264.c     |  8 +++++---
>>   hw/i386/pc_piix.c    |  2 +-
>>   hw/i386/pc_q35.c     | 10 +++++-----
>>   hw/isa/vt82c686.c    |  3 ++-
>>   hw/mips/boston.c     |  3 +--
>>   hw/mips/fuloong2e.c  |  9 +++++----
>>   hw/mips/malta.c      |  2 +-
>>   hw/pci-host/sabre.c  |  6 ++----
>>   hw/pci/pci.c         | 18 ++++++++++++------
>>   hw/ppc/pegasos2.c    |  9 +++++----
>>   hw/ppc/prep.c        |  4 +++-
>>   hw/sparc64/sun4u.c   |  5 ++---
>>   13 files changed, 45 insertions(+), 38 deletions(-)
> 
> Thanks for doing this! The patches basically look good, the only minor 
> niggle is that normally wiring of gpios is done *after* realize() for 
> consistency because some qdev_init_gpio_*() functions may use a property 
> to define the gpio array size.

Sorry this took me so long. The series LGTM too, but I wanted to well
understand the overall problem and run more tests.
Bernhard noticed that the bug is that we access the qdev gpios _before_
the device is realized.

The (undocumented) sysbus_connect_irq() API -- which calls
qdev_connect_gpio_out() -- is expected to be called _after_
DeviceRealize.

Bernhard's fix is to call qdev_connect_gpio_out() _before_
DeviceRealize.

> Having said that it is a nice tidy-up, so I'd be okay with patches 1-3 
> if you added a small comment above the qdev_connect_gpio_out() lines 
> pointing out that this is a temporary solution (hack?) until the 8259 
> device is converted to use gpios.

I agree, while this works, it is a "temporary solution" until we decide
and clarify the QDev/SysBus APIs w.r.t. IRQs.

> However given that Phil wrote the patches I'd wait for him to decide 
> whether he'd prefer a plain revert over the changes in this series 
> before going ahead with a v2.

As discussed with Peter / Mark / David on IRC, a revert is wiser for
this release.

Re: [PATCH 0/5] Fix recent PIC -> CPU interrupt wiring regressions

[Michael S. Tsirkin]

On Sat, Mar 04, 2023 at 12:40:38PM +0100, Bernhard Beschow wrote:
> A recent series [1] attempted to remove some PIC -> CPU interrupt indirections.
> This inadvertantly caused NULL qemu_irqs to be passed to the i8259 because the
> qemu_irqs aren't initialized at that time yet. This series provides a fix by
> initializing the qemu_irq of the respective south bridges before they
> are passed to i2859_init().
> 
> Furthermore -- as an optional extension -- this series also fixes some usability
> issues in the API for creating multifunction PCI devices.
> 
> The series is structured as follows: The first three commits fix the
> regressions, the last two fix the public API for creating multifunction PCI
> devices.
> 
> [1] https://lore.kernel.org/qemu-devel/20230302224058.43315-1-philmd@linaro.org/


Well philmd merged that one so I'll let him untangle it.
However please separate fixes and cleanups.
Cleanups can't go in now, fixes still can.
Thanks!


> Bernhard Beschow (5):
>   hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt
>   hw/alpha/dp264: Fix wiring of PIC -> CPU interrupt
>   hw/ppc/prep: Fix wiring of PIC -> CPU interrupt
>   hw/pci/pci: Remove multifunction parameter from
>     pci_create_simple_multifunction()
>   hw/pci/pci: Remove multifunction parameter from
>     pci_new_multifunction()
> 
>  include/hw/pci/pci.h |  4 +---
>  hw/alpha/dp264.c     |  8 +++++---
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 10 +++++-----
>  hw/isa/vt82c686.c    |  3 ++-
>  hw/mips/boston.c     |  3 +--
>  hw/mips/fuloong2e.c  |  9 +++++----
>  hw/mips/malta.c      |  2 +-
>  hw/pci-host/sabre.c  |  6 ++----
>  hw/pci/pci.c         | 18 ++++++++++++------
>  hw/ppc/pegasos2.c    |  9 +++++----
>  hw/ppc/prep.c        |  4 +++-
>  hw/sparc64/sun4u.c   |  5 ++---
>  13 files changed, 45 insertions(+), 38 deletions(-)
> 
> -- 
> 2.39.2
>