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

Paolo Bonzini
2019-08-19T23:42:37Z

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).

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.

Thanks,

Paolo

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