From patchwork Wed Oct 18 16:47:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 10015033 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 A8FE2600CC for ; Wed, 18 Oct 2017 16:47:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 991D7290FD for ; Wed, 18 Oct 2017 16:47:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8E1342911A; Wed, 18 Oct 2017 16:47:52 +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=-6.5 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E92F3290FD for ; Wed, 18 Oct 2017 16:47:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751180AbdJRQru (ORCPT ); Wed, 18 Oct 2017 12:47:50 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:46454 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750954AbdJRQrt (ORCPT ); Wed, 18 Oct 2017 12:47:49 -0400 Received: by mail-wm0-f67.google.com with SMTP id m72so11146591wmc.1 for ; Wed, 18 Oct 2017 09:47:48 -0700 (PDT) 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=w6PMZdNsrxr7XbBaauzAq29r5BKG/PFkqVeanwytw4I=; b=jhfbRyOolTRM2tEPRDB5PTPdTu+NBOxlKD9KN+TBTeJzINXFYe5rKpgXKs7PAPzb7E 8Btb27GXmn33/xaU9YJBIdSiq9uc+56qxEGCS/lXy8jpiXhWmmyekkvLmFoU5wLz73Au /Y+OZdgeXCCc1II+NFEu086auK1ipWcZUIBCg= 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=w6PMZdNsrxr7XbBaauzAq29r5BKG/PFkqVeanwytw4I=; b=Xa0Nz6h365TnNBcLUKs1AZwJuho/U4jLGwTqEzvMe6gLHhemanFYSnshDeymx/QBMH l+8PnLYbX8KrfGh90qwAg69SlPV6Q9gnX9EjtkqSZWsDYTIm838frinIpofptcikIIf6 L2XYyAURFbCVNwukZDUL5vwIs2yRpPBmAoCMhC6kE3jGO0IFHvIaOUxEdVgAtpf9zZuA MkEm5Y8aBvm+uqMN3cdDVKjbeXePDWS+GIeZf/iB9N74Uqx4ysO5AtkiVF12Qaa4Ww1+ cCAB2nZD8+TN8cp0kLjviHIDP4eAYCUNfU3LSprR78jrCPbfrbxEDTvAICXO0Htf8mkb tIwg== X-Gm-Message-State: AMCzsaX5NSlrioJ4mGl23I4Lp3RoyMYGkoZQgtDYDGLePLS07vSfu9gF z3z+1urQwDbIbR/Bwpa8ceUOZg== X-Google-Smtp-Source: ABhQp+RNcIAt94O2aHGx3L6hxx5WevMUcEcyQdw8ysVsMUASJNlLsdIviTWsCC5ldl4IuK4lT/hPvg== X-Received: by 10.80.141.15 with SMTP id s15mr641571eds.241.1508345267714; Wed, 18 Oct 2017 09:47:47 -0700 (PDT) Received: from localhost (xd93dd96b.cust.hiper.dk. [217.61.217.107]) by smtp.gmail.com with ESMTPSA id c30sm7544066edf.65.2017.10.18.09.47.46 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 18 Oct 2017 09:47:46 -0700 (PDT) Date: Wed, 18 Oct 2017 18:47:50 +0200 From: Christoffer Dall To: Marc Zyngier Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Will Deacon , Catalin Marinas Subject: Re: [PATCH v3 07/20] KVM: arm/arm64: Make timer_arm and timer_disarm helpers more generic Message-ID: <20171018164750.GI8900@cbox> References: <20170923004207.22356-1-cdall@linaro.org> <20170923004207.22356-8-cdall@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Oct 09, 2017 at 06:05:04PM +0100, Marc Zyngier wrote: > On 23/09/17 01:41, Christoffer Dall wrote: > > We are about to add an additional soft timer to the arch timer state for > > a VCPU and would like to be able to reuse the functions to program and > > cancel a timer, so we make them slightly more generic and rename to make > > it more clear that these functions work on soft timers and not the > > hardware resource that this code is managing. > > > > Signed-off-by: Christoffer Dall > > --- > > virt/kvm/arm/arch_timer.c | 33 ++++++++++++++++----------------- > > 1 file changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > > index 8e89d63..871d8ae 100644 > > --- a/virt/kvm/arm/arch_timer.c > > +++ b/virt/kvm/arm/arch_timer.c > > @@ -56,26 +56,22 @@ u64 kvm_phys_timer_read(void) > > return timecounter->cc->read(timecounter->cc); > > } > > > > -static bool timer_is_armed(struct arch_timer_cpu *timer) > > +static bool soft_timer_is_armed(struct arch_timer_cpu *timer) > > { > > return timer->armed; > > } > > > > -/* timer_arm: as in "arm the timer", not as in ARM the company */ > > -static void timer_arm(struct arch_timer_cpu *timer, u64 ns) > > +static void soft_timer_start(struct hrtimer *hrt, u64 ns) > > { > > - timer->armed = true; > > - hrtimer_start(&timer->timer, ktime_add_ns(ktime_get(), ns), > > + hrtimer_start(hrt, ktime_add_ns(ktime_get(), ns), > > HRTIMER_MODE_ABS); > > } > > > > -static void timer_disarm(struct arch_timer_cpu *timer) > > +static void soft_timer_cancel(struct hrtimer *hrt, struct work_struct *work) > > { > > - if (timer_is_armed(timer)) { > > - hrtimer_cancel(&timer->timer); > > - cancel_work_sync(&timer->expired); > > - timer->armed = false; > > - } > > + hrtimer_cancel(hrt); > > + if (work) > > When can this happen? Something in a following patch? > Yeah, sorry about that. I will point this out in the commit message. > > + cancel_work_sync(work); > > } > > > > static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) > > @@ -271,7 +267,7 @@ static void kvm_timer_emulate(struct kvm_vcpu *vcpu, > > return; > > > > /* The timer has not yet expired, schedule a background timer */ > > - timer_arm(timer, kvm_timer_compute_delta(timer_ctx)); > > + soft_timer_start(&timer->timer, kvm_timer_compute_delta(timer_ctx)); > > } > > > > /* > > @@ -285,7 +281,7 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu) > > struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); > > struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); > > > > - BUG_ON(timer_is_armed(timer)); > > + BUG_ON(soft_timer_is_armed(timer)); > > > > /* > > * No need to schedule a background timer if any guest timer has > > @@ -306,13 +302,16 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu) > > * The guest timers have not yet expired, schedule a background timer. > > * Set the earliest expiration time among the guest timers. > > */ > > - timer_arm(timer, kvm_timer_earliest_exp(vcpu)); > > + timer->armed = true; > > + soft_timer_start(&timer->timer, kvm_timer_earliest_exp(vcpu)); > > } > > > > void kvm_timer_unschedule(struct kvm_vcpu *vcpu) > > { > > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > > - timer_disarm(timer); > > + > > + soft_timer_cancel(&timer->timer, &timer->expired); > > + timer->armed = false; > > } > > > > static void kvm_timer_flush_hwstate_vgic(struct kvm_vcpu *vcpu) > > @@ -448,7 +447,7 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu) > > * This is to cancel the background timer for the physical timer > > * emulation if it is set. > > */ > > - timer_disarm(timer); > > + soft_timer_cancel(&timer->timer, &timer->expired); > > timer_disarm() used to set timer->armed to false, but that's not the > case any more. Don't we risk hitting the BUG_ON() in kvm_timer_schedule > if we hit WFI? > We do, and I just didn't hit that because this goes away at the end of the series, and I didn't vigurously test every single patch in the series (just a compile test). We actually only use the armed flag for the BUG_ON(), and I don't think we need that check really. So I suggest simply merging this logic into this patch: Thanks, -Christoffer diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h index f0053f884b4a..d0beae98f755 100644 --- a/include/kvm/arm_arch_timer.h +++ b/include/kvm/arm_arch_timer.h @@ -48,9 +48,6 @@ struct arch_timer_cpu { /* Work queued with the above timer expires */ struct work_struct expired; - /* Background timer active */ - bool armed; - /* Is the timer enabled */ bool enabled; }; diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 871d8ae52f9b..98643bc696a9 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -56,11 +56,6 @@ u64 kvm_phys_timer_read(void) return timecounter->cc->read(timecounter->cc); } -static bool soft_timer_is_armed(struct arch_timer_cpu *timer) -{ - return timer->armed; -} - static void soft_timer_start(struct hrtimer *hrt, u64 ns) { hrtimer_start(hrt, ktime_add_ns(ktime_get(), ns), @@ -281,8 +276,6 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu) struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); - BUG_ON(soft_timer_is_armed(timer)); - /* * No need to schedule a background timer if any guest timer has * already expired, because kvm_vcpu_block will return before putting @@ -302,7 +295,6 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu) * The guest timers have not yet expired, schedule a background timer. * Set the earliest expiration time among the guest timers. */ - timer->armed = true; soft_timer_start(&timer->timer, kvm_timer_earliest_exp(vcpu)); } @@ -311,7 +303,6 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu) struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; soft_timer_cancel(&timer->timer, &timer->expired); - timer->armed = false; } static void kvm_timer_flush_hwstate_vgic(struct kvm_vcpu *vcpu)