[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] Re: Losing PS/2 Interrupts
>>> On 24.05.11 at 15:52, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 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.
> * 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).
Xen-devel mailing list