Re: [PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Jun 30, 2017 at 10:20:28AM +0200, Ladi Prosek wrote:
> Hi Marcelo,
> 
> On Thu, Jun 29, 2017 at 5:59 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > Hi Ladi,
> >
> > Can you disable masterclock if suspend-to-RAM is performed, in
> > case backwards_tsc_observed was observed for any VM ?
> > (which indicates its likely to happen again, backwards TSC).
> 
> Not quite sure I understand the question. High-level, I am not content
> with the lasting effect of backwards_tsc_observed. If it's happened in
> the past, it's likely to happen again, sure, but presumably the way we
> handle it is correct, so it's not clear what remembering the flag buys
> us. Having KVM behave differently by rmmod/insmod'ing it again, to
> make it drop some host-wide state, is just ugly.

See points 1) and 2) below to answer the question.

> > Because a backwards TSC event, with masterclock in place, is likely
> > to cause the guest to see time going backwards.
> 
> Got it. Hence the reasoning that guests whose time hasn't started
> because they don't exist yet should be unaffected.

Sure.

> > What masterclock requires is that TSCs are:
> >
> >         1) In sync across CPUs and TSC clocksource is in use by the host.
> >         2) kvmclock regions updated on VM-entry, after a suspend from RAM (in case it
> >            goes backwards): kvm_gen_update_masterclock.
> >
> > Is the first condition met?
> 
> Yes, the first condition is met. What happens after resume is that in
> pvclock_update_vm_gtod_copy when assigning to ka->use_master_clock,
> all conditions except for !backwards_tsc_observed are met.
> 
> And because ka->use_master_clock is false, vcpu->hv_clock.flags
> doesn't have the PVCLOCK_TSC_STABLE_BIT bit set (per the logic in
> kvm_guest_time_update) and compute_tsc_page_parameters completely
> gives up on updating the TSC page.
> 
> The next thing to look into was going to be what's wrong with this
> chain of events. TSC jumping backwards is something we should be able
> to compensate for in the TSC page so it's not clear why don't do it.
> Maybe I started from the wrong end :/

Of condition 1 and 2 are met, just call kvm_gen_update_masterclock()
before resume and you should be fine (even for guests running before
S1 event).
> 
> Thanks!
> Ladi
> 
> > On Mon, Jun 26, 2017 at 09:56:43AM +0200, Ladi Prosek wrote:
> >> The backwards_tsc_observed global introduced in commit 16a9602 is never
> >> reset to false. If a VM happens to be running while the host is suspended
> >> (a common source of the TSC jumping backwards), master clock will never
> >> be enabled again for any VM. In contrast, if no VM is running while the
> >> host is suspended, master clock is unaffected. This is inconsistent and
> >> unnecessarily strict. Let's track the backwards_tsc_observed variable
> >> separately and let each VM start with a clean slate.
> >>
> >> Real world impact: My Windows VMs get slower after my laptop undergoes a
> >> suspend/resume cycle. The only way to get the perf back is unloading and
> >> reloading the kvm module.
> >>
> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> ---
> >>  arch/x86/include/asm/kvm_host.h | 1 +
> >>  arch/x86/kvm/x86.c              | 6 ++----
> >>  2 files changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >> index 695605e..d8259c3 100644
> >> --- a/arch/x86/include/asm/kvm_host.h
> >> +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -799,6 +799,7 @@ struct kvm_arch {
> >>       int audit_point;
> >>       #endif
> >>
> >> +     bool backwards_tsc_observed;
> >>       bool boot_vcpu_runs_old_kvmclock;
> >>       u32 bsp_vcpu_id;
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 87d3cb9..8586ec6 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
> >>  static bool __read_mostly vector_hashing = true;
> >>  module_param(vector_hashing, bool, S_IRUGO);
> >>
> >> -static bool __read_mostly backwards_tsc_observed = false;
> >> -
> >>  #define KVM_NR_SHARED_MSRS 16
> >>
> >>  struct kvm_shared_msrs_global {
> >> @@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
> >>                                       &ka->master_cycle_now);
> >>
> >>       ka->use_master_clock = host_tsc_clocksource && vcpus_matched
> >> -                             && !backwards_tsc_observed
> >> +                             && !ka->backwards_tsc_observed
> >>                               && !ka->boot_vcpu_runs_old_kvmclock;
> >>
> >>       if (ka->use_master_clock)
> >> @@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
> >>        */
> >>       if (backwards_tsc) {
> >>               u64 delta_cyc = max_tsc - local_tsc;
> >> -             backwards_tsc_observed = true;
> >>               list_for_each_entry(kvm, &vm_list, vm_list) {
> >> +                     kvm->arch.backwards_tsc_observed = true;
> >>                       kvm_for_each_vcpu(i, vcpu, kvm) {
> >>                               vcpu->arch.tsc_offset_adjustment += delta_cyc;
> >>                               vcpu->arch.last_host_tsc = local_tsc;
> >> --
> >> 2.9.3