Re: Using handle_fasteoi_irq for pirqs

["Jan Beulich" <JBeulich@novell.com>]

 >>> On 07.09.10 at 10:02, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> On 09/07/2010 04:58 PM, Jan Beulich wrote:
> Also, in the restore path, the code does a BUG if it fails to call
> PHYSDEVOP_pirq_eoi_gmfn again.  Couldn't it just clear
> pirq_eoi_does_unmask (and re-initialize all the needs-eoi flags using
> PHYSDEVOP_irq_status_query) and carry on the old way?

That would seem possible, yes.

> But the comment in PHYSDEVOP_irq_status_query says:
> 
>         /*
>          * Even edge-triggered or message-based IRQs can need masking from
>          * time to time. If teh guest is not dynamically checking for this
>          * via the new pirq_eoi_map mechanism, it must conservatively always
>          * execute the EOI hypercall. In practice, this only really makes a
>          * difference for maskable MSI sources, and if those are supported
>          * then dom0 is probably modern anyway.
>          */
> 
> which suggests that the "needs EOI" status for each pirq can change
> after the pirq has been initialized (or if not, why isn't using
> PHYSDEVOP_irq_status_query sufficient?).  OTOH, if it can change
> spontaneously, then it all seems a bit racy...
> 
> IOW, what does "from time to time" mean?

Just look at the places where set_pirq_eoi() gets called in
xen/arch/x86/irq.c: The flag (obviously) always gets set for IRQs
that aren't ACKTYPE_NONE, but there is an additional case in
__do_IRQ_guest() where Xen wants the notification to re-enable
the interrupt after disabling it due to all possible handler domains
having it pending already.

And you see that Xen would force you to always execute the EOI
notification hypercall without the shared page
(PHYSDEVOP_irq_status_query setting XENIRQSTAT_needs_eoi
unconditionally), so using it you save a hypercall on each interrupt
that doesn't need an EOI (namely MSI-X and maskable MSI; I don't
think edge triggered ones are very important), and if you (as usual)
don't need to unmask via hypercall, you don't need any hypercall
at all in the IRQ exit path in these cases.

>> Also, regarding your earlier question on .disable - I just ran across
>> http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/51b2b0d0921c,
>> which makes me think that .enable indeed should remain an alias of
>> .startup, but I also think that .disable should nevertheless do the
>> masking (i.e. be an alias of .mask)
> 
> That particular change has the strange asymmetry of making .enable do a
> startup (which only does eoi if the event channel is still valid),
> .disable a no-op, and .end shuts the pirq down iff the irq is pending
> and disabled.  I see how this works in the context of the old __do_IRQ
> stuff in 2.6.18 though.

And it should similarly be fine with handle_fasteoi_irq() afaict,
provided .end and .eoi reference the same function.

The asymmetry was part of what made me change .disable to
alias .shutdown.

> What's the specific deadlock mentioned in the commit comment?  Is that
> still an issue with Xen's auto-EOI-after-timeout behaviour?

Honestly, I don't know. Keir?

Jan