From patchwork Mon May 30 13:16:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 9141397 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1E1A160759 for ; Mon, 30 May 2016 13:19:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0FD5628185 for ; Mon, 30 May 2016 13:19:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 00BFD281C2; Mon, 30 May 2016 13:19:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 59FAB28185 for ; Mon, 30 May 2016 13:19:20 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b7N4A-0003PJ-4R; Mon, 30 May 2016 13:17:10 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b7N48-0003PC-Dy for xen-devel@lists.xensource.com; Mon, 30 May 2016 13:17:08 +0000 Received: from [193.109.254.147] by server-5.bemta-14.messagelabs.com id 2F/53-14748-35D3C475; Mon, 30 May 2016 13:17:07 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrIIsWRWlGSWpSXmKPExsVybKJsh26wrU+ 4wZY/3Bb3prxnd2D02N63iz2AMYo1My8pvyKBNeNg02GWgvM6FVc2nWFpYJyo3MXIySEkMJVR 4u1//y5GLiC7nUniwLyd7CAJFgFtiasLGlhBbDYBQ4m/TzaxdTFycEgA2Us+c4CERQTUJf5cm MAI0ssscJdJYuWMM2D1wgK+Emu/9LGB2JxARc+7tjCC2LwCXhJ7fy1kglicJPGltxWsRlRAV+ LQvz9sEDWCEidnPmEBsZkFtCSWT98GZksIZEjM65nDCmF7SSy6cQnKVpO4em4T8wRGwVlI2mc haV/AyLSKUb04tagstUjXSC+pKDM9oyQ3MTNH19DQRC83tbg4MT01JzGpWC85P3cTIzA4GYBg B2PLHOdDjJIcTEqivJN+eocL8SXlp1RmJBZnxBeV5qQWH2KU4eBQkuC1tPEJFxIsSk1PrUjLz AHGCUxagoNHSYQ3CSTNW1yQmFucmQ6ROsWoy7FlwY21TEIsefl5qVLivKUgRQIgRRmleXAjYD F7iVFWSpiXEegoIZ6C1KLczBJU+VeM4hyMSsK8tiBTeDLzSuA2vQI6ggnoCLNzXiBHlCQipKQ aGBvZ6wq5qrSfr1vr/36nYfbqEwkP4qTWPHvCd92Ld+LE18dcJPQPHPp593HdVpn+xJ/cy0qr d52bZrxqS5Rl2rwXeVO8rpzwE4oWcC489LDj97/bqbVSibU3yzKV5l7MeDhL6/J6ncVcn/7Ni b+0Y3liuNMaIcmwu4c33itexc7xkqeIseC60zUlluKMREMt5qLiRAAOV6pY1AIAAA== X-Env-Sender: sstabellini@kernel.org X-Msg-Ref: server-5.tower-27.messagelabs.com!1464614225!44667226!1 X-Originating-IP: [198.145.29.136] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 8.46; banners=-,-,- X-VirusChecked: Checked Received: (qmail 46295 invoked from network); 30 May 2016 13:17:06 -0000 Received: from mail.kernel.org (HELO mail.kernel.org) (198.145.29.136) by server-5.tower-27.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 30 May 2016 13:17:06 -0000 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 42CA92022D; Mon, 30 May 2016 13:17:03 +0000 (UTC) Received: from [10.0.0.5] (23.105.115.87.dyn.plus.net [87.115.105.23]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5ABB1201C8; Mon, 30 May 2016 13:17:00 +0000 (UTC) Date: Mon, 30 May 2016 14:16:56 +0100 (BST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Julien Grall In-Reply-To: <57485203.3030407@arm.com> Message-ID: References: <1464309582-22637-1-git-send-email-shankerd@codeaurora.org> <57485203.3030407@arm.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 X-Virus-Scanned: ClamAV using ClamSMTP Cc: Philip Elcan , Wei Chen , Vikram Sethi , Steve Capper , xen-devel , Stefano Stabellini , Shanker Donthineni , Shannon Zhao Subject: Re: [Xen-devel] [PATCH] arm/acpi: Fix the deadlock in function vgic_lock_rank() X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Fri, 27 May 2016, Julien Grall wrote: > Hello Shanker, > > On 27/05/16 01:39, Shanker Donthineni wrote: > > Commit 9d77b3c01d1261c (Configure SPI interrupt type and route to > > Dom0 dynamically) causing dead loop inside the spinlock function. > > Note that spinlocks in XEN are not recursive. Re-acquiring a spinlock > > that has already held by calling CPU leads to deadlock. This happens > > whenever dom0 does writes to GICD regs ISENABLER/ICENABLER. > > Thank you for spotting it, I have not noticed it while I was reviewing, only > tested on a model without any SPIs. > > > The following call trace explains the problem. > > > > DOM0 writes GICD_ISENABLER/GICD_ICENABLER > > vgic_v3_distr_common_mmio_write() > > vgic_lock_rank() --> acquiring first time > > vgic_enable_irqs() > > route_irq_to_guest() > > gic_route_irq_to_guest() > > vgic_get_target_vcpu() > > vgic_lock_rank() --> attemping acquired lock > > > > The simple fix release spinlock before calling vgic_enable_irqs() > > and vgic_disable_irqs(). > > You should explain why you think it is valid to release the lock earlier. > > In this case, I think the fix is not correct because the lock is protecting > both the register value and the internal state in Xen (modified by > vgic_enable_irqs). By releasing the lock earlier, they may become inconsistent > if another vCPU is disabling the IRQs at the same time. I agree, the vgic_enable_irqs call need to stay within the vgic_lock_rank/vgic_unlock_rank region. > I cannot find an easy fix which does not involve release the lock. When I was > reviewing this patch, I suggested to split the IRQ configuration from the > routing. Yes, the routing doesn't need to be done from vgic_enable_irqs. It is not nice. That would be the ideal fix, but it is not trivial. For 4.7 we could consider reverting 9d77b3c01d1261c. The only other thing that I can come up with which is simple would be improving gic_route_irq_to_guest to cope with callers that have the vgic rank lock already held (see below, untested) but it's pretty ugly. diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 2bfe4de..57f3f3f 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -127,15 +127,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, /* Program the GIC to route an interrupt to a guest * - desc.lock must be held + * - rank lock must be held */ -int gic_route_irq_to_guest(struct domain *d, unsigned int virq, +static int __gic_route_irq_to_guest(struct domain *d, unsigned int virq, struct irq_desc *desc, unsigned int priority) { - unsigned long flags; - /* Use vcpu0 to retrieve the pending_irq struct. Given that we only - * route SPIs to guests, it doesn't make any difference. */ - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); + struct vcpu *v_target = __vgic_get_target_vcpu(d->vcpu[0], virq); struct pending_irq *p = irq_to_pending(v_target, virq); int res = -EBUSY; @@ -144,12 +141,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, ASSERT(virq >= 32); ASSERT(virq < vgic_num_irqs(d)); - vgic_lock_rank(v_target, rank, flags); - if ( p->desc || /* The VIRQ should not be already enabled by the guest */ test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) - goto out; + return res; desc->handler = gic_hw_ops->gic_guest_irq_type; set_bit(_IRQ_GUEST, &desc->status); @@ -159,12 +154,36 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, p->desc = desc; res = 0; -out: - vgic_unlock_rank(v_target, rank, flags); - return res; } +int gic_route_irq_to_guest(struct domain *d, unsigned int virq, + struct irq_desc *desc, unsigned int priority) +{ + unsigned long flags; + int lock = 0, retval; + struct vgic_irq_rank *rank; + + /* Use vcpu0 to retrieve the pending_irq struct. Given that we only + * route SPIs to guests, it doesn't make any difference. */ + rank = vgic_rank_irq(d->vcpu[0], virq); + + /* Take the rank spinlock unless it has already been taken by the + * caller. */ + if ( !spin_is_locked(&rank->lock) ) { + vgic_lock_rank(d->vcpu[0], rank, flags); + lock = 1; + } + + retval = __gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ); + + if ( lock ) + vgic_unlock_rank(d->vcpu[0], rank, flags); + + return retval; + +} + /* This function only works with SPIs for now */ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, struct irq_desc *desc) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index aa420bb..e45669f 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -215,7 +215,7 @@ int vcpu_vgic_free(struct vcpu *v) } /* The function should be called by rank lock taken. */ -static struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) +struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) { struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index a2fccc0..726e690 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -343,6 +343,7 @@ void vgic_v3_setup_hw(paddr_t dbase, const struct rdist_region *regions, uint32_t rdist_stride); #endif +struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq); #endif /* __ASM_ARM_VGIC_H__ */