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

[Julien Grall <julien.grall@xxxxxxx>]

(+ Andre)

Hi,

Title: Interrupts are still unmasked when executing action for interrupt 
routed to Xen. So you need to be more specific. How about
"xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

On 5/27/19 10:29 AM, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> This reduces the number of context switches in case we have coming guest
> interrupts from different sources at a high rate. What is likely for
s/What/This/

> multimedia use-cases.
> Having irqs unlocked here makes us go through trap path again in case we
what do you mean by "unlocked"?

> have a new guest interrupt arrived (even with the same priority, after
> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
> cycles wasting.
after `desc->....`. This is just a waste a processor cycle as we will 
catch them all in the function gic_interrupt() loop.
 We will catch them all in the `gic_interrupt() function
> loop anyway. And the guest irqs arrival prioritization is meaningless
> here, it is only effective at guest's level.
I am not sure why you speak about guest prioritization here. The main 
issue would be an interrupt to Xen (i.e timer) that would get delayed 
because of longer period without interrupt enabled. I would also not 
rule out the possibility to prioritize guest interrupt at hardware level.
I know we have been discussing on the problem in the past, but a summary 
in the commit message is quite important to not miss out all the problems.
The real problem here is for interrupt routed to guest the interrupt 
will be kept unmasked when calling desc->handler->end(desc). This will 
result to receive the next interrupt as soon as desc->handler->end(desc) 
is called.
In the case of interrupt routed to Xen, interrupts will be kept enabled 
while executing the action but then disabled before calling 
desc->handler->end(desc).
It would be fine to keep the interrupts masked for interrupts routed to 
the guest because vgic_inject_irq(...) will be masking the interrupt in 
most of the cases.
The code below looks good to me. I am happy to help rewording the commit 
message if necessary.
While looking at the code, I noticed that in the new vgic vgic_get_irq() 
looks unsafe to be called with interrupt unmasked. This is because one 
of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. 
Andre, what do you think?
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>
> Changes:
>
>      in v2: Drop irq enabling for lpi processing as well.
>
>
> ---
>   xen/arch/arm/gic.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..113655a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>   
>           if ( likely(irq >= 16 && irq < 1020) )
>           {
> -            local_irq_enable();
>               isb();
>               do_IRQ(regs, irq, is_fiq);
> -            local_irq_disable();
>           }
>           else if ( is_lpi(irq) )
>           {
> -            local_irq_enable();
>               isb();
>               gic_hw_ops->do_LPI(irq);
> -            local_irq_disable();
>           }
>           else if ( unlikely(irq < 16) )
>           {
Cheers,

--
Julien Grall

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