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