Re: Bug in KVM clock backwards compensation

["Roedel, Joerg" <Joerg.Roedel@amd.com>]

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