From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5AD97C38147 for ; Wed, 18 Jan 2023 11:55:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=VsTFLIniEvAs4uDO0EkgLvzYdUpzNLYrRey5hkZHsmc=; b=3UyEutM1rp7KPo IkqtibE8LztLxiGwpwGDked+zUJ9ej/+MGcwa2bEksnfQifPjcXQ4tqjGwieEADmaV8RVLEqlKZu5 OEQ8+AMLxPqf3pASwt4JrVC+GOrpfKvzMkO54l48vbfgQyV889Xtwhb5rgcejTro1Jp0UbaSKgUS0 NUAZVDUXPsjYvmQVA2oq/mAJo0T0xAGEeRfljZn9J/atYotLBegZAzOi0ke4op4bWOR61/3feH1RP eKAH3ktCMgi88gq7P76pmPm+pCdnL0gkPWvLO1+xw4OYbKsqN30XvUFx8KLcTsu9IoVndEqkE0Mt+ 9AHuqRe5nYzMTH8S55cw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pI71B-000cTM-5w; Wed, 18 Jan 2023 11:54:13 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pI715-000cS4-3G for linux-arm-kernel@lists.infradead.org; Wed, 18 Jan 2023 11:54:09 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 93B956179C; Wed, 18 Jan 2023 11:54:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D6EBAC433D2; Wed, 18 Jan 2023 11:54:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674042845; bh=Vi7V9pYw3dgQME5ZLQ6OTz8bl5bu0RPfdqsoC2EYPSg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=o1+302aTiqMN+NUoKj43qeP01rY9Cg/FTj9CupZ6eerEsNE0lQ5QgpiAwQh0Hl8SU 09eKVkLTtHIAUc+vDWAzBhV1cttrDizqRs4gdEsxzPvemqFZINwdOVfMHIXowjRI7c CdGy5U/aB9Rf8fbZq6/KGfQBVYBBtPv6sMtxAF94sultA2uawd9fVBVXgIhCU6HDgF nP+uwfHmDIKi1LwnvKQbkNMrqBawOqba6gZRhp7yTVuEt43sYPlnlShYB+3Yo5bXcI 9fWR6SpI+s2qPmul15ygnVYJCjlapQH/QBkJ1pFJXTj44V6QUmU8Y1rSkO+ZWZsGev WmGjqfz6M2VYQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1pI710-002kME-DI; Wed, 18 Jan 2023 11:54:02 +0000 Date: Wed, 18 Jan 2023 11:54:02 +0000 Message-ID: <863588njmt.wl-maz@kernel.org> From: Marc Zyngier To: Shanker Donthineni Cc: James Morse , Catalin Marinas , Will Deacon , , , , , Vikram Sethi , Zenghui Yu , Oliver Upton , Suzuki K Poulose Subject: Re: [PATCH] KVM: arm64: vgic: Fix soft lockup during VM teardown In-Reply-To: <20230118022348.4137094-1-sdonthineni@nvidia.com> References: <20230118022348.4137094-1-sdonthineni@nvidia.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) X-TUID: 1Q6WKep95Q7I MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: sdonthineni@nvidia.com, james.morse@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, vsethi@nvidia.com, yuzenghui@huawei.com, oliver.upton@linux.dev, suzuki.poulose@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230118_035407_263185_F945D7E5 X-CRM114-Status: GOOD ( 41.77 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Shanker, Please Cc all the KVM/arm64 reviewers when sending KVM/arm64 patches. On Wed, 18 Jan 2023 02:23:48 +0000, Shanker Donthineni wrote: > > Getting intermittent CPU soft lockups during the virtual machines > teardown on a system with GICv4 features enabled. The function > __synchronize_hardirq() has been waiting for IRQD_IRQ_INPROGRESS > to be cleared forever as per the current implementation. > > CPU stuck here for a long time leads to soft lockup: > while (irqd_irq_inprogress(&desc->irq_data)) > cpu_relax(); Is it a soft-lockup from which the system recovers? or a livelock which leaves the system dead? What kernel version is that? Please provide all the relevant context that could help analysing the issue. > > Call trace from the lockup CPU: > [ 87.238866] watchdog: BUG: soft lockup - CPU#37 stuck for 23s! > [ 87.250025] CPU: 37 PID: 1031 Comm: qemu-system-aarch64 > [ 87.358397] Call trace: > [ 87.360891] __synchronize_hardirq+0x48/0x140 > [ 87.365343] free_irq+0x138/0x424 > [ 87.368727] vgic_v4_teardown+0xa4/0xe0 > [ 87.372649] __kvm_vgic_destroy+0x18c/0x194 > [ 87.376922] kvm_vgic_destroy+0x28/0x3c > [ 87.380839] kvm_arch_destroy_vm+0x24/0x44 > [ 87.385024] kvm_destroy_vm+0x158/0x2c4 > [ 87.388943] kvm_vm_release+0x6c/0x98 > [ 87.392681] __fput+0x70/0x220 > [ 87.395800] ____fput+0x10/0x20 > [ 87.399005] task_work_run+0xb4/0x23c > [ 87.402746] do_exit+0x2bc/0x8a4 > [ 87.406042] do_group_exit+0x34/0xb0 > [ 87.409693] get_signal+0x878/0x8a0 > [ 87.413254] do_notify_resume+0x138/0x1530 > [ 87.417440] el0_svc+0xdc/0xf0 > [ 87.420559] el0t_64_sync_handler+0xf0/0x11c > [ 87.424919] el0t_64_sync+0x18c/0x190 > > The state of the IRQD_IRQ_INPROGRESS information is lost inside > irq_domain_activate_irq() which happens before calling free_irq(). > Instrumented the code and confirmed, the IRQD state is changed from > 0x10401400 to 0x10441600 instead of 0x10401600 causing problem. > > Call trace from irqd_set_activated(): > [ 78.983544] irqd_set_activated: lost IRQD_IRQ_INPROGRESS > old=0x10401400, new=0x10441600 > [ 78.992093] CPU: 19 PID: 1511 Comm: qemu-system-aarch64 > [ 79.008461] Call trace: > [ 79.010956] dump_backtrace.part.0+0xc8/0xe0 > [ 79.015328] show_stack+0x18/0x54 > [ 79.018713] dump_stack_lvl+0x64/0x7c > [ 79.022459] dump_stack+0x18/0x30 > [ 79.025842] irq_domain_activate_irq+0x88/0x94 > [ 79.030385] vgic_v3_save_pending_tables+0x260/0x29c > [ 79.035463] vgic_set_common_attr+0xac/0x23c > [ 79.039826] vgic_v3_set_attr+0x48/0x60 > [ 79.043742] kvm_device_ioctl+0x120/0x19c > [ 79.047840] __arm64_sys_ioctl+0x42c/0xe00 > [ 79.052027] invoke_syscall.constprop.0+0x50/0xe0 > [ 79.056835] do_el0_svc+0x58/0x180 > [ 79.060308] el0_svc+0x38/0xf0 > [ 79.063425] el0t_64_sync_handler+0xf0/0x11c > [ 79.067785] el0t_64_sync+0x18c/0x190 Are these two traces an indication of concurrent events? Or are they far apart? > > irqreturn_t handle_irq_event(struct irq_desc *desc) > { > irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS); > raw_spin_unlock(&desc->lock); > > ret = handle_irq_event_percpu(desc); > > raw_spin_lock(&desc->lock); > irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); > } How is that relevant to this trace? Do you see this function running concurrently with the teardown? If it matters here, it must be a VPE doorbell, right? But you claim that this is on a GICv4 platform, while this would only affect GICv4.1... Or are you using GICv4.1? > > In this particular failed case and based on traces, the two functions > irqd_set_activated() and handle_irq_event() are concurrently modifying > IRQD state without both holding desc->lock. The irqd_set_activated() > execution path is reading memory 'state_use_accessors' in between set > & clear of IRQD_IRQ_INPROGRESS state change and writing the modified > data after executing 'irqd_clear(desc->irq_data, IRQD_IRQ_INPROGRESS)'. > > To fix the lockup issue, hold desc->lock when calling functions > irq_domain_activate_irq() and irq_domain_deactivate_irq). For that particular issue, the main problem is that we are abusing the interrupt startup/teardown code. The locking is only a consequence of this. > > Signed-off-by: Shanker Donthineni > --- > arch/arm64/kvm/vgic/vgic-v3.c | 6 ++++++ > arch/arm64/kvm/vgic/vgic-v4.c | 4 ++++ > 2 files changed, 10 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > index 2074521d4a8c..e6aa909fcbe2 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -353,22 +353,28 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) > static void unmap_all_vpes(struct vgic_dist *dist) > { > struct irq_desc *desc; > + unsigned long flags; > int i; > > for (i = 0; i < dist->its_vm.nr_vpes; i++) { > desc = irq_to_desc(dist->its_vm.vpes[i]->irq); > + raw_spin_lock_irqsave(&desc->lock, flags); > irq_domain_deactivate_irq(irq_desc_get_irq_data(desc)); > + raw_spin_unlock_irqrestore(&desc->lock, flags); I guess this is the guilty one, based on your analysis, and assuming this is a v4.1 issue, not v4. > } > } > > static void map_all_vpes(struct vgic_dist *dist) > { > struct irq_desc *desc; > + unsigned long flags; > int i; > > for (i = 0; i < dist->its_vm.nr_vpes; i++) { > desc = irq_to_desc(dist->its_vm.vpes[i]->irq); > + raw_spin_lock_irqsave(&desc->lock, flags); > irq_domain_activate_irq(irq_desc_get_irq_data(desc), false); > + raw_spin_unlock_irqrestore(&desc->lock, flags); > } > } > > diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c > index ad06ba6c9b00..a01b8313e82c 100644 > --- a/arch/arm64/kvm/vgic/vgic-v4.c > +++ b/arch/arm64/kvm/vgic/vgic-v4.c > @@ -139,8 +139,10 @@ static void vgic_v4_enable_vsgis(struct kvm_vcpu *vcpu) > /* Transfer the full irq state to the vPE */ > vgic_v4_sync_sgi_config(vpe, irq); > desc = irq_to_desc(irq->host_irq); > + raw_spin_lock(&desc->lock); > ret = irq_domain_activate_irq(irq_desc_get_irq_data(desc), > false); > + raw_spin_unlock(&desc->lock); This one looks wrong. The interrupt never fires on the host (that's the whole point of this stuff). There isn't even a handler attached to it. How can that result in a problem? My take on the whole thing is that we should stop playing games with the underlying interrupt infrastructure. How about the following hack, which is only compile-tested. Let me know if that helps. M. diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 2074521d4a8c..2624963cb95b 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -350,26 +350,23 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq) * The deactivation of the doorbell interrupt will trigger the * unmapping of the associated vPE. */ -static void unmap_all_vpes(struct vgic_dist *dist) +static void unmap_all_vpes(struct kvm *kvm) { - struct irq_desc *desc; + struct vgic_dist *dist = &kvm->arch.vgic; int i; - for (i = 0; i < dist->its_vm.nr_vpes; i++) { - desc = irq_to_desc(dist->its_vm.vpes[i]->irq); - irq_domain_deactivate_irq(irq_desc_get_irq_data(desc)); - } + for (i = 0; i < dist->its_vm.nr_vpes; i++) + free_irq(dist->its_vm.vpes[i]->irq, kvm_get_vcpu(kvm, i)); } -static void map_all_vpes(struct vgic_dist *dist) +static void map_all_vpes(struct kvm *kvm) { - struct irq_desc *desc; + struct vgic_dist *dist = &kvm->arch.vgic; int i; - for (i = 0; i < dist->its_vm.nr_vpes; i++) { - desc = irq_to_desc(dist->its_vm.vpes[i]->irq); - irq_domain_activate_irq(irq_desc_get_irq_data(desc), false); - } + for (i = 0; i < dist->its_vm.nr_vpes; i++) + WARN_ON(vgic_v4_request_vpe_irq(kvm_get_vcpu(kvm, i), + dist->its_vm.vpes[i]->irq)); } /** @@ -394,7 +391,7 @@ int vgic_v3_save_pending_tables(struct kvm *kvm) * and enabling of the doorbells have already been done. */ if (kvm_vgic_global_state.has_gicv4_1) { - unmap_all_vpes(dist); + unmap_all_vpes(kvm); vlpi_avail = true; } @@ -444,7 +441,7 @@ int vgic_v3_save_pending_tables(struct kvm *kvm) out: if (vlpi_avail) - map_all_vpes(dist); + map_all_vpes(kvm); return ret; } diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c index ad06ba6c9b00..a413718be92b 100644 --- a/arch/arm64/kvm/vgic/vgic-v4.c +++ b/arch/arm64/kvm/vgic/vgic-v4.c @@ -222,6 +222,11 @@ void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val) *val = !!(*ptr & mask); } +int vgic_v4_request_vpe_irq(struct kvm_vcpu *vcpu, int irq) +{ + return request_irq(irq, vgic_v4_doorbell_handler, 0, "vcpu", vcpu); +} + /** * vgic_v4_init - Initialize the GICv4 data structures * @kvm: Pointer to the VM being initialized @@ -283,8 +288,7 @@ int vgic_v4_init(struct kvm *kvm) irq_flags &= ~IRQ_NOAUTOEN; irq_set_status_flags(irq, irq_flags); - ret = request_irq(irq, vgic_v4_doorbell_handler, - 0, "vcpu", vcpu); + ret = vgic_v4_request_vpe_irq(vcpu, irq); if (ret) { kvm_err("failed to allocate vcpu IRQ%d\n", irq); /* diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h index 0c8da72953f0..23e280fa0a16 100644 --- a/arch/arm64/kvm/vgic/vgic.h +++ b/arch/arm64/kvm/vgic/vgic.h @@ -331,5 +331,6 @@ int vgic_v4_init(struct kvm *kvm); void vgic_v4_teardown(struct kvm *kvm); void vgic_v4_configure_vsgis(struct kvm *kvm); void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val); +int vgic_v4_request_vpe_irq(struct kvm_vcpu *vcpu, int irq); #endif -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel