From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751341AbdA2Pdr (ORCPT ); Sun, 29 Jan 2017 10:33:47 -0500 Received: from foss.arm.com ([217.140.101.70]:53184 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211AbdA2Pdn (ORCPT ); Sun, 29 Jan 2017 10:33:43 -0500 From: Marc Zyngier To: Jintack Lim Cc: , , , , , , , , , , Subject: Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level In-Reply-To: <1485479100-4966-7-git-send-email-jintack@cs.columbia.edu> (Jintack Lim's message of "Thu, 26 Jan 2017 20:04:56 -0500") Organization: ARM Ltd References: <1485479100-4966-1-git-send-email-jintack@cs.columbia.edu> <1485479100-4966-7-git-send-email-jintack@cs.columbia.edu> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) Date: Sun, 29 Jan 2017 15:21:06 +0000 Message-ID: <86sho199jx.fsf@arm.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim wrote: > Now that we maintain the EL1 physical timer register states of VMs, > update the physical timer interrupt level along with the virtual one. > > Note that the emulated EL1 physical timer is not mapped to any hardware > timer, so we call a proper vgic function. > > Signed-off-by: Jintack Lim > --- > virt/kvm/arm/arch_timer.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 0f6e935..3b6bd50 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct kvm_vcpu *vcpu, bool new_level, > WARN_ON(ret); > } > > +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level, > + struct arch_timer_context *timer) > +{ > + int ret; > + > + BUG_ON(!vgic_initialized(vcpu->kvm)); Although I've added my fair share of BUG_ON() in the code base, I've since reconsidered my position. If we get in a situation where the vgic is not initialized, maybe it would be better to just WARN_ON and return early rather than killing the whole box. Thoughts? > + > + timer->irq.level = new_level; > + trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq, > + timer->irq.level); > + ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer->irq.irq, > + timer->irq.level); > + WARN_ON(ret); > +} > + > /* > * Check if there was a change in the timer state (should we raise or lower > * the line level to the GIC). > @@ -188,6 +203,7 @@ static int 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); > > /* > * If userspace modified the timer registers via SET_ONE_REG before > @@ -201,6 +217,10 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu) > if (kvm_timer_should_fire(vcpu, vtimer) != vtimer->irq.level) > kvm_timer_update_mapped_irq(vcpu, !vtimer->irq.level, vtimer); > > + /* The emulated EL1 physical timer irq is not mapped to hardware */ Maybe a slightly better comment would be saying that we're using a purely virtual interrupt, unrelated to the hardware interrupt. > + if (kvm_timer_should_fire(vcpu, ptimer) != ptimer->irq.level) > + kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer); > + > return 0; > } Otherwise looks good. M. -- Jazz is not dead. It just smells funny.