Re: Bug in KVM clock backwards compensation

[Zachary Amsden <zamsden@redhat.com>]

On 04/28/2011 12:06 AM, Jan Kiszka wrote:
> On 2011-04-28 08:59, 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.
>>
>> However, the above code is fetching the guest TSC instead of the host
>> TSC, which isn't the way it is supposed to work.
>>
>> I saw a patch floating around that touched this code recently, but I
>> think there's a definite issue here that needs addressing.
>>      
> And /me still wonders (like I did when this first popped up) if the
> proper place of determining TSC stability really have to be KVM.
>    

No, it's not.  I have a patch which fixes that.  Was in the process of 
merging it into my local tree when I found this.

> If the Linux core fails to detect some instability and KVM has to jump
> in, shouldn't we better improve the core's detection abilities and make
> use of them in KVM? Conceptually this looks like we are currently just
> working around a core deficit in KVM.
>    

100% correct you are.

Zach