Re: Re: Losing PS/2 Interrupts

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

>>> On 24.05.11 at 15:52, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Tue, 24 May 2011, Jan Beulich wrote:
>> > 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).
>> 
>> What's so wrong with pirq_eoi_map that you're trying to avoid it
>> by all means?
>  
> The main issue is that if pirq_eoi_map is enabled PHYSDEVOP_eoi
> automatically unmask the event channel.
> There isn't even a way to specify if we want the unmask to be done or
> not, it just does it.

I can't think of situations where this would be a problem. It certainly
never has been in our kernels.

> I also think that it is a violation of the interface, see this comment
> from xen/include/public/xen.h:
> 
>      * Event channels are addressed by a "port index". Each channel is
>      * associated with two bits of information:
>      *  1. PENDING -- notifies the domain that there is a pending notification
>      *     to be processed. This bit is cleared by the guest.
>      *  2. MASK -- if this bit is clear then a 0->1 transition of PENDING
>      *     will cause an asynchronous upcall to be scheduled. This bit is only
> -->  *     updated by the guest. It is read-only within Xen. If a channel

Yeah, that should have been updated when the new feature got
introduced. But I'm sure you know how things go wrt documentation
(especially when a comment like this sits far away from any code
touched during the implementation of something new)...

Anyway - if a kernel is using the new feature, it clearly ought to be
aware that the bitmap then no longer is read-only to the hypervisor.

Jan

>      *     becomes pending while the channel is masked then the 'edge' is lost
>      *     (i.e., when the channel is unmasked, the guest must manually handle
>      *     pending notifications as no upcall will be scheduled by Xen).