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

From: Julien Grall <julien.grall@arm.com>
To: Andrii Anisov <andrii.anisov@gmail.com>, xen-devel@lists.xenproject.org
Cc: Andre Przywara <andre.przywara@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Andrii Anisov <andrii_anisov@epam.com>
Subject: Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing
Date: Fri, 31 May 2019 18:11:31 +0100	[thread overview]
Message-ID: <4d27c123-826d-ad07-ed0a-e7a7517fca62@arm.com> (raw)
In-Reply-To: <c0de5da0-9986-ea97-b7d3-6902bd789c19@gmail.com>

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}.

[ Details to be added once you give more ground ]

Cheers,

-- 
Julien Grall

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