Re: Using handle_fasteoi_irq for pirqs

[Jeremy Fitzhardinge <jeremy@goop.org>]

 On 09/07/2010 04:58 PM, Jan Beulich wrote:
>> Yes, but there's also PHYSDEVOP_irq_status_query call.  Does the "needs
>> EOI" actually change from moment to moment in a way where having a
>> shared page makes sense?
> No, it doesn't - using this function you can of course maintain the
> bitmap on your own (which we also fall back to if PHYSDEVOP_pirq_eoi_gmfn
> is not supported by the hypervisor). The actual advantage of using
> PHYSDEVOP_pirq_eoi_gmfn is that it implies an unmask of the
> corresponding event channel - see
> http://xenbits.xensource.com/xen-unstable.hg?rev/c820bf73a914.

Yes, though it seems like an odd way of implementing it.  The shared
memory region for EOI flags looks like overkill when all it saves is one
hypercall on pirq setup, and making that also have the side-effect of
changing an existing hypercall's behaviour is surprising.  Too late now,
I suppose, but it would seem that just adding a new PHYSDEVOP_eoi_unmask
hypercall would have been a simpler approach.  (But unmask typically
doesn't need a hypercall anyway, unless you've managed to get into a
cross-cpu unmask situation, so it doesn't seem like a big saving?)

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?

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?

> 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.

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

Thanks,
    J