Re: [PATCH v2 3/4] x86/vpic: issue dpci EOI for cleared pins at ICW1

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Jan 22, 2021 at 10:02:15AM +0100, Jan Beulich wrote:
> On 15.01.2021 15:28, Roger Pau Monne wrote:
> > When pins are cleared from either ISR or IRR as part of the
> > initialization sequence forward the clearing of those pins to the dpci
> > EOI handler, as it is equivalent to an EOI. Not doing so can bring the
> > interrupt controller state out of sync with the dpci handling logic,
> > that expects a notification when a pin has been EOI'ed.
> 
> The question though is what this clearing of ISR and some of
> IRR during ICW1 is based upon: Going through various manuals
> (up-to-date from Nov 2020, intermediate, and all the way
> through to an old hard copy from 1993) I can't find a single
> mention of ICW1 having any effect on ISR or IRR, despite both
> soft copy ones being detailed about other state getting
> changed.
> 
> There is "Following initialization, an interrupt request (IRQ)
> input must make a low-to-high transition to generate an
> interrupt", but I'm afraid this can be read to mean different
> things. And if it was meant to describe an effect on ISR
> and/or IRR, it would imo be in conflict with us keeping IRR
> bits of level triggered interrupts.

I have an old copy of the 8259A spec, and it does mention the same
quote that you have above. I also wondered while I was adjusting this
code whether what we do is fine. I have to admit I haven't considered
changing this logic much because I don't have an effective way to test
it.

I've also taken a look at what others do, QEMU for example will do
exactly the same logic as Xen during ICW1, bhyve OTOH will fully zero
IRR and leave ISR as is.

Doing a bit of archaeology in QEMU I've found the following change:

commit aa24822bdc7c4e74afbc6fa1324b01cf067da7cb
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Tue Jan 24 16:29:29 2012 +0100

    i8259: Do not clear level-triggered lines in IRR on init

    When an input line is handled as level-triggered, it will immediately
    raise an IRQ on the output of a PIC again that goes through an init
    reset. So only clear the edge-triggered inputs from IRR in that
    scenario.

    Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Which seems to point out there's a reasoning behind what it's
currently implemented. This seems to be against the spec as there's no
low-to-high transition?

> > @@ -217,6 +219,24 @@ static void vpic_ioport_write(
> >              }
> >  
> >              vpic->init_state = ((val & 3) << 2) | 1;
> > +            vpic_update_int_output(vpic);
> > +            vpic_unlock(vpic);
> > +
> > +            /*
> > +             * Forward the EOI of any pending or in service interrupt that has
> > +             * been cleared from IRR or ISR, or else the dpci logic will get
> > +             * out of sync with the state of the interrupt controller.
> > +             */
> > +            while ( pending )
> > +            {
> > +                unsigned int pin = __scanbit(pending, 8);
> > +
> > +                ASSERT(pin < 8);
> > +                hvm_dpci_eoi(current->domain,
> > +                             hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
> > +                __clear_bit(pin, &pending);
> > +            }
> > +            goto unmask;
> 
> A similar consideration applies here (albeit just as an
> observation, as being orthogonal to your change): A PIC
> becomes ready for handling interrupts only at the end of the
> ICWx sequence. Hence it would seem to me that invoking
> pt_may_unmask_irq()

Right, it might be best to force unmask = 1 when init_state gets set
to 0. A spurious call to pt_may_unmask_irq won't be harmful anyway,
even if no pins have been actually unmasked.

> (maybe also vpic_update_int_output()) at
> ICW1 is premature. To me this seems particularly relevant
> considering that ICW1 clears IMR, and hence an interrupt
> becoming pending between ICW1 and ICW2 wouldn't know which
> vector to use.
> 
> Or maybe on that side of things, vpic_update_int_output()
> should really do
> 
>     if ( vpic->int_output == (!vpic->init_state && irq >= 0) )
>         return;
> 
>     /* INT line transition L->H or H->L. */
>     vpic->int_output = !vpic->init_state && !vpic->int_output;
> 
> ?

So to force int_output = false when in init state. Seems reasonable, I
can implement this either as a pre-patch or a followup, but again I'm
not sure I have means to properly test it.

Thanks, Roger.