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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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() (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;

?

Jan