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

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

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.

Thanks, Roger.