On 09/06/2010 05:58 PM, Jan Beulich wrote:
> >>> 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, I'm using this:
static struct irq_chip xen_pirq_chip __read_mostly = {
.name = "xen-pirq",
.startup = startup_pirq,
.shutdown = shutdown_pirq,
.enable = pirq_eoi,
.unmask = unmask_irq,
.disable = mask_irq,
.mask = mask_irq,
.eoi = ack_pirq,
.end = end_pirq,
.set_affinity = set_affinity_irq,
.retrigger = retrigger_irq,
};
which seems to work OK now. The "enable=pirq_eoi" is essentially the
same as "enable=startup_pirq", because your startup_pirq just does an
EOI if the evtchn is already valid (and EOI always ends up unmasking too).
ack_pirq and pirq_eoi are almost the same, except the former also does
the call to move_masked_irq().
>> 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.
Where's the source to your kernel? The one I looked at most recently
was using handle_level_irq for everything.
But anyway, I found my bug; I'd overlooked where MSI interrupts were
being set up, and they were still using handle_edge_irq; changing them
to fasteoi seems to have done trick.
>> (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.
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?
J
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home