Re: KVM: x86: disable master clock if TSC is reset during suspend

Marcelo Tosatti
2014-05-14T10:06:52Z

On Wed, May 14, 2014 at 11:09:36AM +0200, Paolo Bonzini wrote:
> Il 14/05/2014 09:26, Marcelo Tosatti ha scritto:
> >
> >Updating system_time from the kernel clock once master clock
> >has been enabled can result in time backwards event, in case
> >kernel clock frequency is lower than TSC frequency.
> >
> >Disable master clock in case its necessary to update it
> >from the resume path.
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index de0931c..9f5dd40 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
> > static u32 tsc_tolerance_ppm = 250;
> > module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
> >
> >+static bool suspend_backwards_tsc = false;
> >+
> > #define KVM_NR_SHARED_MSRS 16
> >
> > struct kvm_shared_msrs_global {
> >@@ -1472,6 +1474,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
> > &ka->master_cycle_now);
> >
> > ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
> >+ if (suspend_backwards_tsc)
> >+ ka->use_master_clock = 0;
>
> Use an && instead of an "if"? (Also, why "&" in the other
> assignment of ka->use_master_clock)?

I was asking myself the same.

> > if (ka->use_master_clock)
> > atomic_set(&kvm_guest_has_master_clock, 1);
> >@@ -6939,6 +6943,7 @@ int kvm_arch_hardware_enable(void *garbage)
> > */
> > if (backwards_tsc) {
> > u64 delta_cyc = max_tsc - local_tsc;
> >+ suspend_backwards_tsc = true;
> > list_for_each_entry(kvm, &vm_list, vm_list) {
> > kvm_for_each_vcpu(i, vcpu, kvm) {
> > vcpu->arch.tsc_offset_adjustment += delta_cyc;
> >
>
> Why a global and not a variable inside kvm? The same question holds
> for kvm_guest_has_master_clock.

Because these are global properties: reset-tsc-on-suspend is global,
kvm_guest_has_master_clock is global.