From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751850AbdBAKHZ (ORCPT ); Wed, 1 Feb 2017 05:07:25 -0500 Received: from mail-lf0-f45.google.com ([209.85.215.45]:34888 "EHLO mail-lf0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751344AbdBAKHX (ORCPT ); Wed, 1 Feb 2017 05:07:23 -0500 Date: Wed, 1 Feb 2017 11:07:16 +0100 From: Christoffer Dall To: Jintack Lim Cc: Marc Zyngier , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , linux@armlinux.org.uk, Catalin Marinas , Will Deacon , Andre Przywara , KVM General , arm-mail-list , kvmarm@lists.cs.columbia.edu, lkml - Kernel Mailing List Subject: Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level Message-ID: <20170201100716.GC6226@cbox> References: <1485479100-4966-1-git-send-email-jintack@cs.columbia.edu> <1485479100-4966-7-git-send-email-jintack@cs.columbia.edu> <86sho199jx.fsf@arm.com> <20170201080440.GB6226@cbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 01, 2017 at 03:40:10AM -0500, Jintack Lim wrote: > On Wed, Feb 1, 2017 at 3:04 AM, Christoffer Dall > wrote: > > On Sun, Jan 29, 2017 at 03:21:06PM +0000, Marc Zyngier wrote: > >> 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? > >> > > > > Could we help this series along by saying that since this BUG_ON already > > exists in the kvm_timer_update_mapped_irq function, then it just > > preserves functionality and it's up to someone else (me) to remove the > > BUG_ON from both functions later in life? > > > > Sounds good to me :) Thanks! > So just as you thought you were getting off easy... The reason we now have kvm_timer_update_irq and kvm_timer_update_mapped_irq is that we have the following two vgic functions: kvm_vgic_inject_irq kvm_vgic_inject_mapped_irq But the only difference between the two is what they pass as the mapped_irq argument to vgic_update_irq_pending. What about if we just had this as a precursor patch: diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 6a084cd..91ecf48 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -175,7 +175,8 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level) timer->irq.level = new_level; trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq, timer->irq.level); - ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id, + + ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer->irq.irq, timer->irq.level); WARN_ON(ret); diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index dea12df..4c87fd0 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -336,8 +336,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq) } static int vgic_update_irq_pending(struct kvm *kvm, int cpuid, - unsigned int intid, bool level, - bool mapped_irq) + unsigned int intid, bool level) { struct kvm_vcpu *vcpu; struct vgic_irq *irq; @@ -357,11 +356,6 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid, if (!irq) return -EINVAL; - if (irq->hw != mapped_irq) { - vgic_put_irq(kvm, irq); - return -EINVAL; - } - spin_lock(&irq->irq_lock); if (!vgic_validate_injection(irq, level)) { @@ -399,13 +393,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid, int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid, bool level) { - return vgic_update_irq_pending(kvm, cpuid, intid, level, false); -} - -int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid, - bool level) -{ - return vgic_update_irq_pending(kvm, cpuid, intid, level, true); + return vgic_update_irq_pending(kvm, cpuid, intid, level); } int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq) That would make this patch simpler. If so, I can send out the above patch with a proper description. Thanks, -Christoffer