From patchwork Tue Jul 17 10:10:40 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 10528801 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 1330160545 for ; Tue, 17 Jul 2018 10:11:05 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 125012863D for ; Tue, 17 Jul 2018 10:11:05 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0679828968; Tue, 17 Jul 2018 10:11:05 +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=-2.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.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 5484028A8C for ; Tue, 17 Jul 2018 10:11:04 +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=6jNFIgjJX6PA3MKwOnQ2TaCdnusYSTNe/DUorCRQtWg=; b=Wgb1nBZVJUQVwj eplVPVl57kKSbEeHDtHsNmZJzTjjEJQ428VpJ0RWYpu6+0FdVRE/2ZumE8IMFZVr094rqfp9gFcSp 6yCNSygK1XIVO/xF1sZRjhza/k6h+E1BySP5SIww06IHIt36r8nihBUQJWAocYt9B0szvK6SUSyrk A26zg5cEI0kivOhhZd+e+ZTfXKttggP29Z4Z7hC1zyOpLsjW7DsStelAJ7Bb7x6q2S95GapHbtBRc sR+FDk+Oijb1j7OmKjH8/dyxUykUcS5E5WXk4nrAhPfrRTwAsJ+jnLM6d1hXTXERC3zBUQjGTB3Jv WbIqigE8JllL7vgR14OA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1ffMwd-0006pd-Cf; Tue, 17 Jul 2018 10:10:59 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1ffMwW-0006nq-V3 for linux-arm-kernel@lists.infradead.org; Tue, 17 Jul 2018 10:10:57 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B0B0C7A9; Tue, 17 Jul 2018 03:10:42 -0700 (PDT) Received: from localhost (e113682-lin.copenhagen.arm.com [10.32.144.41]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1A72B3F5A0; Tue, 17 Jul 2018 03:10:41 -0700 (PDT) Date: Tue, 17 Jul 2018 12:10:40 +0200 From: Christoffer Dall To: Andre Przywara Subject: Re: [PATCH] kvm: arm/arm64: Fix emulated physical timer IRQ injection Message-ID: <20180717101040.GE30570@e113682-lin.lund.arm.com> References: <20180716172357.6121-1-andre.przywara@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180716172357.6121-1-andre.przywara@arm.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180717_031053_095533_42972172 X-CRM114-Status: GOOD ( 33.70 ) 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 , Stable , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.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 Mon, Jul 16, 2018 at 06:23:57PM +0100, Andre Przywara wrote: > When KVM emulates a physical timer, we keep track of the interrupt > condition and try to inject an IRQ to the guest when needed. > This works if the timer expires when either the guest is running or KVM > does work on behalf of it, since it calls kvm_timer_update_state(). > However when the guest's VCPU is not scheduled (for instance because > the guest issued a WFI instruction before), we miss injecting the interrupt > when the VCPU's state gets restored back in kvm_timer_vcpu_load(). > > Fix this by moving the interrupt injection check into the > phys_timer_emulate() function, so that all possible paths of execution > are covered. > > This fixes the physical timer emulation, which broke when it got changed > in the 4.15 merge window. > The respective kvm-unit-test check has been posted already. > > Cc: Stable # 4.15+ > Signed-off-by: Andre Przywara > --- > virt/kvm/arm/arch_timer.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index bd3d57f40f1b..1949fb0b80a4 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -294,17 +294,26 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu) > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); > > + /* If the timer cannot fire at all, then we don't need a soft timer. */ > + if (!kvm_timer_irq_can_fire(ptimer)) { > + soft_timer_cancel(&timer->phys_timer, NULL); > + kvm_timer_update_irq(vcpu, false, ptimer); > + return; > + } This stuff is breaking what the intention is with the phys_timer_emulate() function. See the comment above the function, which says that this is about scheduling the background soft timer, and not about managing the rest of the state. At least that function needs updating. > + > /* > - * If the timer can fire now we have just raised the IRQ line and we > - * don't need to have a soft timer scheduled for the future. If the > - * timer cannot fire at all, then we also don't need a soft timer. > + * If the timer can fire now, we don't need to have a soft timer > + * scheduled for the future, as we also raise the IRQ line. > */ > - if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) { > + if (kvm_timer_should_fire(ptimer)) { > soft_timer_cancel(&timer->phys_timer, NULL); > + kvm_timer_update_irq(vcpu, true, ptimer); > + > return; > } > > soft_timer_start(&timer->phys_timer, kvm_timer_compute_delta(ptimer)); > + kvm_timer_update_irq(vcpu, false, ptimer); This is clearly racy as you'll lower the interrupt that may have just fired between these two lines. > } > > /* > @@ -316,7 +325,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu) > { > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); > - struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); > bool level; > > if (unlikely(!timer->enabled)) > @@ -332,9 +340,6 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu) > level = kvm_timer_should_fire(vtimer); > kvm_timer_update_irq(vcpu, level, vtimer); > > - if (kvm_timer_should_fire(ptimer) != ptimer->irq.level) > - kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer); > - > phys_timer_emulate(vcpu); > } > > -- > 2.14.4 > Overall, I think this approach is wrong. Please see the following suggestion for doing this in two patches instead: From 08b40157cc6a69d72a37afff38fce9d0b482953f Mon Sep 17 00:00:00 2001 From: Christoffer Dall Date: Tue, 17 Jul 2018 11:53:39 +0200 Subject: [PATCH 1/2] KVM: arm/arm64: Fix potential loss of ptimer interrupts kvm_timer_update_state() is called when changing the phys timer configuration registers, either via vcpu reset, as a result of a trap from the guest, or when userspace programs the registers. phys_timer_emulate() is in turn called by kvm_timer_update_state() to either cancel an existing software timer, or program a new software timer, to emulate the behavior of a real phys timer, based on the change in configuration registers. Unfortunately, the interaction between these two functions left a small race; if the conceptual emulated phys timer should actually fire, but the soft timer hasn't executed its callback yet, we cancel the timer in phys_timer_emulate without injecting an irq. This only happens if the check in kvm_timer_update_state is called before the timer should fire, which is relatively unlikely, but possible. The solution is to update the state of the phys timer after calling phys_timer_emulate, which will pick up the pending timer state and update the interrupt value. Note that this leaves the opportunity of raising the interrupt twice, once in the just-programmed soft timer, and once in kvm_timer_update_state. Since this always happens synchronously with the VCPU execution, there is no harm in this, and the guest ever only sees a single timer interrupt. Signed-off-by: Christoffer Dall --- virt/kvm/arm/arch_timer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index bd3d57f..4db1fbf 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -332,10 +332,10 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu) level = kvm_timer_should_fire(vtimer); kvm_timer_update_irq(vcpu, level, vtimer); + phys_timer_emulate(vcpu); + if (kvm_timer_should_fire(ptimer) != ptimer->irq.level) kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer); - - phys_timer_emulate(vcpu); } static void vtimer_save_state(struct kvm_vcpu *vcpu) -- 2.7.4 From bce5c9ec3d7e0e2e7b8d4c733a18c0a987be16f5 Mon Sep 17 00:00:00 2001 From: Christoffer Dall Date: Tue, 17 Jul 2018 12:04:28 +0200 Subject: [PATCH 2/2] KVM: arm/arm64: Fix lost IRQs from emulated physcial timer when blocked When the VCPU is blocked (for example from WFI) we don't inject the physical timer interrupt if it should fire while the CPU is blocked, but instead we just wake up the VCPU and expect kvm_timer_vcpu_load to take care of injecting the interrupt. Unfortunately, kvm_timer_vcpu_load() doesn't actually do that, it only has support to schedule a soft timer if the emulated phys timer is expected to fire in the future. Follow the same pattern as kvm_timer_update_state() and update the irq state after potentially scheduling a soft timer. Signed-off-by: Christoffer Dall --- virt/kvm/arm/arch_timer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 4db1fbf..bd64fb4 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -502,6 +502,10 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu) /* Set the background timer for the physical timer emulation. */ phys_timer_emulate(vcpu); + + /* If the timer fired while we weren't running, inject it now */ + if (kvm_timer_should_fire(ptimer) != ptimer->irq.level) + kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer); } bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)