[PATCH 0/2] Solve vt82c686 qemu_irq leak.

[PATCH 0/2] Solve vt82c686 qemu_irq leak.

[BALATON Zoltan]

This is an alternative appriach to solve the qemu_irq leak in
vt82c686. Allowing embedding an irq and init it in place like done
with other objects may allow cleaner fix for similar issues and I also
plan to use this for adding qemu_itq to pegasos2 machine state for
which gpio would not work.

BALATON Zoltan (2):
  hw: Move declaration of IRQState to header and add init function
  hw/isa/vt82c686.c: Embed i8259 irq in device state instead of
    allocating

 hw/core/irq.c     | 25 +++++++++++--------------
 hw/isa/vt82c686.c |  7 ++++---
 include/hw/irq.h  | 18 ++++++++++++++++++
 3 files changed, 33 insertions(+), 17 deletions(-)

-- 
2.30.9

[PATCH 1/2] hw: Move declaration of IRQState to header and add init function

[BALATON Zoltan]

To allow embedding a qemu_irq in a struct move its definition to the
header and add a function to init it in place without allocating it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/core/irq.c    | 25 +++++++++++--------------
 include/hw/irq.h | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/hw/core/irq.c b/hw/core/irq.c
index 3f14e2dda7..db95ffc18f 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -26,16 +26,6 @@
 #include "hw/irq.h"
 #include "qom/object.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(IRQState, IRQ)
-
-struct IRQState {
-    Object parent_obj;
-
-    qemu_irq_handler handler;
-    void *opaque;
-    int n;
-};
-
 void qemu_set_irq(qemu_irq irq, int level)
 {
     if (!irq)
@@ -44,6 +34,15 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
+void qemu_init_irq(IRQState *irq, qemu_irq_handler handler, void *opaque,
+                   int n)
+{
+    object_initialize(irq, sizeof(*irq), TYPE_IRQ);
+    irq->handler = handler;
+    irq->opaque = opaque;
+    irq->n = n;
+}
+
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {
@@ -69,10 +68,8 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
 {
     IRQState *irq;
 
-    irq = IRQ(object_new(TYPE_IRQ));
-    irq->handler = handler;
-    irq->opaque = opaque;
-    irq->n = n;
+    irq = g_new(IRQState, 1);
+    qemu_init_irq(irq, handler, opaque, n);
 
     return irq;
 }
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 645b73d251..c861c1debd 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -1,9 +1,20 @@
 #ifndef QEMU_IRQ_H
 #define QEMU_IRQ_H
 
+#include "qom/object.h"
+
 /* Generic IRQ/GPIO pin infrastructure.  */
 
 #define TYPE_IRQ "irq"
+OBJECT_DECLARE_SIMPLE_TYPE(IRQState, IRQ)
+
+struct IRQState {
+    Object parent_obj;
+
+    qemu_irq_handler handler;
+    void *opaque;
+    int n;
+};
 
 void qemu_set_irq(qemu_irq irq, int level);
 
@@ -23,6 +34,13 @@ static inline void qemu_irq_pulse(qemu_irq irq)
     qemu_set_irq(irq, 0);
 }
 
+/*
+ * Init a single IRQ. The irq is assigned with a handler, an opaque data
+ * and the interrupt number.
+ */
+void qemu_init_irq(IRQState *irq, qemu_irq_handler handler, void *opaque,
+                   int n);
+
 /* Returns an array of N IRQs. Each IRQ is assigned the argument handler and
  * opaque data.
  */
-- 
2.30.9

Re: [PATCH 1/2] hw: Move declaration of IRQState to header and add init function

[Peter Maydell]

On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> To allow embedding a qemu_irq in a struct move its definition to the
> header and add a function to init it in place without allocating it.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Yes, I think this makes sense, and I'm not quite sure
why we don't support it already (probably just historical
inertia -- the qemu_irq APIs were written before QOM
and when the whole design was oriented around an
"allocate and return a pointer" style rather than
allowing for embedding structs).

-- PMM

[PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[BALATON Zoltan]

To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
device state instead of allocating it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8582ac0322..834051abeb 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
 
 struct ViaISAState {
     PCIDevice dev;
+
+    IRQState i8259_irq;
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs_in;
     uint16_t irq_state[ISA_NUM_IRQS];
@@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
+    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
@@ -729,7 +730,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->i8259_irq);
     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);
-- 
2.30.9

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Peter Maydell]

On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
> device state instead of allocating it.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8582ac0322..834051abeb 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>
>  struct ViaISAState {
>      PCIDevice dev;
> +
> +    IRQState i8259_irq;
>      qemu_irq cpu_intr;
>      qemu_irq *isa_irqs_in;
>      uint16_t irq_state[ISA_NUM_IRQS];
> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                            errp);

So if I understand correctly, this IRQ line isn't visible
from outside this chip, we're just trying to wire together
two internal components of the chip? If so, I agree that
this seems a better way than creating a named GPIO that
we then have to document as a "not really an external
connection, don't try to use this" line. (We've done that
before I think in other devices, and it works but it's
a bit odd-looking.)

That said, I do notice that the via_isa_request_i8259_irq()
function doesn't do anything except pass the level onto
another qemu_irq, so I think the theoretical ideal would be
if we could arrange to plumb things directly through rather
than needing this extra qemu_irq and function. There's
probably a reason (order of device creation/connection?)
that doesn't work though.

-- PMM

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[BALATON Zoltan]

On Mon, 1 Jul 2024, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>> device state instead of allocating it.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8582ac0322..834051abeb 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>
>>  struct ViaISAState {
>>      PCIDevice dev;
>> +
>> +    IRQState i8259_irq;
>>      qemu_irq cpu_intr;
>>      qemu_irq *isa_irqs_in;
>>      uint16_t irq_state[ISA_NUM_IRQS];
>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>                            errp);
>
> So if I understand correctly, this IRQ line isn't visible
> from outside this chip, we're just trying to wire together
> two internal components of the chip? If so, I agree that
> this seems a better way than creating a named GPIO that
> we then have to document as a "not really an external
> connection, don't try to use this" line. (We've done that
> before I think in other devices, and it works but it's
> a bit odd-looking.)
>
> That said, I do notice that the via_isa_request_i8259_irq()
> function doesn't do anything except pass the level onto
> another qemu_irq, so I think the theoretical ideal would be
> if we could arrange to plumb things directly through rather
> than needing this extra qemu_irq and function. There's
> probably a reason (order of device creation/connection?)
> that doesn't work though.

Philippe tried that before but was found not working. There's some history 
that I don't remember aby more in commit 38200011319b58.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Mark Cave-Ayland]

On 01/07/2024 13:58, Peter Maydell wrote:

> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>> device state instead of allocating it.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/isa/vt82c686.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8582ac0322..834051abeb 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>
>>   struct ViaISAState {
>>       PCIDevice dev;
>> +
>> +    IRQState i8259_irq;
>>       qemu_irq cpu_intr;
>>       qemu_irq *isa_irqs_in;
>>       uint16_t irq_state[ISA_NUM_IRQS];
>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>       isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>                             errp);
> 
> So if I understand correctly, this IRQ line isn't visible
> from outside this chip, we're just trying to wire together
> two internal components of the chip? If so, I agree that
> this seems a better way than creating a named GPIO that
> we then have to document as a "not really an external
> connection, don't try to use this" line. (We've done that
> before I think in other devices, and it works but it's
> a bit odd-looking.)

It's basically wiring up the 8259 PIC (ISA) IRQs which aren't implemented using qdev 
gpios to an internal IRQ handler.

If the 8259 PIC (ISA) IRQs were already converted to qdev gpios, then presumably 
using a qdev gpio would be the correct solution for this? I'm conscious that if we do 
decide to introduce another method for wiring IRQs then there should be clear 
criteria for developers and reviewers as to which method is appropriate for each use 
case.


ATB,

Mark.

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>> device state instead of allocating it.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8582ac0322..834051abeb 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>
>>  struct ViaISAState {
>>      PCIDevice dev;
>> +
>> +    IRQState i8259_irq;
>>      qemu_irq cpu_intr;
>>      qemu_irq *isa_irqs_in;
>>      uint16_t irq_state[ISA_NUM_IRQS];
>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>                            errp);
>
>So if I understand correctly, this IRQ line isn't visible
>from outside this chip,

Actally it is, in the form of the INTR pin. Assuming similar naming conventions in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 schematics (linked in the QEMU documentation) suggest that this pin is actually used there, although not modeled in QEMU. Compare this to how the "intr" pin is exposed by the piix4 device model and wired in the Malta board.

> we're just trying to wire together
>two internal components of the chip? If so, I agree that
>this seems a better way than creating a named GPIO that
>we then have to document as a "not really an external
>connection, don't try to use this" line. (We've done that
>before I think in other devices, and it works but it's
>a bit odd-looking.)
>
>That said, I do notice that the via_isa_request_i8259_irq()
>function doesn't do anything except pass the level onto
>another qemu_irq, so I think the theoretical ideal would be
>if we could arrange to plumb things directly through rather
>than needing this extra qemu_irq and function. There's
>probably a reason (order of device creation/connection?)
>that doesn't work though.

I think there could be a general pattern of device creation/connection which I've outlined here: https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.com/

Best regards,
Bernhard

>
>-- PMM
>

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>>On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>>> device state instead of allocating it.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/isa/vt82c686.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8582ac0322..834051abeb 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>>
>>>  struct ViaISAState {
>>>      PCIDevice dev;
>>> +
>>> +    IRQState i8259_irq;
>>>      qemu_irq cpu_intr;
>>>      qemu_irq *isa_irqs_in;
>>>      uint16_t irq_state[ISA_NUM_IRQS];
>>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>                            errp);
>>
>>So if I understand correctly, this IRQ line isn't visible
>>from outside this chip,
>
>Actally it is, in the form of the INTR pin. Assuming similar naming conventions in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 schematics (linked in the QEMU documentation) suggest that this pin is actually used there, although not modeled in QEMU.

Well, QEMU does actually wire the intr pin in the pegasos2 board code, except that it isn't a named gpio like in piix4. If we allow this pin to be wired before the south bridge's realize we might be able to eliminate the "intermediate irq forwarder" as Phil used to name it, resulting in less and more efficient code. This solution would basically follow the pattern I outlined under below link.

Best regards,
Bernhard

> Compare this to how the "intr" pin is exposed by the piix4 device model and wired in the Malta board.
>
>> we're just trying to wire together
>>two internal components of the chip? If so, I agree that
>>this seems a better way than creating a named GPIO that
>>we then have to document as a "not really an external
>>connection, don't try to use this" line. (We've done that
>>before I think in other devices, and it works but it's
>>a bit odd-looking.)
>>
>>That said, I do notice that the via_isa_request_i8259_irq()
>>function doesn't do anything except pass the level onto
>>another qemu_irq, so I think the theoretical ideal would be
>>if we could arrange to plumb things directly through rather
>>than needing this extra qemu_irq and function. There's
>>probably a reason (order of device creation/connection?)
>>that doesn't work though.
>
>I think there could be a general pattern of device creation/connection which I've outlined here: https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.com/
>
>Best regards,
>Bernhard
>
>>
>>-- PMM
>>

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[BALATON Zoltan]

On Tue, 2 Jul 2024, Bernhard Beschow wrote:
> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>>> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>
>>>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>>>> device state instead of allocating it.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/isa/vt82c686.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 8582ac0322..834051abeb 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>>>
>>>>  struct ViaISAState {
>>>>      PCIDevice dev;
>>>> +
>>>> +    IRQState i8259_irq;
>>>>      qemu_irq cpu_intr;
>>>>      qemu_irq *isa_irqs_in;
>>>>      uint16_t irq_state[ISA_NUM_IRQS];
>>>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>>>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>>                            errp);
>>>
>>> So if I understand correctly, this IRQ line isn't visible
>>> from outside this chip,
>>
>> Actally it is, in the form of the INTR pin. Assuming similar naming

The INTR pin corresponds to qemu_irq cpu_intr not the i8259_irq.

>> conventions in vt82xx and piix, one can confirm this by consulting the 
>> piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". 
>> Moreover, the pegasos2 schematics (linked in the QEMU documentation) 
>> suggest that this pin is actually used there, although not modeled in 
>> QEMU.
>
> Well, QEMU does actually wire the intr pin in the pegasos2 board code, 
> except that it isn't a named gpio like in piix4. If we allow this pin to

I could make that named to make it clearer, now it's the only output gpio 
so did not name it as usually devices that only have one output don't use 
named gpios for that.

> be wired before the south bridge's realize we might be able to eliminate 
> the "intermediate irq forwarder" as Phil used to name it, resulting in 
> less and more efficient code. This solution would basically follow the 
> pattern I outlined under below link.

I think the problem here is that i8259 does not provide an output gpio for 
this interrupt that the VT82xx could pass on but instead i8259_init() 
needs a qemu_irq to be passed rhat the i8259 model will set. This seems to 
be a legacy init function so the fix may be to Qdev-ify i8259 and add an 
output irq to it then its users could instantiate and connect its IRQs as 
usual and we don't need to create a qemu_irq to pass it to i8259_init().

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
>> Compare this to how the "intr" pin is exposed by the piix4 device model and wired in the Malta board.
>>
>>> we're just trying to wire together
>>> two internal components of the chip? If so, I agree that
>>> this seems a better way than creating a named GPIO that
>>> we then have to document as a "not really an external
>>> connection, don't try to use this" line. (We've done that
>>> before I think in other devices, and it works but it's
>>> a bit odd-looking.)
>>>
>>> That said, I do notice that the via_isa_request_i8259_irq()
>>> function doesn't do anything except pass the level onto
>>> another qemu_irq, so I think the theoretical ideal would be
>>> if we could arrange to plumb things directly through rather
>>> than needing this extra qemu_irq and function. There's
>>> probably a reason (order of device creation/connection?)
>>> that doesn't work though.
>>
>> I think there could be a general pattern of device creation/connection which I've outlined here: https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.com/
>>
>> Best regards,
>> Bernhard
>>
>>>
>>> -- PMM
>>>
>
>

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 3. Juli 2024 00:09:45 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 2 Jul 2024, Bernhard Beschow wrote:
>> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>> 
>>>>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>>>>> device state instead of allocating it.
>>>>> 
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/isa/vt82c686.c | 7 ++++---
>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 8582ac0322..834051abeb 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>>>> 
>>>>>  struct ViaISAState {
>>>>>      PCIDevice dev;
>>>>> +
>>>>> +    IRQState i8259_irq;
>>>>>      qemu_irq cpu_intr;
>>>>>      qemu_irq *isa_irqs_in;
>>>>>      uint16_t irq_state[ISA_NUM_IRQS];
>>>>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>>>>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>>>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>>>                            errp);
>>>> 
>>>> So if I understand correctly, this IRQ line isn't visible
>>>> from outside this chip,
>>> 
>>> Actally it is, in the form of the INTR pin. Assuming similar naming
>
>The INTR pin corresponds to qemu_irq cpu_intr not the i8259_irq.
>
>>> conventions in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 schematics (linked in the QEMU documentation) suggest that this pin is actually used there, although not modeled in QEMU.
>> 
>> Well, QEMU does actually wire the intr pin in the pegasos2 board code, except that it isn't a named gpio like in piix4. If we allow this pin to
>
>I could make that named to make it clearer, now it's the only output gpio so did not name it as usually devices that only have one output don't use named gpios for that.
>
>> be wired before the south bridge's realize we might be able to eliminate the "intermediate irq forwarder" as Phil used to name it, resulting in less and more efficient code. This solution would basically follow the pattern I outlined under below link.
>
>I think the problem here is that i8259 does not provide an output gpio for this interrupt that the VT82xx could pass on but instead i8259_init() needs a qemu_irq to be passed rhat the i8259 model will set. This seems to be a legacy init function so the fix may be to Qdev-ify i8259 and add an output irq to it then its users could instantiate and connect its IRQs as usual and we don't need to create a qemu_irq to pass it to i8259_init().

I've implemented the approach avoiding the intermediate IRQ forwarders here: https://github.com/shentok/qemu/commits/upstream/vt82c686-irq/ . I'd send this series to the list as soon as I resolve some email authentication issues.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Best regards,
>> Bernhard
>> 
>>> Compare this to how the "intr" pin is exposed by the piix4 device model and wired in the Malta board.
>>> 
>>>> we're just trying to wire together
>>>> two internal components of the chip? If so, I agree that
>>>> this seems a better way than creating a named GPIO that
>>>> we then have to document as a "not really an external
>>>> connection, don't try to use this" line. (We've done that
>>>> before I think in other devices, and it works but it's
>>>> a bit odd-looking.)
>>>> 
>>>> That said, I do notice that the via_isa_request_i8259_irq()
>>>> function doesn't do anything except pass the level onto
>>>> another qemu_irq, so I think the theoretical ideal would be
>>>> if we could arrange to plumb things directly through rather
>>>> than needing this extra qemu_irq and function. There's
>>>> probably a reason (order of device creation/connection?)
>>>> that doesn't work though.
>>> 
>>> I think there could be a general pattern of device creation/connection which I've outlined here: https://lore.kernel.org/qemu-devel/0FFB5FD2-08CE-4CEC-9001-E7AC24407A44@gmail.com/
>>> 
>>> Best regards,
>>> Bernhard
>>> 
>>>> 
>>>> -- PMM
>>>> 
>> 
>> 

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[BALATON Zoltan]

On Wed, 3 Jul 2024, Bernhard Beschow wrote:
> Am 3. Juli 2024 00:09:45 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Tue, 2 Jul 2024, Bernhard Beschow wrote:
>>> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>> Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>>> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>>>
>>>>>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>>>>>> device state instead of allocating it.
>>>>>>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>>  hw/isa/vt82c686.c | 7 ++++---
>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 8582ac0322..834051abeb 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>>>>>
>>>>>>  struct ViaISAState {
>>>>>>      PCIDevice dev;
>>>>>> +
>>>>>> +    IRQState i8259_irq;
>>>>>>      qemu_irq cpu_intr;
>>>>>>      qemu_irq *isa_irqs_in;
>>>>>>      uint16_t irq_state[ISA_NUM_IRQS];
>>>>>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>>>>>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>>>>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>>>>                            errp);
>>>>>
>>>>> So if I understand correctly, this IRQ line isn't visible
>>>>> from outside this chip,
>>>>
>>>> Actally it is, in the form of the INTR pin. Assuming similar naming
>>
>> The INTR pin corresponds to qemu_irq cpu_intr not the i8259_irq.
>>
>>>> conventions in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 schematics (linked in the QEMU documentation) suggest that this pin is actually used there, although not modeled in QEMU.
>>>
>>> Well, QEMU does actually wire the intr pin in the pegasos2 board code, except that it isn't a named gpio like in piix4. If we allow this pin to
>>
>> I could make that named to make it clearer, now it's the only output gpio so did not name it as usually devices that only have one output don't use named gpios for that.
>>
>>> be wired before the south bridge's realize we might be able to eliminate the "intermediate irq forwarder" as Phil used to name it, resulting in less and more efficient code. This solution would basically follow the pattern I outlined under below link.
>>
>> I think the problem here is that i8259 does not provide an output gpio for this interrupt that the VT82xx could pass on but instead i8259_init() needs a qemu_irq to be passed rhat the i8259 model will set. This seems to be a legacy init function so the fix may be to Qdev-ify i8259 and add an output irq to it then its users could instantiate and connect its IRQs as usual and we don't need to create a qemu_irq to pass it to i8259_init().
>
> I've implemented the approach avoiding the intermediate IRQ forwarders 
> here: https://github.com/shentok/qemu/commits/upstream/vt82c686-irq/ . 
> I'd send this series to the list as soon as I resolve some email 
> authentication issues.

This connects the gpio out before the device is realized. I don't think 
that's the right fix and confuses all the users of this device as they 
will need to remember to do this. I think the current interrupt forwarder 
is OK until i8259 is Qdev-ified and solves this within the device. I'm OK 
with the patch that makes intr named if you can submit just that.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 3. Juli 2024 11:13:08 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 3 Jul 2024, Bernhard Beschow wrote:
>> Am 3. Juli 2024 00:09:45 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Tue, 2 Jul 2024, Bernhard Beschow wrote:
>>>> Am 2. Juli 2024 18:42:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>>> Am 1. Juli 2024 12:58:15 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>>>> On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>>>> 
>>>>>>> To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>>>>>>> device state instead of allocating it.
>>>>>>> 
>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>> ---
>>>>>>>  hw/isa/vt82c686.c | 7 ++++---
>>>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>>> index 8582ac0322..834051abeb 100644
>>>>>>> --- a/hw/isa/vt82c686.c
>>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>>> @@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>>>>>> 
>>>>>>>  struct ViaISAState {
>>>>>>>      PCIDevice dev;
>>>>>>> +
>>>>>>> +    IRQState i8259_irq;
>>>>>>>      qemu_irq cpu_intr;
>>>>>>>      qemu_irq *isa_irqs_in;
>>>>>>>      uint16_t irq_state[ISA_NUM_IRQS];
>>>>>>> @@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>>>>>>> +    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>>>>>>>      isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>>>>>                            errp);
>>>>>> 
>>>>>> So if I understand correctly, this IRQ line isn't visible
>>>>>> from outside this chip,
>>>>> 
>>>>> Actally it is, in the form of the INTR pin. Assuming similar naming
>>> 
>>> The INTR pin corresponds to qemu_irq cpu_intr not the i8259_irq.
>>> 
>>>>> conventions in vt82xx and piix, one can confirm this by consulting the piix4 datasheet, "Figure 5. Interrupt Controller Block Diagram". Moreover, the pegasos2 schematics (linked in the QEMU documentation) suggest that this pin is actually used there, although not modeled in QEMU.
>>>> 
>>>> Well, QEMU does actually wire the intr pin in the pegasos2 board code, except that it isn't a named gpio like in piix4. If we allow this pin to
>>> 
>>> I could make that named to make it clearer, now it's the only output gpio so did not name it as usually devices that only have one output don't use named gpios for that.
>>> 
>>>> be wired before the south bridge's realize we might be able to eliminate the "intermediate irq forwarder" as Phil used to name it, resulting in less and more efficient code. This solution would basically follow the pattern I outlined under below link.
>>> 
>>> I think the problem here is that i8259 does not provide an output gpio for this interrupt that the VT82xx could pass on but instead i8259_init() needs a qemu_irq to be passed rhat the i8259 model will set. This seems to be a legacy init function so the fix may be to Qdev-ify i8259 and add an output irq to it then its users could instantiate and connect its IRQs as usual and we don't need to create a qemu_irq to pass it to i8259_init().
>> 
>> I've implemented the approach avoiding the intermediate IRQ forwarders here: https://github.com/shentok/qemu/commits/upstream/vt82c686-irq/ . I'd send this series to the list as soon as I resolve some email authentication issues.
>
>This connects the gpio out before the device is realized. I don't think that's the right fix and confuses all the users of this device as they will need to remember to do this. I think the current interrupt forwarder is OK until i8259 is Qdev-ified and solves this within the device. I'm OK with the patch that makes intr named if you can submit just that.

I've now sent a series with naming the gpio as the first patch: https://lore.kernel.org/qemu-devel/20240704205854.18537-1-shentey@gmail.com/

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating

[Bernhard Beschow]

Am 29. Juni 2024 20:01:54 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
>device state instead of allocating it.
>
>Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>---
> hw/isa/vt82c686.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>index 8582ac0322..834051abeb 100644
>--- a/hw/isa/vt82c686.c
>+++ b/hw/isa/vt82c686.c
>@@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
> 
> struct ViaISAState {
>     PCIDevice dev;
>+
>+    IRQState i8259_irq;

Rename s/i8259/intr/ to match the pin name of the chip? Same for commit message and subject.

For consistency with piix4 it might be cleaner to export the pin. Though not need at the moment since it has no users.

With the renaming applied:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs_in;
>     uint16_t irq_state[ISA_NUM_IRQS];
>@@ -715,13 +717,12 @@ 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(dev, &s->cpu_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);
>+    qemu_init_irq(&s->i8259_irq, via_isa_request_i8259_irq, s, 0);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
> 
>@@ -729,7 +730,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->i8259_irq);
>     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);

Re: [PATCH 0/2] Solve vt82c686 qemu_irq leak.

[Michael S. Tsirkin]

On Sat, Jun 29, 2024 at 10:01:52PM +0200, BALATON Zoltan wrote:
> This is an alternative appriach to solve the qemu_irq leak in
> vt82c686. Allowing embedding an irq and init it in place like done
> with other objects may allow cleaner fix for similar issues and I also
> plan to use this for adding qemu_itq to pegasos2 machine state for
> which gpio would not work.
> 
> BALATON Zoltan (2):
>   hw: Move declaration of IRQState to header and add init function
>   hw/isa/vt82c686.c: Embed i8259 irq in device state instead of
>     allocating

This looked like a simpler approach to shut up analyzer warnings, so I
picked this one.



>  hw/core/irq.c     | 25 +++++++++++--------------
>  hw/isa/vt82c686.c |  7 ++++---
>  include/hw/irq.h  | 18 ++++++++++++++++++
>  3 files changed, 33 insertions(+), 17 deletions(-)
> 
> -- 
> 2.30.9
> 
> 

Re: [PATCH 0/2] Solve vt82c686 qemu_irq leak.

[BALATON Zoltan]

On Tue, 10 Sep 2024, Michael S. Tsirkin wrote:
> On Sat, Jun 29, 2024 at 10:01:52PM +0200, BALATON Zoltan wrote:
>> This is an alternative appriach to solve the qemu_irq leak in
>> vt82c686. Allowing embedding an irq and init it in place like done
>> with other objects may allow cleaner fix for similar issues and I also
>> plan to use this for adding qemu_itq to pegasos2 machine state for
>> which gpio would not work.
>>
>> BALATON Zoltan (2):
>>   hw: Move declaration of IRQState to header and add init function
>>   hw/isa/vt82c686.c: Embed i8259 irq in device state instead of
>>     allocating
>
> This looked like a simpler approach to shut up analyzer warnings, so I
> picked this one.

Thanks. Looks like you had some mixup with adding your Signed-off-by 
though but I did not notice that in the pull request only now that it 
landed in master. (Just in case this can be corrected somehow in git but 
otherwise it probably does not matter much.)

Regards,
BALATON Zoltan