On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote:
> On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> >
> > The VGIC can now support the life-cycle of mapped level-triggered
> > interrupts, and we no longer have to read back the timer state on every
> > exit from the VM if we had an asserted timer interrupt signal, because
> > the VGIC already knows if we hit the unlikely case where the guest
> > disables the timer without ACKing the virtual timer interrupt.
> >
> > This means we rework a bit of the code to factor out the functionality
> > to snapshot the timer state from vtimer_save_state(), and we can reuse
> > this functionality in the sync path when we have an irqchip in
> > userspace, and also to support our implementation of the
> > get_input_level() function for the timer.
> >
> > This change also means that we can no longer rely on the timer's view of
> > the interrupt line to set the active state, because we no longer
> > maintain this state for mapped interrupts when exiting from the guest.
> > Instead, we only set the active state if the virtual interrupt is
> > active, and otherwise we simply let the timer fire again and raise the
> > virtual interrupt from the ISR.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > ---
> > include/kvm/arm_arch_timer.h | 2 ++
> > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++-----------------------
> > 2 files changed, 38 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index 01ee473517e2..f57f795d704c 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
> >
> > void kvm_timer_init_vhe(void);
> >
> > +bool kvm_arch_timer_get_input_level(int vintid);
> > +
> > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
> > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
> >
>
> [...]
>
> > +bool kvm_arch_timer_get_input_level(int vintid)
> > +{
> > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> > + struct arch_timer_context *timer;
> > +
> > + if (vintid == vcpu_vtimer(vcpu)->irq.irq)
> > + timer = vcpu_vtimer(vcpu);
> > + else
> > + BUG(); /* We only map the vtimer so far */
> > +
> > + if (timer->loaded)
> > + __timer_snapshot_state(timer);
> > +
> > + return kvm_timer_should_fire(timer);
> > +}
>
> I think it worth to reword to highlight your intention about BUG,
> and save 1 call to vcpu_vtimer()
>
> bool kvm_arch_timer_get_input_level(int vintid)
> {
> struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
> struct arch_timer_context *timer = vcpu_vtimer(vcpu);
>
> /* We only map the vtimer so far */
> BUG_ON(vintid != timer->irq.irq)
>
> if (timer->loaded)
> __timer_snapshot_state(timer);
>
> return kvm_timer_should_fire(timer);
> }
>
Besides the incredible bikesheding nature of your comments, I disagree.
The current code suggests where to add functionality once we move to
using the physical timer hardware to drive an EL1 physical timer on VHE
systems, and is purposely written this way.
I'm sure we have real bugs and real issues in the code, perhaps you
could spend your energy looking for those, and if you cannot find any,
then provide a reviewed-by instead of these pointless cosmetic
adjustments.
Thanks,
-Christoffer
