[PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[Bernhard Beschow]

This series first turns vt82c686's "INTR" pin into a named GPIO for better
comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
and piix4 by connecting out IRQs of the south bridges before they get realized.
This approach is already used in the pc machines after it had been discussed at
KVM forum `23.

Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
south bridge wants to trigger. If, as an implementation detail, the south bridge
wants to pass this callback to a child device, such as the PIC, then this
callback must be known to the south bridge before it gets realized. Otherwise
board code had to wire the PIC device itself, breaking encapsulation. This means
that qdev_connect_gpio_out*() has to be called before realize() which this
series implements. Another way to look at it is that callbacks apparently are
resouces such as memory regions which are also populated before realize().

Please check if above paragraph makes sense.

Best regards,
Bernhard

See also:
* https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
com/
* "Remove intermediate IRQ forwarder" patches in
https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/

Testing done:
* Boot amigaone machine into Linux
* Boot pegasos2 machine into MorphOS
* Start fuloong2e machine and check that it doesn't abort
* Boot malta machine with https://people.debian.org/~gio/dqib/

Bernhard Beschow (3):
  hw/isa/vt82c686: Turn "intr" irq into a named gpio
  hw/isa/vt82c686: Resolve intermediate IRQ forwarder
  hw/isa/piix: Resolve intermediate IRQ forwarder

 hw/isa/piix.c       | 13 ++-----------
 hw/isa/vt82c686.c   | 12 ++----------
 hw/mips/fuloong2e.c |  2 +-
 hw/mips/malta.c     |  4 +---
 hw/ppc/amigaone.c   |  8 ++++----
 hw/ppc/pegasos2.c   |  4 ++--
 6 files changed, 12 insertions(+), 31 deletions(-)

-- 
2.45.2

[PATCH 1/3] hw/isa/vt82c686: Turn "intr" irq into a named gpio

[Bernhard Beschow]

Makes the code more comprehensible, matches the datasheet and the piix4 device
model.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 2 +-
 hw/mips/fuloong2e.c | 2 +-
 hw/ppc/amigaone.c   | 4 ++--
 hw/ppc/pegasos2.c   | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8582ac0322..505b44c4e6 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -719,7 +719,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     ISABus *isa_bus;
     int i;
 
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index a45aac368c..6e4303ba47 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -299,7 +299,7 @@ static void mips_fuloong2e_init(MachineState *machine)
                               object_resolve_path_component(OBJECT(pci_dev),
                                                             "rtc"),
                               "date");
-    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
+    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 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/amigaone.c b/hw/ppc/amigaone.c
index ddfa09457a..9dcc486c1a 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -153,8 +153,8 @@ static void amigaone_init(MachineState *machine)
     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(DEVICE(cpu), PPC6xx_INPUT_INT));
+    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
+                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
     for (i = 0; i < PCI_NUM_PINS; i++) {
         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
                                                              "pirq", i));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index c1bd8dfa21..9b0a6b70ab 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -195,8 +195,8 @@ static void pegasos2_init(MachineState *machine)
     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));
+    qdev_connect_gpio_out_named(DEVICE(via), "intr", 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.45.2

[PATCH 2/3] hw/isa/vt82c686: Resolve intermediate IRQ forwarder

[Bernhard Beschow]

When @cpu_intr is populated before vt82xx's realize(), it can be directly passed
to i8259_init(), avoiding the need for the intermediate
via_isa_request_i8259_irq() handler. The result is less code and runtime
overhead, and a fixed memory leak caused by qemu_allocate_irqs().

Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 12 ++----------
 hw/mips/fuloong2e.c |  2 +-
 hw/ppc/amigaone.c   |  8 ++++----
 hw/ppc/pegasos2.c   |  4 ++--
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 505b44c4e6..ca02ad4c20 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -624,6 +624,7 @@ 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_named(DEVICE(obj), &s->cpu_intr, "intr", 1);
 }
 
 static const TypeInfo via_isa_info = {
@@ -704,24 +705,15 @@ static void via_isa_pirq(void *opaque, int pin, int level)
     via_isa_set_irq(opaque, pin, level);
 }
 
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-    ViaISAState *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static void via_isa_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
     PCIBus *pci_bus = pci_get_bus(d);
-    qemu_irq *isa_irq;
     ISABus *isa_bus;
     int i;
 
-    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
@@ -729,7 +721,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
         return;
     }
 
-    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
+    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(OBJECT(d), isa_bus, 0);
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 6e4303ba47..e6487c34d8 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -286,6 +286,7 @@ static void mips_fuloong2e_init(MachineState *machine)
     /* South bridge -> IP5 */
     pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
                                     TYPE_VT82C686B_ISA);
+    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
 
     /* Set properties on individual devices before realizing the south bridge */
     if (machine->audiodev) {
@@ -299,7 +300,6 @@ static void mips_fuloong2e_init(MachineState *machine)
                               object_resolve_path_component(OBJECT(pci_dev),
                                                             "rtc"),
                               "date");
-    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 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/amigaone.c b/hw/ppc/amigaone.c
index 9dcc486c1a..2110875f56 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -148,13 +148,13 @@ static void amigaone_init(MachineState *machine)
     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
 
     /* VIA VT82c686B South Bridge (multifunction PCI device) */
-    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
-                                                 TYPE_VT82C686B_ISA));
+    via = OBJECT(pci_new_multifunction(PCI_DEVFN(7, 0), TYPE_VT82C686B_ISA));
+    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
+                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
+    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(via, "rtc"),
                               "date");
-    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
-                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
     for (i = 0; i < PCI_NUM_PINS; i++) {
         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
                                                              "pirq", i));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 9b0a6b70ab..54e60082ce 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -181,6 +181,8 @@ static void pegasos2_init(MachineState *machine)
 
     /* VIA VT8231 South Bridge (multifunction PCI device) */
     via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
+    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
+                                qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
     /* Set properties on individual devices before realizing the south bridge */
     if (machine->audiodev) {
@@ -195,8 +197,6 @@ static void pegasos2_init(MachineState *machine)
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(via, "rtc"),
                               "date");
-    qdev_connect_gpio_out_named(DEVICE(via), "intr", 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.45.2

[PATCH 3/3] hw/isa/piix: Resolve intermediate IRQ forwarder

[Bernhard Beschow]

When @cpu_intr is populated before pixx4's realize(), it can be directly passed
to i8259_init(), avoiding the need for the intermediate piix_request_i8259_irq()
handler. The result is less code and runtime overhead, and a fixed memory leak
caused by qemu_allocate_irqs().

Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix.c   | 13 ++-----------
 hw/mips/malta.c |  4 +---
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 2d30711b17..e070628f25 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -81,12 +81,6 @@ static void piix_set_pci_irq(void *opaque, int pirq, int level)
     piix_set_pci_irq_level(s, pirq, level);
 }
 
-static void piix_request_i8259_irq(void *opaque, int irq, int level)
-{
-    PIIXState *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static PCIINTxRoute piix_route_intx_pin_to_irq(void *opaque, int pin)
 {
     PCIDevice *pci_dev = opaque;
@@ -315,9 +309,7 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
 
     /* PIC */
     if (d->has_pic) {
-        qemu_irq *i8259_out_irq = qemu_allocate_irqs(piix_request_i8259_irq, d,
-                                                     1);
-        qemu_irq *i8259 = i8259_init(isa_bus, *i8259_out_irq);
+        qemu_irq *i8259 = i8259_init(isa_bus, d->cpu_intr);
         size_t i;
 
         for (i = 0; i < ISA_NUM_IRQS; i++) {
@@ -325,8 +317,6 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
         }
 
         g_free(i8259);
-
-        qdev_init_gpio_out_named(DEVICE(dev), &d->cpu_intr, "intr", 1);
     }
 
     isa_bus_register_input_irqs(isa_bus, d->isa_irqs_in);
@@ -402,6 +392,7 @@ static void pci_piix_init(Object *obj)
 {
     PIIXState *d = PIIX_PCI_DEVICE(obj);
 
+    qdev_init_gpio_out_named(DEVICE(obj), &d->cpu_intr, "intr", 1);
     qdev_init_gpio_out_named(DEVICE(obj), d->isa_irqs_in, "isa-irqs",
                              ISA_NUM_IRQS);
 
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 664a2ae0a9..50823bd5fb 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1238,15 +1238,13 @@ void mips_malta_init(MachineState *machine)
     /* Southbridge */
     piix4 = pci_new_multifunction(PIIX4_PCI_DEVFN, TYPE_PIIX4_PCI_DEVICE);
     qdev_prop_set_uint32(DEVICE(piix4), "smb_io_base", 0x1100);
+    qdev_connect_gpio_out_named(DEVICE(piix4), "intr", 0, i8259_irq);
     pci_realize_and_unref(piix4, pci_bus, &error_fatal);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
 
     dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "ide"));
     pci_ide_create_devs(PCI_DEVICE(dev));
 
-    /* Interrupt controller */
-    qdev_connect_gpio_out_named(DEVICE(piix4), "intr", 0, i8259_irq);
-
     /* generate SPD EEPROM data */
     dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "pm"));
     smbus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
-- 
2.45.2

Re: [PATCH 1/3] hw/isa/vt82c686: Turn "intr" irq into a named gpio

[BALATON Zoltan]

On Thu, 4 Jul 2024, Bernhard Beschow wrote:
> Makes the code more comprehensible, matches the datasheet and the piix4 device
> model.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c   | 2 +-
> hw/mips/fuloong2e.c | 2 +-
> hw/ppc/amigaone.c   | 4 ++--
> hw/ppc/pegasos2.c   | 4 ++--
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8582ac0322..505b44c4e6 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -719,7 +719,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     ISABus *isa_bus;
>     int i;
>
> -    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> +    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
>     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index a45aac368c..6e4303ba47 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -299,7 +299,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>                               object_resolve_path_component(OBJECT(pci_dev),
>                                                             "rtc"),
>                               "date");
> -    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
> +    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);

I was wondering why we still have 0 when we have a name so checked the doc 
commant in include/hw/qdev-core.h and found that the docs in 
qdev_connect_gpio_out_named is mostly just a copy&paste of the 
qdev_connect_gpio_out and it also talks about output GPIO array but then 
says input GPIOs in that array. I've stopped reading at that point as this 
text makes little sense. Somebody who knows how this actually works might 
want to update that doc comment.

But that's unrelated to this patch so this is

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

>
>     dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>     pci_ide_create_devs(PCI_DEVICE(dev));
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index ddfa09457a..9dcc486c1a 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -153,8 +153,8 @@ static void amigaone_init(MachineState *machine)
>     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(DEVICE(cpu), PPC6xx_INPUT_INT));
> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> +                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
>     for (i = 0; i < PCI_NUM_PINS; i++) {
>         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>                                                              "pirq", i));
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index c1bd8dfa21..9b0a6b70ab 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -195,8 +195,8 @@ static void pegasos2_init(MachineState *machine)
>     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));
> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> +                                qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
>     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
>     pci_ide_create_devs(dev);
>

Re: [PATCH 2/3] hw/isa/vt82c686: Resolve intermediate IRQ forwarder

[BALATON Zoltan]

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

On Thu, 4 Jul 2024, Bernhard Beschow wrote:
> When @cpu_intr is populated before vt82xx's realize(), it can be directly passed
> to i8259_init(), avoiding the need for the intermediate
> via_isa_request_i8259_irq() handler. The result is less code and runtime
> overhead, and a fixed memory leak caused by qemu_allocate_irqs().
>
> Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c   | 12 ++----------
> hw/mips/fuloong2e.c |  2 +-
> hw/ppc/amigaone.c   |  8 ++++----
> hw/ppc/pegasos2.c   |  4 ++--
> 4 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 505b44c4e6..ca02ad4c20 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -624,6 +624,7 @@ 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_named(DEVICE(obj), &s->cpu_intr, "intr", 1);
> }
>
> static const TypeInfo via_isa_info = {
> @@ -704,24 +705,15 @@ static void via_isa_pirq(void *opaque, int pin, int level)
>     via_isa_set_irq(opaque, pin, level);
> }
>
> -static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> -{
> -    ViaISAState *s = opaque;
> -    qemu_set_irq(s->cpu_intr, level);
> -}
> -
> static void via_isa_realize(PCIDevice *d, Error **errp)
> {
>     ViaISAState *s = VIA_ISA(d);
>     DeviceState *dev = DEVICE(d);
>     PCIBus *pci_bus = pci_get_bus(d);
> -    qemu_irq *isa_irq;
>     ISABus *isa_bus;
>     int i;
>
> -    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
>     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);

I still don't like how this makes handling of out and in gpios different 
and it also prevents to create the device with 
pci_create_simple_multifunction() and needs tweaking before realize. I 
think the fix should be in i8259 and not in this device.

Regards,
BALATON Zoltan

> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
>
> @@ -729,7 +721,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>         return;
>     }
>
> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
> +    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
>     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(OBJECT(d), isa_bus, 0);
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 6e4303ba47..e6487c34d8 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -286,6 +286,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>     /* South bridge -> IP5 */
>     pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
>                                     TYPE_VT82C686B_ISA);
> +    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
>
>     /* Set properties on individual devices before realizing the south bridge */
>     if (machine->audiodev) {
> @@ -299,7 +300,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>                               object_resolve_path_component(OBJECT(pci_dev),
>                                                             "rtc"),
>                               "date");
> -    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 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/amigaone.c b/hw/ppc/amigaone.c
> index 9dcc486c1a..2110875f56 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -148,13 +148,13 @@ static void amigaone_init(MachineState *machine)
>     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>
>     /* VIA VT82c686B South Bridge (multifunction PCI device) */
> -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
> -                                                 TYPE_VT82C686B_ISA));
> +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(7, 0), TYPE_VT82C686B_ISA));
> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> +                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
> +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(via, "rtc"),
>                               "date");
> -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> -                                qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
>     for (i = 0; i < PCI_NUM_PINS; i++) {
>         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>                                                              "pirq", i));
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 9b0a6b70ab..54e60082ce 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -181,6 +181,8 @@ static void pegasos2_init(MachineState *machine)
>
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
>     via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> +                                qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
>     /* Set properties on individual devices before realizing the south bridge */
>     if (machine->audiodev) {
> @@ -195,8 +197,6 @@ static void pegasos2_init(MachineState *machine)
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(via, "rtc"),
>                               "date");
> -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> -                                qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
>     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
>     pci_ide_create_devs(dev);
>

Re: [PATCH 2/3] hw/isa/vt82c686: Resolve intermediate IRQ forwarder

[BALATON Zoltan]

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

On Fri, 5 Jul 2024, BALATON Zoltan wrote:
> On Thu, 4 Jul 2024, Bernhard Beschow wrote:
>> When @cpu_intr is populated before vt82xx's realize(), it can be directly 
>> passed
>> to i8259_init(), avoiding the need for the intermediate
>> via_isa_request_i8259_irq() handler. The result is less code and runtime
>> overhead, and a fixed memory leak caused by qemu_allocate_irqs().
>> 
>> Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c   | 12 ++----------
>> hw/mips/fuloong2e.c |  2 +-
>> hw/ppc/amigaone.c   |  8 ++++----
>> hw/ppc/pegasos2.c   |  4 ++--
>> 4 files changed, 9 insertions(+), 17 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 505b44c4e6..ca02ad4c20 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -624,6 +624,7 @@ 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_named(DEVICE(obj), &s->cpu_intr, "intr", 1);
>> }
>> 
>> static const TypeInfo via_isa_info = {
>> @@ -704,24 +705,15 @@ static void via_isa_pirq(void *opaque, int pin, int 
>> level)
>>     via_isa_set_irq(opaque, pin, level);
>> }
>> 
>> -static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>> -{
>> -    ViaISAState *s = opaque;
>> -    qemu_set_irq(s->cpu_intr, level);
>> -}
>> -
>> static void via_isa_realize(PCIDevice *d, Error **errp)
>> {
>>     ViaISAState *s = VIA_ISA(d);
>>     DeviceState *dev = DEVICE(d);
>>     PCIBus *pci_bus = pci_get_bus(d);
>> -    qemu_irq *isa_irq;
>>     ISABus *isa_bus;
>>     int i;
>> 
>> -    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
>>     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
>
> I still don't like how this makes handling of out and in gpios different and 
> it also prevents to create the device with pci_create_simple_multifunction() 
> and needs tweaking before realize. I think the fix should be in i8259 and not 
> in this device.

I mean users of this device should not need changing.

> Regards,
> BALATON Zoltan
>
>> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>     isa_bus = isa_bus_new(dev, pci_address_space(d), 
>> pci_address_space_io(d),
>>                           errp);
>> 
>> @@ -729,7 +721,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>         return;
>>     }
>> 
>> -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>> +    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
>>     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>     i8257_dma_init(OBJECT(d), isa_bus, 0);
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 6e4303ba47..e6487c34d8 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -286,6 +286,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>>     /* South bridge -> IP5 */
>>     pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
>>                                     TYPE_VT82C686B_ISA);
>> +    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
>>
>>     /* Set properties on individual devices before realizing the south 
>> bridge */
>>     if (machine->audiodev) {
>> @@ -299,7 +300,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>>                               object_resolve_path_component(OBJECT(pci_dev),
>>                                                             "rtc"),
>>                               "date");
>> -    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 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/amigaone.c b/hw/ppc/amigaone.c
>> index 9dcc486c1a..2110875f56 100644
>> --- a/hw/ppc/amigaone.c
>> +++ b/hw/ppc/amigaone.c
>> @@ -148,13 +148,13 @@ static void amigaone_init(MachineState *machine)
>>     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>
>>     /* VIA VT82c686B South Bridge (multifunction PCI device) */
>> -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
>> -                                                 TYPE_VT82C686B_ISA));
>> +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(7, 0), 
>> TYPE_VT82C686B_ISA));
>> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>> +                                qdev_get_gpio_in(DEVICE(cpu), 
>> PPC6xx_INPUT_INT));
>> +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
>>     object_property_add_alias(OBJECT(machine), "rtc-time",
>>                               object_resolve_path_component(via, "rtc"),
>>                               "date");
>> -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>> -                                qdev_get_gpio_in(DEVICE(cpu), 
>> PPC6xx_INPUT_INT));
>>     for (i = 0; i < PCI_NUM_PINS; i++) {
>>         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>>                                                              "pirq", i));
>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>> index 9b0a6b70ab..54e60082ce 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -181,6 +181,8 @@ static void pegasos2_init(MachineState *machine)
>>
>>     /* VIA VT8231 South Bridge (multifunction PCI device) */
>>     via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
>> +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>> +                                qdev_get_gpio_in_named(pm->mv, "gpp", 
>> 31));
>>
>>     /* Set properties on individual devices before realizing the south 
>> bridge */
>>     if (machine->audiodev) {
>> @@ -195,8 +197,6 @@ static void pegasos2_init(MachineState *machine)
>>     object_property_add_alias(OBJECT(machine), "rtc-time",
>>                               object_resolve_path_component(via, "rtc"),
>>                               "date");
>> -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>> -                                qdev_get_gpio_in_named(pm->mv, "gpp", 
>> 31));
>>
>>     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
>>     pci_ide_create_devs(dev);
>

Re: [PATCH 1/3] hw/isa/vt82c686: Turn "intr" irq into a named gpio

[Peter Maydell]

On Fri, 5 Jul 2024 at 01:32, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Thu, 4 Jul 2024, Bernhard Beschow wrote:
> > Makes the code more comprehensible, matches the datasheet and the piix4 device
> > model.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > hw/isa/vt82c686.c   | 2 +-
> > hw/mips/fuloong2e.c | 2 +-
> > hw/ppc/amigaone.c   | 4 ++--
> > hw/ppc/pegasos2.c   | 4 ++--
> > 4 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 8582ac0322..505b44c4e6 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -719,7 +719,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
> >     ISABus *isa_bus;
> >     int i;
> >
> > -    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> > +    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
> >     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
> >     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
> >     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
> > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> > index a45aac368c..6e4303ba47 100644
> > --- a/hw/mips/fuloong2e.c
> > +++ b/hw/mips/fuloong2e.c
> > @@ -299,7 +299,7 @@ static void mips_fuloong2e_init(MachineState *machine)
> >                               object_resolve_path_component(OBJECT(pci_dev),
> >                                                             "rtc"),
> >                               "date");
> > -    qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
> > +    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
>
> I was wondering why we still have 0 when we have a name so checked the doc
> commant in include/hw/qdev-core.h and found that the docs in
> qdev_connect_gpio_out_named is mostly just a copy&paste of the
> qdev_connect_gpio_out and it also talks about output GPIO array but then
> says input GPIOs in that array. I've stopped reading at that point as this
> text makes little sense. Somebody who knows how this actually works might
> want to update that doc comment.

Yeah, there's some copy-n-paste errors there. I'll send a patch.

The answer to "why is there both a name and a number 0" is
that named GPIOs (both input and output) are always created as
arrays of GPIOs, not single GPIOs. So you can create a named GPIO
output array like this:
   qdev_init_gpio_out_named(dev, s->fiq, "fiq", BCM2836_NCORES);
and then connect to fiq 0, fiq 1, fiq 2, and so on.
A single named output GPIO is a special case of the array-output
with only one element, so when you connect it up you still have
to say "I want element 0 of this length-1 array".

(The unnamed (anonymous) GPIOs are implemented under the hood
as "a named GPIO array where the name of the GPIO array is NULL".
So their semantics and also the documentation for the functions
is very similar to that for named GPIO arrays. But there are
also some places where I made cut-n-paste errors...)

thanks
-- PMM

Re: [PATCH 1/3] hw/isa/vt82c686: Turn "intr" irq into a named gpio

[Philippe Mathieu-Daudé]

On 4/7/24 22:58, Bernhard Beschow wrote:
> Makes the code more comprehensible, matches the datasheet and the piix4 device
> model.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/vt82c686.c   | 2 +-
>   hw/mips/fuloong2e.c | 2 +-
>   hw/ppc/amigaone.c   | 4 ++--
>   hw/ppc/pegasos2.c   | 4 ++--
>   4 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
> This series first turns vt82c686's "INTR" pin into a named GPIO for better
> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
> and piix4 by connecting out IRQs of the south bridges before they get realized.
> This approach is already used in the pc machines after it had been discussed at
> KVM forum `23.
> 
> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
> south bridge wants to trigger. If, as an implementation detail, the south bridge
> wants to pass this callback to a child device, such as the PIC, then this
> callback must be known to the south bridge before it gets realized. Otherwise
> board code had to wire the PIC device itself, breaking encapsulation. This means
> that qdev_connect_gpio_out*() has to be called before realize() which this
> series implements. Another way to look at it is that callbacks apparently are
> resouces such as memory regions which are also populated before realize().
> 
> Please check if above paragraph makes sense.
> 
> Best regards,
> Bernhard


Bernhard, do you intend to resolve Zoltan's comments on this
patchset?

Thanks!


> See also:
> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
> com/
> * "Remove intermediate IRQ forwarder" patches in
> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/
> 
> Testing done:
> * Boot amigaone machine into Linux
> * Boot pegasos2 machine into MorphOS
> * Start fuloong2e machine and check that it doesn't abort
> * Boot malta machine with https://people.debian.org/~gio/dqib/
> 
> Bernhard Beschow (3):
>   hw/isa/vt82c686: Turn "intr" irq into a named gpio
>   hw/isa/vt82c686: Resolve intermediate IRQ forwarder
>   hw/isa/piix: Resolve intermediate IRQ forwarder
> 
>  hw/isa/piix.c       | 13 ++-----------
>  hw/isa/vt82c686.c   | 12 ++----------
>  hw/mips/fuloong2e.c |  2 +-
>  hw/mips/malta.c     |  4 +---
>  hw/ppc/amigaone.c   |  8 ++++----
>  hw/ppc/pegasos2.c   |  4 ++--
>  6 files changed, 12 insertions(+), 31 deletions(-)
> 
> -- 
> 2.45.2
> 

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[BALATON Zoltan]

On Sat, 20 Jul 2024, Michael S. Tsirkin wrote:
> On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
>> This series first turns vt82c686's "INTR" pin into a named GPIO for better
>> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
>> and piix4 by connecting out IRQs of the south bridges before they get realized.
>> This approach is already used in the pc machines after it had been discussed at
>> KVM forum `23.
>>
>> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
>> south bridge wants to trigger. If, as an implementation detail, the south bridge
>> wants to pass this callback to a child device, such as the PIC, then this
>> callback must be known to the south bridge before it gets realized. Otherwise
>> board code had to wire the PIC device itself, breaking encapsulation. This means
>> that qdev_connect_gpio_out*() has to be called before realize() which this
>> series implements. Another way to look at it is that callbacks apparently are
>> resouces such as memory regions which are also populated before realize().
>>
>> Please check if above paragraph makes sense.
>>
>> Best regards,
>> Bernhard
>
>
> Bernhard, do you intend to resolve Zoltan's comments on this
> patchset?

You can take this alternative for now:

https://patchew.org/QEMU/cover.1719690591.git.balaton@eik.bme.hu/

I think solving it the right way would need to QDev-ify i8259 and make it 
use gpio out and get rid of the legacy init function but as that's an old 
piece of code used by multiple machines, it's not easily done so maybe not 
for few days before the freeze.

For now I think either embedding the IRQ in the state struct (or 
alternatively adding an unrealize or finalize or whatever is the right 
method to free it) is better than changing all users of this device.

Regards,
BALATON Zoltan

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[Bernhard Beschow]

Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
>> This series first turns vt82c686's "INTR" pin into a named GPIO for better
>> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
>> and piix4 by connecting out IRQs of the south bridges before they get realized.
>> This approach is already used in the pc machines after it had been discussed at
>> KVM forum `23.
>> 
>> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
>> south bridge wants to trigger. If, as an implementation detail, the south bridge
>> wants to pass this callback to a child device, such as the PIC, then this
>> callback must be known to the south bridge before it gets realized. Otherwise
>> board code had to wire the PIC device itself, breaking encapsulation. This means
>> that qdev_connect_gpio_out*() has to be called before realize() which this
>> series implements. Another way to look at it is that callbacks apparently are
>> resouces such as memory regions which are also populated before realize().
>> 
>> Please check if above paragraph makes sense.
>> 
>> Best regards,
>> Bernhard
>
>
>Bernhard, do you intend to resolve Zoltan's comments on this
>patchset?

Hi Michael,

I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community.

As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series.

Best regards,
Bernhard

>
>Thanks!
>
>
>> See also:
>> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
>> com/
>> * "Remove intermediate IRQ forwarder" patches in
>> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/
>> 
>> Testing done:
>> * Boot amigaone machine into Linux
>> * Boot pegasos2 machine into MorphOS
>> * Start fuloong2e machine and check that it doesn't abort
>> * Boot malta machine with https://people.debian.org/~gio/dqib/
>> 
>> Bernhard Beschow (3):
>>   hw/isa/vt82c686: Turn "intr" irq into a named gpio
>>   hw/isa/vt82c686: Resolve intermediate IRQ forwarder
>>   hw/isa/piix: Resolve intermediate IRQ forwarder
>> 
>>  hw/isa/piix.c       | 13 ++-----------
>>  hw/isa/vt82c686.c   | 12 ++----------
>>  hw/mips/fuloong2e.c |  2 +-
>>  hw/mips/malta.c     |  4 +---
>>  hw/ppc/amigaone.c   |  8 ++++----
>>  hw/ppc/pegasos2.c   |  4 ++--
>>  6 files changed, 12 insertions(+), 31 deletions(-)
>> 
>> -- 
>> 2.45.2
>> 
>

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[Michael S. Tsirkin]

On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> >On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
> >> This series first turns vt82c686's "INTR" pin into a named GPIO for better
> >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
> >> and piix4 by connecting out IRQs of the south bridges before they get realized.
> >> This approach is already used in the pc machines after it had been discussed at
> >> KVM forum `23.
> >> 
> >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
> >> south bridge wants to trigger. If, as an implementation detail, the south bridge
> >> wants to pass this callback to a child device, such as the PIC, then this
> >> callback must be known to the south bridge before it gets realized. Otherwise
> >> board code had to wire the PIC device itself, breaking encapsulation. This means
> >> that qdev_connect_gpio_out*() has to be called before realize() which this
> >> series implements. Another way to look at it is that callbacks apparently are
> >> resouces such as memory regions which are also populated before realize().
> >> 
> >> Please check if above paragraph makes sense.
> >> 
> >> Best regards,
> >> Bernhard
> >
> >
> >Bernhard, do you intend to resolve Zoltan's comments on this
> >patchset?
> 
> Hi Michael,
> 
> I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community.
> 
> As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series.
> 
> Best regards,
> Bernhard


ATM I'm just looking at whether we can fix any bugs for the coming
release.

> >
> >Thanks!
> >
> >
> >> See also:
> >> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
> >> com/
> >> * "Remove intermediate IRQ forwarder" patches in
> >> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/
> >> 
> >> Testing done:
> >> * Boot amigaone machine into Linux
> >> * Boot pegasos2 machine into MorphOS
> >> * Start fuloong2e machine and check that it doesn't abort
> >> * Boot malta machine with https://people.debian.org/~gio/dqib/
> >> 
> >> Bernhard Beschow (3):
> >>   hw/isa/vt82c686: Turn "intr" irq into a named gpio
> >>   hw/isa/vt82c686: Resolve intermediate IRQ forwarder
> >>   hw/isa/piix: Resolve intermediate IRQ forwarder
> >> 
> >>  hw/isa/piix.c       | 13 ++-----------
> >>  hw/isa/vt82c686.c   | 12 ++----------
> >>  hw/mips/fuloong2e.c |  2 +-
> >>  hw/mips/malta.c     |  4 +---
> >>  hw/ppc/amigaone.c   |  8 ++++----
> >>  hw/ppc/pegasos2.c   |  4 ++--
> >>  6 files changed, 12 insertions(+), 31 deletions(-)
> >> 
> >> -- 
> >> 2.45.2
> >> 
> >

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[Bernhard Beschow]

Am 23. Juli 2024 00:21:32 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>> >On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
>> >> This series first turns vt82c686's "INTR" pin into a named GPIO for better
>> >> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
>> >> and piix4 by connecting out IRQs of the south bridges before they get realized.
>> >> This approach is already used in the pc machines after it had been discussed at
>> >> KVM forum `23.
>> >> 
>> >> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
>> >> south bridge wants to trigger. If, as an implementation detail, the south bridge
>> >> wants to pass this callback to a child device, such as the PIC, then this
>> >> callback must be known to the south bridge before it gets realized. Otherwise
>> >> board code had to wire the PIC device itself, breaking encapsulation. This means
>> >> that qdev_connect_gpio_out*() has to be called before realize() which this
>> >> series implements. Another way to look at it is that callbacks apparently are
>> >> resouces such as memory regions which are also populated before realize().
>> >> 
>> >> Please check if above paragraph makes sense.
>> >> 
>> >> Best regards,
>> >> Bernhard
>> >
>> >
>> >Bernhard, do you intend to resolve Zoltan's comments on this
>> >patchset?
>> 
>> Hi Michael,
>> 
>> I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community.
>> 
>> As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series.
>> 
>> Best regards,
>> Bernhard
>
>
>ATM I'm just looking at whether we can fix any bugs for the coming
>release.

Yes, makes sense. Both Zoltan's and my proposal qualify for bug fixing IMO, and both are an improvement over the current code (fixing memory leaks). I'm not too attached to either proposal, but I'd prefer a third opinion on the matter. @Phil: As maintainer and one who worked on the very same issues before you look like a good candidate. Though I'm not sure if your time and focus allows for that right now.

Best regards,
Bernhard

>
>> >
>> >Thanks!
>> >
>> >
>> >> See also:
>> >> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
>> >> com/
>> >> * "Remove intermediate IRQ forwarder" patches in
>> >> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/
>> >> 
>> >> Testing done:
>> >> * Boot amigaone machine into Linux
>> >> * Boot pegasos2 machine into MorphOS
>> >> * Start fuloong2e machine and check that it doesn't abort
>> >> * Boot malta machine with https://people.debian.org/~gio/dqib/
>> >> 
>> >> Bernhard Beschow (3):
>> >>   hw/isa/vt82c686: Turn "intr" irq into a named gpio
>> >>   hw/isa/vt82c686: Resolve intermediate IRQ forwarder
>> >>   hw/isa/piix: Resolve intermediate IRQ forwarder
>> >> 
>> >>  hw/isa/piix.c       | 13 ++-----------
>> >>  hw/isa/vt82c686.c   | 12 ++----------
>> >>  hw/mips/fuloong2e.c |  2 +-
>> >>  hw/mips/malta.c     |  4 +---
>> >>  hw/ppc/amigaone.c   |  8 ++++----
>> >>  hw/ppc/pegasos2.c   |  4 ++--
>> >>  6 files changed, 12 insertions(+), 31 deletions(-)
>> >> 
>> >> -- 
>> >> 2.45.2
>> >> 
>> >
>

Re: [PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks

[BALATON Zoltan]

On Wed, 24 Jul 2024, Bernhard Beschow wrote:
> Am 23. Juli 2024 00:21:32 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>> On Mon, Jul 22, 2024 at 10:21:30PM +0000, Bernhard Beschow wrote:
>>>
>>>
>>> Am 20. Juli 2024 18:38:42 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>>>> On Thu, Jul 04, 2024 at 10:58:51PM +0200, Bernhard Beschow wrote:
>>>>> This series first turns vt82c686's "INTR" pin into a named GPIO for better
>>>>> comprehensibility. It then continues fixing qemu_irq memory leaks in vt82c686
>>>>> and piix4 by connecting out IRQs of the south bridges before they get realized.
>>>>> This approach is already used in the pc machines after it had been discussed at
>>>>> KVM forum `23.
>>>>>
>>>>> Observe that out IRQs are callbacks such as an INTR IRQ handler in a CPU which a
>>>>> south bridge wants to trigger. If, as an implementation detail, the south bridge
>>>>> wants to pass this callback to a child device, such as the PIC, then this
>>>>> callback must be known to the south bridge before it gets realized. Otherwise
>>>>> board code had to wire the PIC device itself, breaking encapsulation. This means
>>>>> that qdev_connect_gpio_out*() has to be called before realize() which this
>>>>> series implements. Another way to look at it is that callbacks apparently are
>>>>> resouces such as memory regions which are also populated before realize().
>>>>>
>>>>> Please check if above paragraph makes sense.
>>>>>
>>>>> Best regards,
>>>>> Bernhard
>>>>
>>>>
>>>> Bernhard, do you intend to resolve Zoltan's comments on this
>>>> patchset?
>>>
>>> Hi Michael,
>>>
>>> I was hoping for some more comments on the last two commits. These resolve various issues in one go, some of which have been addressed in the past. Since this is apparently a new style to handle out-GPIOs, I'd like to get an OK from the community.
>>>
>>> As Zoltan writes the ideal solution might be to qdev'ify the PIC. This is a bigger task which is more than I intended to do in this series.
>>>
>>> Best regards,
>>> Bernhard
>>
>>
>> ATM I'm just looking at whether we can fix any bugs for the coming
>> release.
>
> Yes, makes sense. Both Zoltan's and my proposal qualify for bug fixing 
> IMO, and both are an improvement over the current code (fixing memory 
> leaks). I'm not too attached to either proposal, but I'd prefer a third

Maybe there is no real bugs to fix. We don't allocate these devices other 
than creating it once in the beginning with the machine that lives 
throughout the QEMU session. So plugging memory leaks is only to silence 
some analysers that complain about this but it won't fix any real bug. 
It's still good to silence the warnings which may even lead to failed 
compilation with some compilers.

> opinion on the matter. @Phil: As maintainer and one who worked on the 
> very same issues before you look like a good candidate. Though I'm not 
> sure if your time and focus allows for that right now.

I already said that my problem with your approach is that it relies on 
confusing rules for using gpio outs and gpio ins (one would need to be 
connected before realize and the other one after). I think qemu_irq is 
already a confusing part of QEMU hiding a pointer, qemu_gpio adds another 
level of complexity to thar and adding more rules making it inconsistent 
and behave differently in hard to remember ways is too much for anybody to 
use it correctly. So instead of making this more complex for users and 
changing all the call sites, it's better to fix it at the root and allow 
users to simply use it. Currently closest to the root is within vt82c686 
as i8259 isn't yet completely QDev-ified (some very old code that is used 
by a lot of other devices like i8259 is still pre QDev using legacy init).

The qemu_gpio is a QDev thing and QDev is meant to model devices so they 
can eventually be combined into a machine without board code just using a 
declarative description. So gpios should be used for outside facing pins 
of the device and it's more intuitive to connect these pins after the 
device is realized than to connect some of them before realize and some of 
them after. Using gpio for internal connections just because it's freed 
with the device is also wrong. For that embedding qemu_irq would be the 
way that's currently used for similar cases of one device internally using 
another (like SoCs). (I have a problem with embedding devices too as that 
exposes their state struct and thus implementation details outside of 
object implementation but that's another issue. Maybe the best solution 
would be creating resources and objects as children of the device so it 
would be freed with the device but some of these can be used outside of 
the device so then those places would need to reatain/release or ref/unref 
them which we likely forget to do so it would break either way.)

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
>>
>>>>
>>>> Thanks!
>>>>
>>>>
>>>>> See also:
>>>>> * https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.
>>>>> com/
>>>>> * "Remove intermediate IRQ forwarder" patches in
>>>>> https://lore.kernel.org/qemu-devel/20230210163744.32182-1-philmd@linaro.org/
>>>>>
>>>>> Testing done:
>>>>> * Boot amigaone machine into Linux
>>>>> * Boot pegasos2 machine into MorphOS
>>>>> * Start fuloong2e machine and check that it doesn't abort
>>>>> * Boot malta machine with https://people.debian.org/~gio/dqib/
>>>>>
>>>>> Bernhard Beschow (3):
>>>>>   hw/isa/vt82c686: Turn "intr" irq into a named gpio
>>>>>   hw/isa/vt82c686: Resolve intermediate IRQ forwarder
>>>>>   hw/isa/piix: Resolve intermediate IRQ forwarder
>>>>>
>>>>>  hw/isa/piix.c       | 13 ++-----------
>>>>>  hw/isa/vt82c686.c   | 12 ++----------
>>>>>  hw/mips/fuloong2e.c |  2 +-
>>>>>  hw/mips/malta.c     |  4 +---
>>>>>  hw/ppc/amigaone.c   |  8 ++++----
>>>>>  hw/ppc/pegasos2.c   |  4 ++--
>>>>>  6 files changed, 12 insertions(+), 31 deletions(-)
>>>>>
>>>>> --
>>>>> 2.45.2
>>>>>
>>>>
>>
>
>