diff mbox series

[v3,5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

Message ID 20210126134521.25784-6-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/intr: HVM guest interrupt handling fixes/cleanup | expand

Commit Message

Roger Pau Monné Jan. 26, 2021, 1:45 p.m. UTC
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.

Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Remove the unmask label.
---
 xen/arch/x86/hvm/vpic.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Jan Beulich Jan. 26, 2021, 4:57 p.m. UTC | #1
On 26.01.2021 14:45, 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.
> 
> Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

As said before, under the assumption that the clearing of IRR
and ISR that we do is correct, I agree with the change. I'd
like to give it some time though before giving my R-b here, for
the inquiry to hopefully get answered. After all there's still
the possibility of us needing to instead squash that clearing
(which then would seem to result in getting things in sync the
other way around).

Jan

> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -197,6 +197,8 @@ static void vpic_ioport_write(
>      {
>          if ( val & 0x10 )
>          {
> +            unsigned int pending = vpic->isr | (vpic->irr & ~vpic->elcr);
> +
>              /* ICW1 */
>              /* Clear edge-sensing logic. */
>              vpic->irr &= vpic->elcr;
> @@ -220,6 +222,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);
> +            }
> +            return;
>          }
>          else if ( val & 0x08 )
>          {
>
Roger Pau Monné Jan. 27, 2021, 9:15 a.m. UTC | #2
On Tue, Jan 26, 2021 at 05:57:49PM +0100, Jan Beulich wrote:
> On 26.01.2021 14:45, 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.
> > 
> > Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> As said before, under the assumption that the clearing of IRR
> and ISR that we do is correct, I agree with the change. I'd
> like to give it some time though before giving my R-b here, for
> the inquiry to hopefully get answered. After all there's still
> the possibility of us needing to instead squash that clearing
> (which then would seem to result in getting things in sync the
> other way around).

OK, let's wait a bit to see what Intel replies. I assume this would
qualify as a bugfix for getting it committed later?

Thanks, Roger.
Jan Beulich Jan. 27, 2021, 9:30 a.m. UTC | #3
On 27.01.2021 10:15, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 05:57:49PM +0100, Jan Beulich wrote:
>> On 26.01.2021 14:45, 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.
>>>
>>> Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> As said before, under the assumption that the clearing of IRR
>> and ISR that we do is correct, I agree with the change. I'd
>> like to give it some time though before giving my R-b here, for
>> the inquiry to hopefully get answered. After all there's still
>> the possibility of us needing to instead squash that clearing
>> (which then would seem to result in getting things in sync the
>> other way around).
> 
> OK, let's wait a bit to see what Intel replies. I assume this would
> qualify as a bugfix for getting it committed later?

It would be up to Ian then to judge, but as long as it fixes an
actual bug, my understanding is it will still qualify for being
considered.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 795a76768d..f465b7f997 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -197,6 +197,8 @@  static void vpic_ioport_write(
     {
         if ( val & 0x10 )
         {
+            unsigned int pending = vpic->isr | (vpic->irr & ~vpic->elcr);
+
             /* ICW1 */
             /* Clear edge-sensing logic. */
             vpic->irr &= vpic->elcr;
@@ -220,6 +222,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);
+            }
+            return;
         }
         else if ( val & 0x08 )
         {