Re: [PATCH] KVM: lapic: restart counter on change to periodic mode

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Aug 20, 2019 at 01:42:37AM +0200, Paolo Bonzini wrote:
> On 20/08/19 01:04, Matt delco wrote:
> > From: Matt Delco <delco@google.com>
> > 
> > Time seems to eventually stop in a Windows VM when using Skype.
> > Instrumentation shows that the OS is frequently switching the APIC
> > timer between one-shot and periodic mode.  The OS is typically writing
> > to both LVTT and TMICT.  When time stops the sequence observed is that
> > the APIC was in one-shot mode, the timer expired, and the OS writes to
> > LVTT (but not TMICT) to change to periodic mode.  No future timer events
> > are received by the OS since the timer is only re-armed on TMICT writes.
> > 
> > With this change time continues to advance in the VM.  TBD if physical
> > hardware will reset the current count if/when the mode is changed to
> > period and the current count is zero.
> > 
> > Signed-off-by: Matt Delco <delco@google.com>
> > ---
> >  arch/x86/kvm/lapic.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 685d17c11461..fddd810eeca5 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1935,14 +1935,19 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
> >  
> >  		break;
> >  
> > -	case APIC_LVTT:
> > +	case APIC_LVTT: {
> > +		u32 timer_mode = apic->lapic_timer.timer_mode;
> >  		if (!kvm_apic_sw_enabled(apic))
> >  			val |= APIC_LVT_MASKED;
> >  		val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);
> >  		kvm_lapic_set_reg(apic, APIC_LVTT, val);
> >  		apic_update_lvtt(apic);
> > +		if (timer_mode == APIC_LVT_TIMER_ONESHOT &&
> > +		    apic_lvtt_period(apic) &&
> > +		    !hrtimer_active(&apic->lapic_timer.timer))
> > +			start_apic_timer(apic);
> 
> The manual says "A write to the LVT Timer Register that changes the
> timer mode disarms the local APIC timer", but we already know this is
> not true (commit dedf9c5e216902c6d34b5a0d0c40f4acbb3706d8).

That was a confirmed SDM bug that has been fixed as of the May 2019
version of the SDM.

> 
> Still, this needs some more explanation.  Can you cover this, as well as
> the oneshot->periodic transition, in kvm-unit-tests' x86/apic.c
> testcase?  Then we could try running it on bare metal and see what happens.

Only transitions to/from deadline should disable the timer, i.e. this
blurb from the SDM was found to be correct.

  Transitioning between TSC-deadline mode and other timer modes also
  disarms the timer.

But yeah, tests are in order, at least for oneshot->periodic and vice
versa.  I can't find any internal code that tests whether transitioning
between oneshot and periodic actually rearms the timer or if it simply
doesn't disable it, and the SDM doesn't clarify what constitutes
"reprogrammed".

If possible, we should also test what happens if APIC_TMCCT != 0, though
that might be tricky and/or fragile.  If the timer is rearmed on a
transition between oneshot and periodic, then I would expect it to happen
for both APIC_TMCCT==0 and APIC_TMCCT!=0.

> 
> Thanks,
> 
> Paolo
> 
> 
> >  		break;
> > -
> > +	}
> >  	case APIC_TMICT:
> >  		if (apic_lvtt_tscdeadline(apic))
> >  			break;
> > 
>