From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: KVM: x86: disable master clock if TSC is reset during suspend
Date: Wed, 14 May 2014 07:05:56 -0300
Message-ID: <20140514100556.GA20451@amt.cnet>
References: <20140514072638.GA9285@amt.cnet>
 <537332D0.1090505@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>, Alexander Graf <agraf@suse.de>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:46966 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752857AbaENKGw (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 14 May 2014 06:06:52 -0400
Content-Disposition: inline
In-Reply-To: <537332D0.1090505@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.