Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 31 May 2019, Julien Grall wrote:
> Hi Andrii,
> 
> On 30/05/2019 17:12, Andrii Anisov wrote:
> > On 29.05.19 18:32, Julien Grall wrote:
> > > It would have been nice to at least fix up the commit message with the
> > > typoes (and rewording) I mentioned in my previous e-mail.
> > > Your commit message needs to explained why this is fine to keep the
> > > interrupt masked a bit longer. I wrote the explanation in my previous
> > > e-mail so you can borrow the rationale from there.
> > xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()
> > 
> > Having irqs enabled here leaves a room for trapping and going through the
> > trap
> 
> Please avoid "here" in commit message if you haven't defined where is the
> issue.
> 
> > path again if we have a new guest interrupt arrived (even with the same or
> 
> I don't understand the "new guest interrupt arrived".
> 
> > lower priority, after `desc->handler->end(desc)` in `do_IRQ()`).
> > Keeping interrupts disabled during guest interrupts processing allows as
> 
> Missing word because "allows" and "as"?
> 
> > avoiding excessive traps (and wasting cpu cycles for trap path) while the
> > new
> > interrupts would be processed in the loop anyway. Processing guest
> > interrupts by
> > the loop should not introduce significant additional latency because
> 
> I am always worry when I see the word "should not" associated with "latency"
> because often it is actually the contrary (see the recent attempt to optimize
> the old vGIC). If you don't have number, then you should detail the rationale
> here.
> 
> The more I think about it, the more I feel it would just be best to mask the
> interrupt just before dropping the priority. But I am happy to consider this
> if you have some ground to back the approach (they should be part of the
> commit message).
> 
> > vgic_inject_irq(...) already masking the interrupts in most of the cases.
> 
> Here my take on the commit message:
> 
> gic_interrupt() was implemented using a loop to limit the cost of the trap if
> there are multiple interrupts pending.
> 
> At the moment, interrupts are unmasked by gic_interrupt() before calling
> do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its
> priority will be dropped, via desc->handler->end() called from do_irq(), with
> interrupt unmasked.
> 
> In other words:
>     - Until the priority is dropped, only higher priority interrupt can be
> received. Today, only Xen interrupts have higher priority.
>     - As soon as priority is dropped, any interrupt can be received.
> 
> This means the purpose of the loop in gic_interrupt() is defeated as all
> interrupts may get trapped earlier. To reinstate the purpose of the loop (and
> prevent the trap), interrupts should be masked when dropping the priority.
> 
> For interrupts routed to Xen, priority will always be dropped with interrupts
> masked. So the issue is not present. However, it means that we are pointless
> try to mask the interrupts.
> 
> To avoid conflicting behavior between interrupt handling, gic_interrupt() is
> now keeping interrupts masked and defer the decision to do_{LPI, IRQ}.

This is a really well written commit message, and together with the
patch it looks fine to me.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel