From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751918AbdAaRYc (ORCPT ); Tue, 31 Jan 2017 12:24:32 -0500 Received: from foss.arm.com ([217.140.101.70]:43216 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751606AbdAaRXo (ORCPT ); Tue, 31 Jan 2017 12:23:44 -0500 Subject: Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level To: Christoffer Dall 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> <20170130150203.GD16459@cbox> <20170130184131.GE16459@cbox> <13152c8e-2256-4224-e15c-73c8d6ce06e4@arm.com> <20170130190654.GG16459@cbox> Cc: Jintack Lim , pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, andre.przywara@arm.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org From: Marc Zyngier Organization: ARM Ltd Message-ID: <772a1d9b-4410-7bf5-d36b-19d49d261506@arm.com> Date: Tue, 31 Jan 2017 17:00:03 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170130190654.GG16459@cbox> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/01/17 19:06, Christoffer Dall wrote: > On Mon, Jan 30, 2017 at 06:48:02PM +0000, Marc Zyngier wrote: >> On 30/01/17 18:41, Christoffer Dall wrote: >>> On Mon, Jan 30, 2017 at 05:50:03PM +0000, Marc Zyngier wrote: >>>> On 30/01/17 15:02, 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? >>>>>> >>>>> >>>>> The distinction to me is whether this will cause fatal crashes or >>>>> exploits down the road if we're working on uninitialized data. If all >>>>> that can happen if the vgic is not initialized, is that the guest >>>>> doesn't see interrupts, for example, then a WARN_ON is appropriate. >>>>> >>>>> Which is the case here? >>>>> >>>>> That being said, do we need this at all? This is in the critial path >>>>> and is actually measurable (I know this from my work on the other timer >>>>> series), so it's better to get rid of it if we can. Can we simply >>>>> convince ourselves this will never happen, and is the code ever likely >>>>> to change so that it gets called with the vgic disabled later? >>>> >>>> That'd be the best course of action. I remember us reworking some of >>>> that in the now defunct vgic-less series. Maybe we could salvage that >>>> code, if only for the time we spent on it... >>>> >>> Ah, we never merged it? Were we waiting on a userspace implementation >>> or agreement on the ABI? >> >> We were waiting on the userspace side to be respun against the latest >> API, and there were some comments from Peter (IIRC) about supporting >> PPIs in general (the other timers and the PMU, for example). >> >> None of that happened, as the most vocal proponent of the series >> apparently lost interest. >> >>> There was definitely a useful cleanup with the whole enabled flag thing >>> on the timer I remember. >> >> Indeed. We should at least try to resurrect that bit. >> > > It's probably worth it trying to resurrect the whole thing I think, > especially since I think the implementation ended up looking quite nice. Indeed. My only concern is about the userspace counterpart, which hasn't materialized when expected. Hopefully it will this time around! > I can add a rebase of that to my list of never-ending timer rework. We all know that you can do that while sleeping! ;-) Thanks, M. -- Jazz is not dead. It just smells funny...