Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

From: Christoffer Dall <christoffer.dall@linaro.org>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Jintack Lim <jintack@cs.columbia.edu>,
	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
Subject: Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level
Date: Wed, 1 Feb 2017 09:02:35 +0100	[thread overview]
Message-ID: <20170201080235.GA6226@cbox> (raw)
In-Reply-To: <772a1d9b-4410-7bf5-d36b-19d49d261506@arm.com>

On Tue, Jan 31, 2017 at 05:00:03PM +0000, Marc Zyngier wrote:
> 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 <jintack@cs.columbia.edu> 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 <jintack@cs.columbia.edu>
> >>>>>>> ---
> >>>>>>>  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! ;-)
> 

Haha, maybe that will finally make the code right.

-Christoffer