Re: [PATCH] x86/dpci: remove the dpci EOI timer

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 13, 2021 at 06:21:03AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > Sent: Wednesday, January 13, 2021 1:33 AM
> > 
> > Current interrupt pass though code will setup a timer for each
> > interrupt injected to the guest that requires an EOI from the guest.
> > Such timer would perform two actions if the guest doesn't EOI the
> > interrupt before a given period of time. The first one is deasserting
> > the virtual line, the second is perform an EOI of the physical
> > interrupt source if it requires such.
> > 
> > The deasserting of the guest virtual line is wrong, since it messes
> > with the interrupt status of the guest. It's not clear why this was
> > odne in the first place, it should be the guest the one to EOI the
> > interrupt and thus deassert the line.
> > 
> > Performing an EOI of the physical interrupt source is redundant, since
> > there's already a timer that takes care of this for all interrupts,
> > not just the HVM dpci ones, see irq_guest_action_t struct eoi_timer
> > field.
> > 
> > Since both of the actions performed by the dpci timer are not
> > required, remove it altogether.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > As with previous patches, I'm having a hard time figuring out why this
> > was required in the first place. I see no reason for Xen to be
> > deasserting the guest virtual line. There's a comment:
> > 
> > /*
> >  * Set a timer to see if the guest can finish the interrupt or not. For
> >  * example, the guest OS may unmask the PIC during boot, before the
> >  * guest driver is loaded. hvm_pci_intx_assert() may succeed, but the
> >  * guest will never deal with the irq, then the physical interrupt line
> >  * will never be deasserted.
> >  */
> > 
> > Did this happen because the device was passed through in a bogus state
> > where it would generate interrupts without the guest requesting
> 
> It could be a case where two devices share the same interrupt line and
> are assigned to different domains. In this case, the interrupt activity of 
> two devices interfere with each other.

This would also seem to be problematic if the device decides to use
MSI instead of INTx, but due to the shared nature of the INTx line we
would continue to inject INTx (generated from another device not
passed through to the guest) to the guest even if the device has
switched to MSI.

> > 
> > Won't the guest face the same issues when booted on bare metal, and
> > thus would already have the means to deal with such issues?
> 
> The original commit was added by me in ~13yrs ago (0f843ba00c95)
> when enabling Xen in a client virtualization environment where interrupt
> sharing is popular.

Thanks, the reference to the above commit is helpful, I wasn't able to
find it and it contains a comment that I think has been lost, which
provides the background on why this was added.

> I believe above comment was recorded for a real 
> problem at the moment (deassert resets the intx line to unblock further
> interrupts). But I'm not sure whether it is still the case after both Xen and 
> guest OS have changed a lot. At least some test from people who
> still use Xen in shared interrupt scenario would be helpful. Or, if such
> usage is already niche, maybe we can consider disallow passing through
> devices which share the same interrupt line to different domains and
> then safely remove this dpci EOI trick.

So the deassert done by timeout only deasserts the virtual line, but
doesn't for example clear the IRR bit from the vIO-APIC pin, which
will cause further interrupts to not be delivered anyway until a
proper EOI (or a switch to trigger mode) is done to the pin.

I think it's going to be complicated for me to find a system that has
two devices I can passthrough sharing the same GSI.

It also makes me wonder why do we need to keep all this internal state
about virtual lines and assert counts. Such virtual lines will get
de-asserted when the guest perform an EOI of the interrupt, which also
clear the PIC pending bits and allows further interrupts to be
injected. Wouldn't it be enough to simply try to assert the guest
irq/gsi, and let the interrupt controller decide whether it needs to
inject another interrupt, or there's one already pending?

vIO-APIC has the IRR field which I think it's fairly similar to what
Xen achieves with all the line logic: prevent another interrupt
injection until the current one has been EOI'ed. The i8259 doesn't
have such field, and might indeed end up with an extra interrupt
queued in IRR while there's one still pending in ISR, but I don't
think that would be harmful to the guest.

Thanks, Roger.