From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: KVM: x86: fix tsc catchup issue with tsc scaling
Date: Wed, 15 Jan 2014 14:55:04 -0200
Message-ID: <20140115165504.GA8301@amt.cnet>
References: <20140106141859.GA27260@amt.cnet>
 <52D67446.3020403@redhat.com>
 <20140115123444.GA27345@amt.cnet>
 <52D68556.8020303@redhat.com>
 <20140115163758.GA7161@amt.cnet>
 <52D6BD02.5050807@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>,
	Jeremy Fitzhardinge <jeremy@goop.org>,
	Glauber Costa <glommer@gmail.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:3589 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751776AbaAOQzU (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 15 Jan 2014 11:55:20 -0500
Content-Disposition: inline
In-Reply-To: <52D6BD02.5050807@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Wed, Jan 15, 2014 at 05:53:22PM +0100, Paolo Bonzini wrote:
> Il 15/01/2014 17:37, Marcelo Tosatti ha scritto:
> > > Right.  Another question, what about this comment:
> > > 
> > >         /* Reset of TSC must disable overshoot protection below */
> > >         vcpu->arch.hv_clock.tsc_timestamp = 0;
> > >         vcpu->arch.last_guest_tsc = data;
> > > 
> > > Should it be instead like this:
> > > 
> > > 	vcpu->arch.hv_clock.tsc_timestamp += data - vcpu->arch.last_guest_tsc;
> > > 	vcpu->arch.last_guest_tsc = data;
> > > 
> > > ?
> > 
> > Don't see why it should?
> 
> Setting tsc_timestamp to 0 makes no sense with the current code, I'm
> trying to understand if the right fix is to delete that line or
> something else.  What I proposed (plus a version increase) would make a
> pvclock read continuous before and after the change.
> 
> But looking more at the surrounding code, the calls to kvm_write_tsc
> ultimately result in a master clock update as soon as all TSCs agree and
> the master clock is re-enabled.  This master clock update rewrites
> tsc_timestamp.  Then I think that the line that sets tsc_timestamp to 0
> can be deleted.

ACK.