This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] Re: Using handle_fasteoi_irq for pirqs

To: "Jeremy Fitzhardinge" <jeremy@xxxxxxxx>
Subject: [Xen-devel] Re: Using handle_fasteoi_irq for pirqs
From: "Jan Beulich" <JBeulich@xxxxxxxxxx>
Date: Mon, 06 Sep 2010 08:58:48 +0100
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, "Xen-devel@xxxxxxxxxxxxxxxxxxx" <Xen-devel@xxxxxxxxxxxxxxxxxxx>, Tom Kopec <tek@xxxxxxx>, Daniel Stodden <daniel.stodden@xxxxxxxxxx>
Delivery-date: Mon, 06 Sep 2010 01:00:29 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C814278.5070904@xxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C743B2C.8070208@xxxxxxxx> <4C74E7C802000078000120C0@xxxxxxxxxxxxxxxxxx> <4C814278.5070904@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
 >>> On 03.09.10 at 20:46, Jeremy Fitzhardinge <jeremy@xxxxxxxx> wrote:
> Using .startup/.shutdown for enable/disable seems very heavyweight.  Do
> we really want to be rebinding the pirq each time?  Isn't unmask/masking
> the event channel sufficient?

Depends - the original (2.6.18) implementation only makes enable_pirq()
a conditional startup (and really startup_pirq() is conditional too), while
disable_pirq() does nothing at all. While forward porting, considering
the contexts in which ->disable() gets called (namely note_interrupt())
and after initially having had no ->enable()/->disable() methods at all
(for default_enable() calling ->unmask() anyway, and default_disable()
being a no-op as much as disable_pirq() was), I got to the conclusion
that it might be better to do a full shutdown/startup, since it isn't
known whether a disable is permanent or just temporary.

Now part of the question whether this is actually a good choice is
why default_disable() doesn't mask the affected IRQ - likely because
IRQ_DISABLED is checked for and handled accordingly in all non-
trivial flow handlers.

The other aspect is that with the (original) switch to use
handle_level_irq() we noticed at some point that the disabling of
bad IRQs (where e.g. storms are noticed) didn't work anymore,
due to that logic sitting in ->end(), which doesn't usually get
called at all (nor does any other method except ->unmask() for
the level case). Right now I don't really remember whether making
->disable() an alias of shutdown wasn't just a (failed iirc) attempt
at getting Xen to know of the need to shut down such a bad IRQ.
After the switch to fasteoi this logic should now be working again
independent of what ->disable() does, so I will have to consider
to un-alias disable_pirq() and shutdown_pirq() again.

> At the moment my xen_evtchn_do_upcall() is masking and clearing the
> event channel before calling into generic_handle_irq_desc(), which will
> call handle_fasteoi_irq fairly directly.  That runs straight through and
> the priq_chip's eoi just does an EOI on the pirq if Xen says it needs one.
> But apparently this isn't enough.  Is there anything else I should be doing?

I can't see anything, and our kernel also doesn't.

> (I just implemented PHYSDEVOP_pirq_eoi_gmfn method of getting the pirq
> eoi flags, but I haven't tested it yet.  I'm also not really sure what
> the advantage of it is.)

This is for you to avoid the EOI hypercall if it would be a no-op in
Xen anyway.


Xen-devel mailing list