From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: KVM: x86: fix tsc catchup issue with tsc scaling Date: Wed, 15 Jan 2014 13:55:50 +0100 Message-ID: <52D68556.8020303@redhat.com> References: <20140106141859.GA27260@amt.cnet> <52D67446.3020403@redhat.com> <20140115123444.GA27345@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: kvm-devel , Jeremy Fitzhardinge , Glauber Costa To: Marcelo Tosatti Return-path: Received: from mail-qe0-f43.google.com ([209.85.128.43]:60871 "EHLO mail-qe0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751632AbaAOMzz (ORCPT ); Wed, 15 Jan 2014 07:55:55 -0500 Received: by mail-qe0-f43.google.com with SMTP id nc12so954748qeb.16 for ; Wed, 15 Jan 2014 04:55:54 -0800 (PST) In-Reply-To: <20140115123444.GA27345@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: Il 15/01/2014 13:34, Marcelo Tosatti ha scritto: > On Wed, Jan 15, 2014 at 12:43:02PM +0100, Paolo Bonzini wrote: >> Il 06/01/2014 15:18, Marcelo Tosatti ha scritto: >>> >>> To fix a problem related to different resolution of TSC and system clock, >>> the offset in TSC units is approximated by >>> >>> delta = vcpu->hv_clock.tsc_timestamp - vcpu->last_guest_tsc >>> >>> (Guest TSC value at (Guest TSC value at last VM-exit) >>> the last kvm_guest_time_update >>> call) >>> >>> Delta is then later scaled using mult,shift pair found in hv_clock >>> structure (which is correct against tsc_timestamp in that >>> structure). >>> >>> However, if a frequency change is performed between these two points, >>> this delta is measured using different TSC frequencies, but scaled using >>> mult,shift pair for one frequency only. >>> >>> The end result is an incorrect delta. >>> >>> The bug which this code works around is not the only cause for >>> clock backwards events. The global accumulator is still >>> necessary, so remove the max_kernel_ns fix and rely on the >>> global accumulator for no clock backwards events. >> >> This is basically reverting commit 1d5f066 (KVM: x86: Fix a possible >> backwards warp of kvmclock, 2010-08-19). >> >> Your commit message basically says that the guest-side 489fb49 (x86, >> paravirt: Add a global synchronization point for pvclock, 2010-05-11) is >> the real solution to the problem that the host-side commit 1d5f066 was >> trying to fix. Right? >> >> This patch makes vcpu->hv_clock.tsc_timestamp write only. Please >> provide a follow up that drops the field entirely, then I'll apply both. >> In the meanwhile: >> >> Reviewed-by: Paolo Bonzini > > Can't do that: its inside hv_clock structure. 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; ? Paolo