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.