From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: Using handle_fasteoi_irq for pirqs Date: Tue, 07 Sep 2010 09:58:40 +0100 Message-ID: <4C861AE00200007800014A67@vpn.id2.novell.com> References: <4C743B2C.8070208@goop.org> <4C74E7C802000078000120C0@vpn.id2.novell.com> <4C814278.5070904@goop.org> <4C84BB580200007800014717@vpn.id2.novell.com> <4C859B27.6000400@goop.org> <4C85FED10200007800014A0C@vpn.id2.novell.com> <4C85F1A0.5090308@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <4C85F1A0.5090308@goop.org> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Fitzhardinge Cc: Konrad Rzeszutek Wilk , "Xen-devel@lists.xensource.com" , Keir Fraser , Tom Kopec , Daniel Stodden List-Id: xen-devel@lists.xenproject.org >>> On 07.09.10 at 10:02, Jeremy Fitzhardinge wrote: > On 09/07/2010 04:58 PM, Jan Beulich wrote: > Also, in the restore path, the code does a BUG if it fails to call > PHYSDEVOP_pirq_eoi_gmfn again. Couldn't it just clear > pirq_eoi_does_unmask (and re-initialize all the needs-eoi flags using > PHYSDEVOP_irq_status_query) and carry on the old way? That would seem possible, yes. > But the comment in PHYSDEVOP_irq_status_query says: >=20 > /* > * 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. > */ >=20 > which suggests that the "needs EOI" status for each pirq can change > after the pirq has been initialized (or if not, why isn't using > PHYSDEVOP_irq_status_query sufficient?). OTOH, if it can change > spontaneously, then it all seems a bit racy... >=20 > IOW, what does "from time to time" mean? Just look at the places where set_pirq_eoi() gets called in xen/arch/x86/irq.c: The flag (obviously) always gets set for IRQs that aren't ACKTYPE_NONE, but there is an additional case in __do_IRQ_guest() where Xen wants the notification to re-enable the interrupt after disabling it due to all possible handler domains having it pending already. And you see that Xen would force you to always execute the EOI notification hypercall without the shared page (PHYSDEVOP_irq_status_query setting XENIRQSTAT_needs_eoi unconditionally), so using it you save a hypercall on each interrupt that doesn't need an EOI (namely MSI-X and maskable MSI; I don't think edge triggered ones are very important), and if you (as usual) don't need to unmask via hypercall, you don't need any hypercall at all in the IRQ exit path in these cases. >> Also, regarding your earlier question on .disable - I just ran across >> http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/51b2b0d0921c, >> which makes me think that .enable indeed should remain an alias of >> .startup, but I also think that .disable should nevertheless do the >> masking (i.e. be an alias of .mask) >=20 > That particular change has the strange asymmetry of making .enable do a > startup (which only does eoi if the event channel is still valid), > .disable a no-op, and .end shuts the pirq down iff the irq is pending > and disabled. I see how this works in the context of the old __do_IRQ > stuff in 2.6.18 though. And it should similarly be fine with handle_fasteoi_irq() afaict, provided .end and .eoi reference the same function. The asymmetry was part of what made me change .disable to alias .shutdown. > What's the specific deadlock mentioned in the commit comment? Is that > still an issue with Xen's auto-EOI-after-timeout behaviour? Honestly, I don't know. Keir? Jan