Re: [Xen-devel] [PATCH 2/2] arm/irq: skip action avalability check for non-debug build

[Julien Grall <julien.grall@xxxxxxx>]

Title: s/avalability/availability/

On 12/12/2018 16:55, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> An IRQ with _IRQ_GUEST flag set always has an action.
> An IRQ with _IRQ_DISABLED flag cleared always have an action.
s/have/has/

Those conditions are not sufficient to ensure desc->action is not NULL. You also 
need to take the spinlock.
While looking at the code, I noticed an interesting race with the release code. 
Guest IRQ are released using the function gic_remove_irq_to_guest. The sequence 
is roughly:
1) spin_lock(desc->lock);
2) writel(desc->irq, ICENABLER);
3) set_bit(_IRQ_DISABLED, &desc->status);
4) clear_bit(_IRQ_GUES, &desc->status);
5) desc->handler = &no_irq_type;
6) spin_unlock(desc->lock);

Even if 2) will disable the interrupt in the hardware, the interrupt may have 
been received earlier on another CPU and waiting on the lock. As soon as the 
lock is taken, the code will notice the irq disabled (thanks to 3)) and will 
then end the interrupt. The callbak end for no_irq_type is a NOP, therefore the 
interrupt will stay active and the priority will not be dropped.
Because of that, the CPU will never be able to receive interrupt for guest 
anymore. AFAICT, this can only happen if an interrupt is received while 
destroying the assigned domain.
I think 5) should be replaced with

desc->handler = gic_hw_ops->gic_host_irq_type;

Or we potentially need to update no_irq_type and EOI "spurious interrupt".

I am not entirely sure which way is the best to address the race. Any opinions?


> Those flags checks cover all accesses to desc->action in do_IRQ, > so we can skip 
> desc->action check.
"in non-debug build".

> Still keep it in place for debug build.
"Keep in place for debug build to help diagnostics potential misconfiguration".

> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
Please don't add a reviewed-by tag until it was explicitly written by the 
reviewer.

> ---
>   xen/arch/arm/irq.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d6a0273..4a02cc1 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -209,12 +209,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
> int is_fiq)
>       spin_lock(&desc->lock);
>       desc->handler->ack(desc);
>   
> +#ifndef NDEBUG
>       if ( !desc->action )
>       {
>           printk("Unknown %s %#3.3x\n",
>                  is_fiq ? "FIQ" : "IRQ", irq);
>           goto out;
>       }
> +#endif
>   
>       if ( test_bit(_IRQ_GUEST, &desc->status) )
>       {
Cheers,

--
Julien Grall

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