Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752932AbdLULfT (ORCPT ); Thu, 21 Dec 2017 06:35:19 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35082 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751403AbdLULfR (ORCPT ); Thu, 21 Dec 2017 06:35:17 -0500 X-Google-Smtp-Source: ACJfBovBmabTqX3wF3QO4M7m4aj/f22yhzJHiru9jwg8iBZGbSRD2ejnmGcS3XXXgOmfv9rrpfT2tQ== Date: Thu, 21 Dec 2017 12:35:06 +0100 From: Christoffer Dall To: Jia He Cc: Marc Zyngier , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, Jia He Subject: Re: [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler Message-ID: <20171221113506.GD29301@cbox> References: <1513148407-2611-1-git-send-email-hejianet@gmail.com> <20171213091803.GQ910@cbox> <20171214130954.GV910@cbox> <5615f3e1-756e-0537-f0b6-20ae8626ac87@gmail.com> <20171214154518.GX910@cbox> <20171215100451.GY910@cbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5023 Lines: 141 On Thu, Dec 21, 2017 at 05:16:48PM +0800, Jia He wrote: > > Sorry for the late, I ever thought you would send out v2 with isb(). > It seems not. > I did: https://lists.cs.columbia.edu/pipermail/kvmarm/2017-December/029078.html > > On 12/15/2017 6:04 PM, Christoffer Dall Wrote: > >On Fri, Dec 15, 2017 at 10:27:02AM +0800, Jia He wrote: > > > >[...] > [...] > > > >Meanwhile, I think I thought of a cleaner way to do this. Could you > >test the following two patches on your platform as well? > > > >>From 3a594a3aa222bd64a86f6c6afcb209c9be20d5c5 Mon Sep 17 00:00:00 2001 > >From: Christoffer Dall > >Date: Thu, 14 Dec 2017 19:54:50 +0100 > >Subject: [PATCH 1/2] KVM: arm/arm64: Properly handle arch-timer IRQs after > > vtimer_save_state > > > >The recent timer rework was assuming that once the timer was disabled, > >we should no longer see any interrupts from the timer. This assumption > >turns out to not be true, and instead we have to handle the case when > >the timer ISR runs even after the timer has been disabled. > > > >This requires a couple of changes: > > > >First, we should never overwrite the cached guest state of the timer > >control register when the ISR runs, because KVM may have disabled its > >timers when doing vcpu_put(), even though the guest still had the timer > >enabled. > > > >Second, we shouldn't assume that the timer is actually firing just > >because we see an ISR, but we should check the ISTATUS field of the > >timer control register to understand if the hardware timer is really > >firing or not. > > > >Signed-off-by: Christoffer Dall > > Signed-off-by: Jia He > Did you write parts of this patch and thus I should have had your signed-off-by ? Or did you mean to provide another tag. Anyway, these patches have been pulled already, so I hope we can live with the way they are. > >--- > > virt/kvm/arm/arch_timer.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > >index aa9adfafe12b..792bcf6277b6 100644 > >--- a/virt/kvm/arm/arch_timer.c > >+++ b/virt/kvm/arm/arch_timer.c > >@@ -92,16 +92,21 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) > > { > > struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; > > struct arch_timer_context *vtimer; > >+ u32 cnt_ctl; > >- if (!vcpu) { > >- pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n"); > >- return IRQ_NONE; > >- } > >- vtimer = vcpu_vtimer(vcpu); > >+ /* > >+ * We may see a timer interrupt after vcpu_put() has been called which > >+ * sets the CPU's vcpu pointer to NULL, because even though the timer > >+ * has been disabled in vtimer_save_state(), the singal may not have > >+ * been retired from the interrupt controller yet. > >+ */ > >+ if (!vcpu) > >+ return IRQ_HANDLED; > >+ vtimer = vcpu_vtimer(vcpu); > > if (!vtimer->irq.level) { > >- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); > >- if (kvm_timer_irq_can_fire(vtimer)) > >+ cnt_ctl = read_sysreg_el0(cntv_ctl); > >+ if (cnt_ctl & ARCH_TIMER_CTRL_IT_STAT) > > kvm_timer_update_irq(vcpu, true, vtimer); > > } > > > > > >>From ed96302b47d209024814df116994f64dc8f07c96 Mon Sep 17 00:00:00 2001 > >From: Christoffer Dall > >Date: Fri, 15 Dec 2017 00:30:12 +0100 > >Subject: [PATCH 2/2] KVM: arm/arm64: Fix timer enable flow > > > >When enabling the timer on the first run, we fail to ever restore the > >state and mark it as loaded. That means, that in the initial entry to > >the VCPU ioctl, unless we exit to userspace for some reason such as a > >pending signal, if the guest programs a timer and blocks, we will wait > >forever, because we never read back the hardware state (the loaded flag > >is not set), and so we think the timer is disabled, and we never > >schedule a background soft timer. > > > >The end result? The VCPU blocks forever, and the only solution is to > >kill the thread. > > > >Signed-off-by: Christoffer Dall > >--- > > virt/kvm/arm/arch_timer.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > >index 792bcf6277b6..8869658e6983 100644 > >--- a/virt/kvm/arm/arch_timer.c > >+++ b/virt/kvm/arm/arch_timer.c > >@@ -843,10 +843,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu) > > no_vgic: > > preempt_disable(); > > timer->enabled = 1; > >- if (!irqchip_in_kernel(vcpu->kvm)) > >- kvm_timer_vcpu_load_user(vcpu); > >- else > >- kvm_timer_vcpu_load_vgic(vcpu); > >+ kvm_timer_vcpu_load(vcpu); > > preempt_enable(); > > return 0; > > > > > >Thanks, > >-Christoffer > > > I have tested your 2 patches in my QDF2400 server for 10 times, the > guest can be boot up without any issues. > Without this patch, the guest will always hang in booting stages. > Thanks for this, it's comforting to know. Thanks, -Christoffer