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

[David Woodhouse]

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?

smime.p7s
Description: S/MIME cryptographic signature