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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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
