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

[Wanpeng Li <kernellwp@gmail.com>]

On Tue, 20 Aug 2019 at 12:10, Nadav Amit <nadav.amit@gmail.com> wrote:
>
> > On Aug 19, 2019, at 6:56 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> > +Cc Nadav
> >
> > On Mon, Aug 19, 2019 at 06:07:01PM -0700, Matt Delco wrote:
> >> On Mon, Aug 19, 2019 at 5:37 PM Sean Christopherson <
> >> sean.j.christopherson@intel.com> wrote:
> >>
> >>> 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);
> >>>>
> >>>> 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.
> >>
> >> I looked at apic.c and test_apic_change_mode() might already be testing
> >> this.  It sets oneshot & TMICT, waits for the current value to get
> >> half-way, changes the mode to periodic, and then tries to test that the
> >> value wraps back to the upper half.  It then waits again for the half-way
> >> point, changes the mode back to oneshot, and waits for zero.  After
> >> reaching zero it does:
> >>
> >> /* now tmcct == 0 and tmict != 0 */
> >> apic_change_mode(APIC_LVT_TIMER_PERIODIC);
> >> report("TMCCT should stay at zero", !apic_read(APIC_TMCCT));
> >>
> >> which seems to be testing that oneshot->periodic won't reset the timer if
> >> it's already zero.  A possible caveat is there's hardly any delay between
> >> the mode change and the timer read.  Emulated hardware will react
> >> instantaneously (at least as seen from within the VM), but hardware might
> >> need more time to react (though offhand I'd expect HW to be fast enough for
> >> this particular timer).
> >>
> >> So, it looks like the code might already be ready to run on physical
> >> hardware, and if it has (or does already as part of a regular test), then
> >> that does raise some doubt on what's the appropriate code change to make
> >> this work.
> >
> > Nadav has been running tests on bare metal, maybe he can weigh in on
> > whether or not test_apic_change_mode() passes on bare metal.
>
> These tests pass on bare-metal.

Good to know this. In addition, in linux apic driver, during mode
switch __setup_APIC_LVTT() always sets lapic_timer_period(number of
clock cycles per jiffy)/APIC_DIVISOR to APIC_TMICT which can avoid the
issue Matt report. So is it because there is no such stuff in windows
or the windows version which Matt testing is too old?

Regards,
Wanpeng Li