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.kis...@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.kis...@siemens.com>
Signed-off-by: Anthony Liguori <aligu...@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.
