From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Roedel, Joerg" Subject: Re: Bug in KVM clock backwards compensation Date: Thu, 28 Apr 2011 09:13:16 +0200 Message-ID: <20110428071316.GG20365@amd.com> References: <4DB9106D.6040203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: kvm , Jan Kiszka To: Zachary Amsden Return-path: Received: from tx2ehsobe001.messaging.microsoft.com ([65.55.88.11]:16291 "EHLO TX2EHSOBE002.bigfish.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753963Ab1D1HNW (ORCPT ); Thu, 28 Apr 2011 03:13:22 -0400 Content-Disposition: inline In-Reply-To: <4DB9106D.6040203@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Apr 28, 2011 at 02:59:57AM -0400, Zachary Amsden wrote: > So I've been going over the new code changes to the TSC related code and > I don't like one particular set of changes. In particular, here: > > kvm_x86_ops->vcpu_load(vcpu, cpu); > if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) { > /* Make sure TSC doesn't go backwards */ > s64 tsc_delta; > u64 tsc; > > kvm_get_msr(vcpu, MSR_IA32_TSC, &tsc); > tsc_delta = !vcpu->arch.last_guest_tsc ? 0 : > tsc - vcpu->arch.last_guest_tsc; > > if (tsc_delta < 0) > mark_tsc_unstable("KVM discovered backwards TSC"); > if (check_tsc_unstable()) { > kvm_x86_ops->adjust_tsc_offset(vcpu, -tsc_delta); > vcpu->arch.tsc_catchup = 1; > } > > > The point of this code fragment is to test the host clock to see if it > is stable, because we may have just come back from an idle phase which > stopped the TSC, switched CPUs, or come back from a deep sleep state > which reset the host TSC. I see it different. This code wants to check if the _guest_ tsc moves forwared (or at least not backwards). So it is fully legitimate to just do this by reading the guest-tsc and compare it to the last one the guest had. > I saw a patch floating around that touched this code recently, but I > think there's a definite issue here that needs addressing. In fact, this change was done to address one of your concerns. You mentioned that the values passed to adjust_tsc_offset() were in unconsistent units in my first version of tsc-scaling. This was a right objection because one call-site used guest-tsc-units while the other used host-tsc-units. This change intended to fix that by using guest-tsc-units always for adjust_tsc_offset(). Not that the guest and the host tsc have the same units on current machines. But with tsc-scaling these units are different. Regards, Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo, Andrew Bowd Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632