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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Sent: Friday, January 15, 2021 1:21 AM
> 
> On Thu, Jan 14, 2021 at 02:41:29PM +0100, Jan Beulich wrote:
> > On 14.01.2021 13:33, Roger Pau Monné wrote:
> > > On Thu, Jan 14, 2021 at 12:45:27PM +0100, Jan Beulich wrote:
> > >> On 14.01.2021 11:22, Roger Pau Monné wrote:
> > >>> On Wed, Jan 13, 2021 at 04:31:33PM -0500, Jason Andryuk wrote:
> > >>>> On Wed, Jan 13, 2021 at 1:34 PM Jason Andryuk
> <jandryuk@xxxxxxxxx> wrote:
> > >>>>> I guess I'd also need to disable MSI for the two devices to ensure
> > >>>>> they are both using the GSI?
> > >>>>
> > >>>> lspci in dom0 shows the USB xhci controller, iwlwifi, and e1000e
> > >>>> devices all with IRQ 16 and all with MSI disabled ("MSI: Enabled-").
> > >>>> The two linux HVMs run with (PV) linux stubdoms, and the HVM
> kernels
> > >>>> were started with pci=nosmi.  Networking through the iwlwifi device
> > >>>> works for that VM while a mouse attached to the xhci controller is
> > >>>> also working in the second VM.  Is there something else I should test?
> > >>>
> > >>> Not really, I think that test should be good enough, the issue is that
> > >>> we don't know which OS was seeing the issues noted by Kevin.
> > >>
> > >> Why a specific OS? Isn't this also guarding against malice?
> > >
> > > No, I don't think this protects against any kind of malice (at least
> > > that I can think of). It just deasserts the guest virtual line
> > > periodically if the guest itself hasn't done so. It will also attempt
> > > to EOI the physical interrupt, but that's already done by the
> > > eoi_timer in irq_guest_action_t (which would be the part that protects
> > > against malice IMO).
> >
> > Hmm, yes, there's certainly some overlap. And indeed the EOI
> > timer is set 1ms in the future, while the timer here allows
> > for 8ms to pass before taking action.
> >
> > What I'm uncertain about is the interaction between both: It
> > would seem to me that the pirq_guest_eoi() invocation from
> > here could undermine the purpose of the EOI timer. In which
> > case it would in fact be a win to get rid of this timer here.
> 
> It's not clear to me either. In any case having two timers for the
> same irq also seems like a waste of resources.
> 
> > > It's my understanding that according to what Kevin pointed out this
> > > was done because when sharing a pirq amongst different guests a guest
> > > can get interrupts delivered before it has properly setup the device,
> > > and not deasserting those by Xen would get the guest into some kind of
> > > stuck state, where it's not deasserting the line for itself.
> > >
> > > TBH I'm still trying to figure out how that scenario would look like,
> > > and why would just deasserting the line fix it. On the vIO-APIC case
> > > you would need to forcefully clean the IRR bit in order to receive
> > > further interrupts on that pin, so maybe the issue is that switching a
> > > vIO-APIC pin from level to trigger mode (which clears the IRR bit)
> > > should also deassert the line?
> >
> > I suppose this was directed at Kevin - I'm struggling as well.
> 
> Right, was a question for anyone who might know the answer really. I
> think I will prepare some more stuff to try to clean this up. Let's
> see if Kevin has some input.
> 

Honestly speaking I don't have a good memory of this old stuff and
also cannot figure out a reasonable scenario at this moment. I think
we could just go cleaning it up and see what might jump out later.

Thanks
Kevin