Re: Re: Losing PS/2 Interrupts

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

>>> On 24.05.11 at 14:58, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, May 24, 2011 at 01:24:24PM +0100, Jan Beulich wrote:
>> >>> On 24.05.11 at 13:04, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> wrote:
>> > On Tue, 24 May 2011, Jan Beulich wrote:
>> >> > Relevant code snippets included below:
>> >> > 
>> >> >         if (pirq_needs_eoi(irq)) {
>> >> >                 printk(KERN_ERR "%s: irq %d handle_fasteoi_irq\n", 
>> >> > __FUNCTION__, irq);
>> >> >                 set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
>> >> >                                 handle_fasteoi_irq, name);
>> >> >         } else {
>> >> >                 printk(KERN_ERR "%s: irq %d handle_edge_irq\n", 
>> >> > __FUNCTION__, irq);
>> >> >                 set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
>> >> >                                 handle_edge_irq, name);
>> >> >         }
>> >> 
>> >> Now this, imo, is a very good reason to not use handle_edge_irq()
>> >> at all, and instead use the prior control flow (masking and clearing
>> >> the event channel up front in do_upcall()) with only fasteoi (leaving
>> >> aside per-CPU ones).
>> >> 
>> > 
>> > Actually I think it is a good reason to fix pirq_needs_eoi that shouldn't
>> > return unconditionally yes if dom0 doesn't support pirq_eoi_map.
>> > The comment in Xen 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.
>> >      */
>> > 
>> > Considering that I would rather avoid supporting pirq_eoi_map and we are
>> > talking about edge triggered interrupts, do you think it would be safe
>> > for me to send a patch to xen to change this behaviour?
>> > Shouldn't we set XENIRQSTAT_needs_eoi only for level triggered
>> > interrupts (and maybe maskable MSI sources)?
>> 
>> Only if you can prove that the very first part of that comment is
>> incorrect (in including "edge-triggered" and ignoring whether MSI
>> sources are maskable). And your Linux side code would then still
>> be incorrect for maskable MSIs (you'd continue to handle them
>> as fasteoi with no up front clearing/masking while that is necessary
>> as Thomas' report made clear).
> 
> I believe we handle MSI's as 'handle_edge_chip' (xen_bind_pirq_msi_to_irq)
> irregardless of the above hypercall.

So how do you issue the possibly necessary EOI call then?

> You wouldn't have a nice chart of the right type of events to
> do for different types of interrupts, would you?

No, sorry - all I usually look at are the three more or less different
implementations.

Jan

>> 
>> What's so wrong with pirq_eoi_map that you're trying to avoid it
>> by all means?
> 
> My recollection is that we had a hard time trying to work it in with the
> tglr'x rewrite of the IRQ code and not enough understanding of this (at 
> least
> on my side). Any help here would be appreciated.