[PATCH 0/6] hw/isa: Remove unuseful qemu_allocate_irqs() call

[PATCH 0/6] hw/isa: Remove unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

I started to fix the LeakSanitizer error in piix4_realize(),
then looked for similar pattern and found 2 other places.
The older is i82378 (historically the first one) which then
spread.

Philippe Mathieu-Daudé (6):
  hw/isa/i82378: Name output IRQ as 'intr'
  hw/isa/i82378: Simplify removing unuseful qemu_allocate_irqs() call
  hw/isa/i82378: Rename output IRQ variable
  hw/isa/vt82c686: Name output IRQ as 'intr'
  hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call
  hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call

 hw/isa/i82378.c     | 13 +++----------
 hw/isa/piix4.c      | 10 +---------
 hw/isa/vt82c686.c   | 12 ++----------
 hw/mips/fuloong2e.c |  2 +-
 hw/ppc/prep.c       |  4 ++--
 5 files changed, 9 insertions(+), 32 deletions(-)

-- 
2.26.2

[PATCH 1/6] hw/isa/i82378: Name output IRQ as 'intr'

[Philippe Mathieu-Daudé]

Named IRQs are easier to understand in the monitor.
Name the single output interrupt as 'intr'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/i82378.c | 2 +-
 hw/ppc/prep.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 2a2ff05b937..fd296c8ed7a 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -113,7 +113,7 @@ static void i82378_init(Object *obj)
     DeviceState *dev = DEVICE(obj);
     I82378State *s = I82378(obj);
 
-    qdev_init_gpio_out(dev, s->out, 1);
+    qdev_init_gpio_out_named(dev, s->out, "intr", 1);
     qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
 }
 
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index f1b1efdcef9..50d9b6f0d54 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -284,8 +284,8 @@ static void ibm_40p_init(MachineState *machine)
 
     /* PCI -> ISA bridge */
     i82378_dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
-    qdev_connect_gpio_out(i82378_dev, 0,
-                          cpu->env.irq_inputs[PPC6xx_INPUT_INT]);
+    qdev_connect_gpio_out_named(i82378_dev, "intr", 0,
+                                cpu->env.irq_inputs[PPC6xx_INPUT_INT]);
     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.26.2

[PATCH 2/6] hw/isa/i82378: Simplify removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

When the i82378 model was added in commit a04ff940974 ("prep:
Add i82378 PCI-to-ISA bridge emulation") the i8259 model was
not yet QOM'ified. This happened later in commit 747c70af78f
("i8259: Convert to qdev").

Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, we can now simplify
and directly connect the PIC to the "intr" named output.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/i82378.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index fd296c8ed7a..817eca47053 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -47,12 +47,6 @@ static const VMStateDescription vmstate_i82378 = {
     },
 };
 
-static void i82378_request_out0_irq(void *opaque, int irq, int level)
-{
-    I82378State *s = opaque;
-    qemu_set_irq(s->out[0], level);
-}
-
 static void i82378_request_pic_irq(void *opaque, int irq, int level)
 {
     DeviceState *dev = opaque;
@@ -94,8 +88,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
      */
 
     /* 2 82C59 (irq) */
-    s->i8259 = i8259_init(isabus,
-                          qemu_allocate_irq(i82378_request_out0_irq, s, 0));
+    s->i8259 = i8259_init(isabus, s->out[0]);
     isa_bus_irqs(isabus, s->i8259);
 
     /* 1 82C54 (pit) */
-- 
2.26.2

[PATCH 3/6] hw/isa/i82378: Rename output IRQ variable

[Philippe Mathieu-Daudé]

The i82378 has 2 output IRQs: "INT" and "NMI".
We do not model the NMI, so simplify I82378State by
removing the unused IRQ. To avoid keeping an array of
one element, remove the array and rename the variable.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/i82378.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 817eca47053..164d6c65f64 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -32,7 +32,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(I82378State, I82378)
 struct I82378State {
     PCIDevice parent_obj;
 
-    qemu_irq out[2];
+    qemu_irq intr;
     qemu_irq *i8259;
     MemoryRegion io;
 };
@@ -88,7 +88,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
      */
 
     /* 2 82C59 (irq) */
-    s->i8259 = i8259_init(isabus, s->out[0]);
+    s->i8259 = i8259_init(isabus, s->intr);
     isa_bus_irqs(isabus, s->i8259);
 
     /* 1 82C54 (pit) */
@@ -106,7 +106,7 @@ static void i82378_init(Object *obj)
     DeviceState *dev = DEVICE(obj);
     I82378State *s = I82378(obj);
 
-    qdev_init_gpio_out_named(dev, s->out, "intr", 1);
+    qdev_init_gpio_out_named(dev, &s->intr, "intr", 1);
     qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
 }
 
-- 
2.26.2

[PATCH 4/6] hw/isa/vt82c686: Name output IRQ as 'intr'

[Philippe Mathieu-Daudé]

Named IRQs are easier to understand in the monitor.
Name the single output interrupt as 'intr'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c   | 2 +-
 hw/mips/fuloong2e.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 05d084f6982..87473ec121f 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -387,7 +387,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     qemu_irq *isa_irq;
     int i;
 
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 4f61f2c873b..931385c760f 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -206,7 +206,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
-    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
+    qdev_connect_gpio_out_named(DEVICE(dev), "intr", 0, intc);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
     pci_ide_create_devs(dev);
-- 
2.26.2

[PATCH 5/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

Instead of creating an input IRQ with qemu_allocate_irqs()
to pass it as output IRQ of the PIC, with its handler simply
dispatching into the "intr" output IRQ, simplify by directly
connecting the PIC to the "intr" named output.

Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 87473ec121f..3dc3454858e 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -323,12 +323,6 @@ struct VT82C686BISAState {
     SuperIOConfig superio_cfg;
 };
 
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-    VT82C686BISAState *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
@@ -384,14 +378,12 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     VT82C686BISAState *s = VT82C686B_ISA(d);
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     int i;
 
     qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
-    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+    isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
-- 
2.26.2

[PATCH 6/6] hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

We locally create an input IRQ with qemu_allocate_irqs() to
pass it as output IRQ of the PIC, but its handler simply dispatch
into another of our output IRQ ("intr" output).

Simplify by directly connecting the PIC output to our "intr"
output.

This fixes when using QEMU built with --enable-sanitizers:

  ==338425==ERROR: LeakSanitizer: detected memory leaks

  Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x5641b361e1df in malloc (qemu-system-mips+0x1b201df)
    #1 0x7f995e683958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
    #2 0x5641b5558e36 in qemu_allocate_irqs hw/core/irq.c:66:12
    #3 0x5641b4161817 in piix4_realize hw/isa/piix4.c:171:21
    #4 0x5641b42f077a in pci_qdev_realize hw/pci/pci.c:2114:9
    #5 0x5641b554c802 in device_set_realized hw/core/qdev.c:761:13
    #6 0x5641b5578458 in property_set_bool qom/object.c:2257:5
    #7 0x5641b55709e2 in object_property_set qom/object.c:1402:5
    #8 0x5641b55861c9 in object_property_set_qobject qom/qom-qobject.c:28:10
    #9 0x5641b5571831 in object_property_set_bool qom/object.c:1472:15
   #10 0x5641b55410fd in qdev_realize hw/core/qdev.c:389:12

Fixes: 078778c5a55 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/piix4.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a50d97834c7..79ed20e2a1a 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -103,12 +103,6 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
-static void piix4_request_i8259_irq(void *opaque, int irq, int level)
-{
-    PIIX4State *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
 static void piix4_set_i8259_irq(void *opaque, int irq, int level)
 {
     PIIX4State *s = opaque;
@@ -149,7 +143,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
     ISABus *isa_bus;
-    qemu_irq *i8259_out_irq;
 
     isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
                           pci_address_space_io(dev), errp);
@@ -168,8 +161,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
                                         PIIX_RCR_IOPORT, &s->rcr_mem, 1);
 
     /* initialize i8259 pic */
-    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
-    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+    s->isa = i8259_init(isa_bus, s->cpu_intr);
 
     /* initialize ISA irqs */
     isa_bus_irqs(isa_bus, s->isa);
-- 
2.26.2

Re: [PATCH 5/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call

[BALATON Zoltan]

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

On Wed, 24 Mar 2021, Philippe Mathieu-Daudé wrote:
> Instead of creating an input IRQ with qemu_allocate_irqs()
> to pass it as output IRQ of the PIC, with its handler simply
> dispatching into the "intr" output IRQ, simplify by directly
> connecting the PIC to the "intr" named output.

I think I've tried to do it that way first but it did not work for some 
reason, that's why I had to add the additional handler, but this was about 
a year ago so I don't remember the details. Did you test it still works or 
expect me to test it? (Note that testing with firmware only may not be 
enough as some firmwares don't use interrupts so only booting a guest 
might reveal a problem. Not sure about pegasos2 firmware but sam460ex 
U-Boot seems to poll instead of using IRQs.)

Regards,
BALATON Zoltan

> Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA bridge")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/isa/vt82c686.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 87473ec121f..3dc3454858e 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -323,12 +323,6 @@ struct VT82C686BISAState {
>     SuperIOConfig superio_cfg;
> };
>
> -static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> -{
> -    VT82C686BISAState *s = opaque;
> -    qemu_set_irq(s->cpu_intr, level);
> -}
> -
> static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>                                    uint32_t val, int len)
> {
> @@ -384,14 +378,12 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>     VT82C686BISAState *s = VT82C686B_ISA(d);
>     DeviceState *dev = DEVICE(d);
>     ISABus *isa_bus;
> -    qemu_irq *isa_irq;
>     int i;
>
>     qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
>                           &error_fatal);
> -    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> +    isa_bus_irqs(isa_bus, i8259_init(isa_bus, s->cpu_intr));
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(isa_bus, 0);
>     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>

Re: [PATCH 6/6] hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

ping for review?

On 3/24/21 12:13 AM, Philippe Mathieu-Daudé wrote:
> We locally create an input IRQ with qemu_allocate_irqs() to
> pass it as output IRQ of the PIC, but its handler simply dispatch
> into another of our output IRQ ("intr" output).
> 
> Simplify by directly connecting the PIC output to our "intr"
> output.
> 
> This fixes when using QEMU built with --enable-sanitizers:
> 
>   ==338425==ERROR: LeakSanitizer: detected memory leaks
> 
>   Direct leak of 8 byte(s) in 1 object(s) allocated from:
>     #0 0x5641b361e1df in malloc (qemu-system-mips+0x1b201df)
>     #1 0x7f995e683958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
>     #2 0x5641b5558e36 in qemu_allocate_irqs hw/core/irq.c:66:12
>     #3 0x5641b4161817 in piix4_realize hw/isa/piix4.c:171:21
>     #4 0x5641b42f077a in pci_qdev_realize hw/pci/pci.c:2114:9
>     #5 0x5641b554c802 in device_set_realized hw/core/qdev.c:761:13
>     #6 0x5641b5578458 in property_set_bool qom/object.c:2257:5
>     #7 0x5641b55709e2 in object_property_set qom/object.c:1402:5
>     #8 0x5641b55861c9 in object_property_set_qobject qom/qom-qobject.c:28:10
>     #9 0x5641b5571831 in object_property_set_bool qom/object.c:1472:15
>    #10 0x5641b55410fd in qdev_realize hw/core/qdev.c:389:12
> 
> Fixes: 078778c5a55 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/isa/piix4.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index a50d97834c7..79ed20e2a1a 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -103,12 +103,6 @@ static const VMStateDescription vmstate_piix4 = {
>      }
>  };
>  
> -static void piix4_request_i8259_irq(void *opaque, int irq, int level)
> -{
> -    PIIX4State *s = opaque;
> -    qemu_set_irq(s->cpu_intr, level);
> -}
> -
>  static void piix4_set_i8259_irq(void *opaque, int irq, int level)
>  {
>      PIIX4State *s = opaque;
> @@ -149,7 +143,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>  {
>      PIIX4State *s = PIIX4_PCI_DEVICE(dev);
>      ISABus *isa_bus;
> -    qemu_irq *i8259_out_irq;
>  
>      isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
>                            pci_address_space_io(dev), errp);
> @@ -168,8 +161,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>                                          PIIX_RCR_IOPORT, &s->rcr_mem, 1);
>  
>      /* initialize i8259 pic */
> -    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
> -    s->isa = i8259_init(isa_bus, *i8259_out_irq);
> +    s->isa = i8259_init(isa_bus, s->cpu_intr);
>  
>      /* initialize ISA irqs */
>      isa_bus_irqs(isa_bus, s->isa);
> 

Re: [PATCH 5/6] hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

On 3/24/21 12:43 AM, BALATON Zoltan wrote:
> On Wed, 24 Mar 2021, Philippe Mathieu-Daudé wrote:
>> Instead of creating an input IRQ with qemu_allocate_irqs()
>> to pass it as output IRQ of the PIC, with its handler simply
>> dispatching into the "intr" output IRQ, simplify by directly
>> connecting the PIC to the "intr" named output.
> 
> I think I've tried to do it that way first but it did not work for some
> reason, that's why I had to add the additional handler, but this was
> about a year ago so I don't remember the details. Did you test it still
> works or expect me to test it? (Note that testing with firmware only may
> not be enough as some firmwares don't use interrupts so only booting a
> guest might reveal a problem. Not sure about pegasos2 firmware but
> sam460ex U-Boot seems to poll instead of using IRQs.)

I tested with Fuloong2E (PMON + Linux).

>> Fixes: 3dc31cb8490 ("vt82c686: Move creation of ISA devices to the ISA
>> bridge")
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/isa/vt82c686.c | 10 +---------
>> 1 file changed, 1 insertion(+), 9 deletions(-)

Re: [PATCH 0/6] hw/isa: Remove unuseful qemu_allocate_irqs() call

[Philippe Mathieu-Daudé]

ping (for series or simply patch 6).

On 3/24/21 12:13 AM, Philippe Mathieu-Daudé wrote:
> I started to fix the LeakSanitizer error in piix4_realize(),
> then looked for similar pattern and found 2 other places.
> The older is i82378 (historically the first one) which then
> spread.
> 
> Philippe Mathieu-Daudé (6):
>   hw/isa/i82378: Name output IRQ as 'intr'
>   hw/isa/i82378: Simplify removing unuseful qemu_allocate_irqs() call
>   hw/isa/i82378: Rename output IRQ variable
>   hw/isa/vt82c686: Name output IRQ as 'intr'
>   hw/isa/vt82c686: Simplify removing unuseful qemu_allocate_irqs() call
>   hw/isa/piix4: Fix leak removing unuseful qemu_allocate_irqs() call
> 
>  hw/isa/i82378.c     | 13 +++----------
>  hw/isa/piix4.c      | 10 +---------
>  hw/isa/vt82c686.c   | 12 ++----------
>  hw/mips/fuloong2e.c |  2 +-
>  hw/ppc/prep.c       |  4 ++--
>  5 files changed, 9 insertions(+), 32 deletions(-)
>