From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq() Date: Wed, 5 Feb 2014 14:38:48 +0000 Message-ID: <52F24CF8.5070500@eu.citrix.com> References: <1391448828-28414-1-git-send-email-oleksandr.tyshchenko@globallogic.com> <52EFD711.5060201@linaro.org> <52F10CD4.1050000@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52F10CD4.1050000@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , Oleksandr Tyshchenko Cc: Stefano Stabellini , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 02/04/2014 03:52 PM, Julien Grall wrote: > Forget to add George. > > On 02/03/2014 05:51 PM, Julien Grall wrote: >> (+ 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 >> Acked-by: Julien Grall >> >> I think this patch should have a release exception for Xen 4.4. It's fix >> a race condition in the interrupt management. Release-acked-by: George Dunlap >> >>> --- >>> 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, >>> >> >