From patchwork Fri Dec 15 10:04:51 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 10114573 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 7540E60327 for ; Fri, 15 Dec 2017 10:05:32 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5D80829EC2 for ; Fri, 15 Dec 2017 10:05:32 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5054F29EF2; Fri, 15 Dec 2017 10:05:32 +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,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 3588029EC2 for ; Fri, 15 Dec 2017 10:05:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=MFo8Dhdhd6y2lNbW51AskW6g2EZ3qMrHq02OMBCF+6c=; b=tFZhWui6sqf/Q7 I2aLn4LH+a1iC+xC/gaVJWI5fpG0yPYhRmSfGMoucfWy26PbRn579u/RYojc8LDv5xwbiHlftFeVF YjLRqWCXTQ0dKVfzfrm7uAQwOHEqZysM0k1SAH1qQIn3vhFaqKSDdDHlV+IYTjG/9OYJMWof4S2v5 D8LmR5cG3eO8NNJUuyIeawvHzUmfKZ6lHYYgG8pUYl7tHx2ePfjatPmSTgYVrxnzW4u+VL29Ir/zU zy3gjbMOTpECDTrRdg2J9OdiM1QsRIrdWDfE6y2wzBvg3dfc1W6v3HGU9j9GAmGk/X06ikIoPbzWd 1oPD9T2ZN1cOs4fUjK/Q==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1ePmry-0000Qc-IE; Fri, 15 Dec 2017 10:05:30 +0000 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1ePmrq-0007eS-6h for linux-arm-kernel@lists.infradead.org; Fri, 15 Dec 2017 10:05:28 +0000 Received: by mail-wm0-x241.google.com with SMTP id b199so16466449wme.1 for ; Fri, 15 Dec 2017 02:05:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=TRTflxDoC5lHQE0YXN4EvVrozDFSA0QwtJlZur/Qb9E=; b=eIGZ2ZlGzPEGCO6V9Ul+L+2CA3U0qXXcENHtajF1lbWeyKDHbBWgNIAScB+f7Ugpm7 3oub4qyg7VkTYp/zuFTfOrSlRA54sXNwTH3uIFy1yj+WyatZw04FI09lO9afW/OXb/W7 Wz2xmOu7GsaGgW8rbGVrwE7Lg8j79kNdKrxBQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=TRTflxDoC5lHQE0YXN4EvVrozDFSA0QwtJlZur/Qb9E=; b=pW2mDtYso3zLMUHbeuxlOjuP6cEzangoHh06fHe5ChqHgBxfURg768Gzf4NBsWYZfG S0wlW23/AXeiNe8HvxpgdH3J6f5Yazw5XrRbSNhvOLMHkDSamaJBWOCad7yDSH9ALiSW KqY2Sq2Bw3pfkI6pH2H5pvVTHR2Qx324N5LPlxPwYLGGy6yLFFP3H+Hk7nCVSC3Ufz2/ 920yuVSWYlBTDaV0tIYBH2z5M2tXKgLxybY/3g9/Az5M1RTNj1DnSy7jM/VuTpj1pvyB P+wGS/UjVEjHdrjwlYgOPJDq7Z++trQjtMcviRzhtBKmHqzWI8fIBa1rdNH7zrOoPKUg XVRg== X-Gm-Message-State: AKGB3mILBP8XFyMJBAeWZae/4dMiJmHBTnszfGOx3yvs2QScdfFkdQIa T04uBsBJQKhKHvTmRZW4Bb9rKg== X-Google-Smtp-Source: ACJfBouzxq+ZuWaWdgnsd2ZbCl5mAqtHwrm11zNzMeBMAfEAvCE0P3nitaSvBdfAGvE6sWmdLZMWVg== X-Received: by 10.80.179.187 with SMTP id s56mr16060008edd.87.1513332300171; Fri, 15 Dec 2017 02:05:00 -0800 (PST) Received: from localhost (x50d2404e.cust.hiper.dk. [80.210.64.78]) by smtp.gmail.com with ESMTPSA id h16sm5157548edj.34.2017.12.15.02.04.58 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Fri, 15 Dec 2017 02:04:58 -0800 (PST) Date: Fri, 15 Dec 2017 11:04:51 +0100 From: Christoffer Dall To: Jia He Subject: Re: [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler Message-ID: <20171215100451.GY910@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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171215_020522_402840_3AF0C88E X-CRM114-Status: GOOD ( 22.37 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marc Zyngier , Jia He , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Dec 15, 2017 at 10:27:02AM +0800, Jia He wrote: [...] > >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > >index 73d262c4712b..544ed15fbbb3 100644 > >--- a/virt/kvm/arm/arch_timer.c > >+++ b/virt/kvm/arm/arch_timer.c > >@@ -46,7 +46,7 @@ static const struct kvm_irq_level default_vtimer_irq = { > > .level = 1, > > }; > >-static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx); > >+static bool kvm_timer_irq_can_fire(u32 cnt_ctl); > > static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level, > > struct arch_timer_context *timer_ctx); > > static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx); > >@@ -94,6 +94,7 @@ 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"); > >@@ -101,8 +102,8 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) > > } > > vtimer = vcpu_vtimer(vcpu); > >- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); > >- if (kvm_timer_irq_can_fire(vtimer)) > >+ cnt_ctl = read_sysreg_el0(cntv_ctl); > >+ if (kvm_timer_irq_can_fire(cnt_ctl)) > > kvm_timer_update_irq(vcpu, true, vtimer); > IIUC, your patch makes kvm_arch_timer_handler never changesvtimer->cnt_ctl Yes, that's the idea. 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 --- virt/kvm/arm/arch_timer.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) Thanks, -Christoffer 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;