Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

On Wed, 1 Mar 2023, David Woodhouse wrote:
> On Wed, 2023-03-01 at 12:27 +0100, BALATON Zoltan wrote:
> > On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> > > Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > > > MorphOS sets the ISA PIC to level sensitive mode but QEMU does
> > > > not
> > > > support that so this causes a freeze if multiple devices try to
> > > > raise
> > > > a shared interrupt. Work around it by lowering the interrupt
> > > > before
> > > > raising it again if it is already raised. This could be reverted
> > > > when
> > > > the i8259 model is fixed.
> > > >
> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > ---
> > > > hw/isa/vt82c686.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > > index 018a119964..3e44a51f92 100644
> > > > --- a/hw/isa/vt82c686.c
> > > > +++ b/hw/isa/vt82c686.c
> > > > @@ -549,6 +549,7 @@ struct ViaISAState {
> > > >      PCIDevice dev;
> > > >      qemu_irq cpu_intr;
> > > >      qemu_irq *isa_irqs_in;
> > > > +    uint16_t isa_irqs_state;
> > > >      ViaSuperIOState via_sio;
> > > >      MC146818RtcState rtc;
> > > >      PCIIDEState ide;
> > > > @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void
> > > > *opaque, int irq_num, int level)
> > > >              pic_level |= pci_bus_get_irq_level(bus, i);
> > > >          }
> > > >      }
> > > > +    /* FIXME: workaround for i8259: level sensitive irq not
> > > > supported */
> > > > +    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
> > > > +        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
> > > > +    } else if (pic_level) {
> > > > +        s->isa_irqs_state |= BIT(pic_irq);
> > > > +    } else {
> > > > +        s->isa_irqs_state &= ~BIT(pic_irq);
> > > > +    }
> > >
> > > Let's not clutter the device model with workarounds which quickly
> > > snowball into unmaintainable code. Please fix the i8259 instead.
> >
> > Do you have an idea how?
>
> Let's start by understanding the problem completely. The i8259 *does*
> support level-triggered interrupts. Look at the checks for s->elcr in
> hw/intc/i8259.c, in pic_set_irq()...
>
>    if (s->elcr & mask) {
>        /* level triggered */
>        if (level) {
>            s->irr |= mask;
>            s->last_irr |= mask;
>        } else {
>            s->irr &= ~mask;
>            s->last_irr &= ~mask;
>        }
>    } else {
>        /* edge triggered */
>
>
> ... and in pic_intack() ...
>
>
>    /* We don't clear a level sensitive interrupt here */
>    if (!(s->elcr & (1 << irq))) {
>        s->irr &= ~(1 << irq);
>    }
>
>
> What qemu typically has an issue with is *shared* level-triggered
> interrupts. But that would cause a level to be "forgotten" if another
> input asserts and then deasserts the IRQ while one input thinks it's
> holding it asserted. And I don't see how your workaround above would
> have helped in that situation.
>
> Are you sure the PIC ELCR is actually set for the lines you're having
> trouble with? Is that something the Pegasos SmartFirmware would have
> done, and MorphOS is expecting to inherit but isn't actually setting up
> for itself?

No, it works with other guests like Linux and AmigaOS that use PIC as set 
up by the firmware but MorphOS tries to use it in level sensitive mode and 
likely has an IRQ handler which expects this to work. This is where I've 
debugged it and came to this workaround:

https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html

When booting MorphOS with -d unimp I see these logs:

i8259: level sensitive irq not supported
i8259: level sensitive irq not supported

which is I guess when it tries to set it for both PICs. (If you want to 
try this MorphOS iso is downloadable and instructions how to boot it is 
here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos

Regards,
BALATON Zoltan