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: 10015035 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 675FA600CC for ; Wed, 18 Oct 2017 16:48:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 56720290FD for ; Wed, 18 Oct 2017 16:48:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4B0562911A; Wed, 18 Oct 2017 16:48:22 +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 B2690290FD for ; Wed, 18 Oct 2017 16:48:21 +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=DLeBmbitG4GETADEc+lLzDuWa7u/5FymSipkwA5L3sI=; b=Bn0OuAkLBfunSb S1xG7+SaRbwcwRib6ugyzdSDkY2IsFjesajV6AntzJHzJyQ9ISEE4qjHw26w3zcQ96IwPf2YcM5mz H16cDAFUUaT/kM1ZwQkzOB7csRVnl3dMaAlKBgbyizY3H5UaN976pR0Nj1MauoF1iE4MCIvZM9tfw oWArAJm3kTDLw3RcQLpbpjAQ7QQ0MBNQBMniP4dIXHVurzLhZX7xBFb8pneOOs7tHtl10xKzGC58v f7k2MVHuFQfgUMnHz0l0fo4YzKCBQ2F1tBIQDqiggozAjZNNxc+ooHvBq0vYkPttBr/kYTJlJ+1Ke gam5xXRP8nQr+RIihi/A==; 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 1e4rW0-0007B1-T1; Wed, 18 Oct 2017 16:48:20 +0000 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1e4rVp-0006YH-Du for linux-arm-kernel@lists.infradead.org; Wed, 18 Oct 2017 16:48:13 +0000 Received: by mail-wm0-x243.google.com with SMTP id u138so11109775wmu.5 for ; Wed, 18 Oct 2017 09:47:49 -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=R1g0ee5HkGIHw1Y078hipI6888y5sTjxfgBB+tfrkz8cxmkiw66d16Dqv6xaN8Qdrp Sn7N+TrY4TXVsk1Oc+BYEtn1wTy/Dn+MW79BIv9DgUOX/Axn5oPdP1RFmb/K7uiCHmva OT4ivwEczwCqGjsKDxMiDU5onYs1zonSVnlAOded/KyALgvDUqkgWIOijDJNs2jicZD8 5gW6I5viMNW/2Vlo45vW9Pcw+fdBqTxL7tTDrsXy1n9EjF6Fj4r6XG+JwkATw7HgwZOs PqDyhSACAWl0vKY/SvCK5trO+UJXbEOueyx7xyVdf1RmLhlt2H5fxMz7cOLLNJsa4Gax ZCLw== X-Gm-Message-State: AMCzsaV0Fa+XWJq2Hjjc6FxHiM8pFFmBlc9dxH75zZsPspdR9XC44GIi A3tqvarE65wA0B1A238GU+ciPA== 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 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) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171018_094809_867259_74DA34D5 X-CRM114-Status: GOOD ( 29.23 ) 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: Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@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 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)