Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()

[Julien Grall <julien.grall@linaro.org>]

(+ Xen ARM maintainers)

Hello Oleksandr,

Thanks for the patch. For next time, can you add the Xen ARM maintainers
in cc? With the amount of mail in the mailing list, your mail could be
lost easily. :)

On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
> The possible deadlock scenario is explained below:
> 
> non interrupt context:    interrupt contex        interrupt context
>                           (CPU0):                 (CPU1):
> vgic_distr_mmio_write()   do_trap_irq()           do_softirq()
>   |                         |                       |
>   vgic_disable_irqs()       ...                     ...
>     |                         |                       |
>     gic_remove_from_queues()  vgic_vcpu_inject_irq()  vgic_vcpu_inject_irq()
>     |  ...                      |                       |
>     |  spin_lock(...)           gic_set_guest_irq()     gic_set_guest_irq()
>     |  ...                        ...                     ...
>     |  ... <----------------.---- spin_lock_irqsave(...)  ...
>     |  ... <----------------.-.---------------------------spin_lock_irqsave(...)
>     |  ...                  . .       Oops! The lock has already taken.
>     |  spin_unlock(...)     . .
>     |  ...                  . .
>     gic_irq_disable()       . .
>        ...                  . .
>        spin_lock(...)       . .
>        ...                  . .
>        ... <----------------. .
>        ... <------------------.
>        ...
>        spin_unlock(...)
> 
> Since the gic_remove_from_queues() and gic_irq_disable() called from
> non interrupt context and they acquire the same lock as gic_set_guest_irq()
> which called from interrupt context we must disable interrupts in these
> functions to avoid possible deadlocks.
> 
> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

I think this patch should have a release exception for Xen 4.4. It's fix
a race condition in the interrupt management.

> ---
>  xen/arch/arm/gic.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index c44a4d0..7d83b0c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
>  static void gic_irq_disable(struct irq_desc *desc)
>  {
>      int irq = desc->irq;
> +    unsigned long flags;
>  
> -    spin_lock(&desc->lock);
> +    spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
>      /* Disable routing */
>      GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>      desc->status |= IRQ_DISABLED;
>      spin_unlock(&gic.lock);
> -    spin_unlock(&desc->lock);
> +    spin_unlock_irqrestore(&desc->lock, flags);
>  }
>  
>  static unsigned int gic_irq_startup(struct irq_desc *desc)
> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
>  void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>  {
>      struct pending_irq *p = irq_to_pending(v, virtual_irq);
> +    unsigned long flags;
>  
> -    spin_lock(&gic.lock);
> +    spin_lock_irqsave(&gic.lock, flags);
>      if ( !list_empty(&p->lr_queue) )
>          list_del_init(&p->lr_queue);
> -    spin_unlock(&gic.lock);
> +    spin_unlock_irqrestore(&gic.lock, flags);
>  }
>  
>  void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> 


-- 
Julien Grall