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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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.
I can add a rebase of that to my list of never-ending timer rework.
Thanks,
-Christoffer
