Bug in KVM clock backwards compensation

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.

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.

Quoted 3 lines · Jan Kiszka

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.

Quoted 5 lines

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.

Yes, good question. Has this ever triggered on a real machine (not counting the suspend/resume issue in)?

Quoted 5 lines · Joerg

Quoted 3 lines · Jan Kiszka

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.

Quoted 8 lines

Quoted 5 lines

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.

Yes, good question. Has this ever triggered on a real machine (not counting the suspend/resume issue in)?

Yes... some platforms don't go haywire until you start using power mangement, TSC is stable before that, but not afterwards, and depending on the version of the kernel, KVM might detect this before the kernel does.

Honestly, the code is obsolete, but still useful for those who build KVM as an external module on older kernels using the kvm-kmod system.

Quoted 26 lines · Zachary Amsden

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.

Quoted 6 lines

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.

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.

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 saw a patch floating around that touched this code recently, but I think there's a definite issue here that needs addressing.

Quoted 21 lines · Zachary Amsden

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;
                  }

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 4 lines · Joerg

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.

That wasn't the intention when I wrote that code. It's simply there to detect backwards motion of the host TSC. The guest TSC can legally go backwards whenever the guest decides to change it, so checking the guest TSC doesn't make sense here.

Yes, with tsc-scaling, the machines already have stable TSCs - the above test is for older hardware which could have problems, and can be reverted back to the original code without worrying about switching units.

Quoted 8 lines · Zachary Amsden

Quoted 5 lines · Joerg

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.

Quoted 5 lines

That wasn't the intention when I wrote that code. It's simply there to detect backwards motion of the host TSC. The guest TSC can legally go backwards whenever the guest decides to change it, so checking the guest TSC doesn't make sense here.

This code checks how many guest tsc cycles have passed since this vCPU was de-scheduled last time (and before it is running again). So since the vCPU hasn't run in the meantime it had no chance to change its TSC. Further, the other parameters like the TSC offset and the scaling multiplier havn't changed too, so the only variable in the guest-tsc calculation is the host-tsc.
So this calculation using the guest-tsc can detect backwards going host-tsc as good as the old one. The benefit here is that we can feed consistent values into adjust_tsc_offset().

Quoted 5 lines

Yes, with tsc-scaling, the machines already have stable TSCs - the above test is for older hardware which could have problems, and can be reverted back to the original code without worrying about switching units.

This is the case pratically. But architecturally the tsc-scaling feature does not depend on a constant tsc, so we can make no such assumtions. Additionally, it may happen that Linux mis-detects an unstable tsc for some reason (broken BIOS, bug in the code, ...). Therefore I think it is dangerous to assume that this code will never run on tsc-scaling capable hosts. And if it does and we don't manage the tsc-offset units right, we may see very weird behavior.

Quoted 10 lines · Joerg Roedel

This code checks how many guest tsc cycles have passed since this vCPU was de-scheduled last time (and before it is running again). So since the vCPU hasn't run in the meantime it had no chance to change its TSC. Further, the other parameters like the TSC offset and the scaling multiplier havn't changed too, so the only variable in the guest-tsc calculation is the host-tsc.
So this calculation using the guest-tsc can detect backwards going host-tsc as good as the old one. The benefit here is that we can feed consistent values into adjust_tsc_offset().

While true, this is more complex than the original code. The original code here doesn't try to actually compensate for the guest TSC difference - instead what it does is NULL any discovered host TSC delta:

        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;
        }
        kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

Erasing that delta also erases elapsed time since the VCPU has last been run, which isn't desirable, so it then sets tsc_catchup mode, which will restore the proper TSC. The request here triggers code which later updates the TSC offset again.

I left compute_guest_tsc in place to recompute time in guest units here, even if the underlying hardware rate changes.

/*
* We may have to catch up the TSC to match elapsed wall clock
* time for two reasons, even if kvmclock is used.
* 1) CPU could have been running below the maximum TSC rate
* 2) Broken TSC compensation resets the base at each VCPU
* entry to avoid unknown leaps of TSC even when running
* again on the same CPU. This may cause apparent elapsed
* time to disappear, and the guest to stand still or run
* very slowly.
*/
if (vcpu->tsc_catchup) {
u64 tsc = compute_guest_tsc(v, kernel_ns); if (tsc > tsc_timestamp) {
kvm_x86_ops->adjust_tsc_offset(v, tsc -

tsc_timestamp);
                        tsc_timestamp = tsc;
                }
        }

So yeah, the code is getting pretty complex but we'd like to avoid that as much as possible - so I would prefer to have the hardware backwards compensation separate from the guest rate computation by doing this:

step 1) remove any backwards hardware TSC delta (in hardware units) step 2) recompute guest TSC from a stable clock (gotten from kernel_ns) and apply adjustment (in guest units)

So it appears you can just share most of the logic of guest TSC catchup mode.

What do you think?

Quoted 14 lines · Zachary Amsden

Quoted 11 lines · Joerg Roedel

This code checks how many guest tsc cycles have passed since this vCPU was de-scheduled last time (and before it is running again). So since the vCPU hasn't run in the meantime it had no chance to change its TSC. Further, the other parameters like the TSC offset and the scaling multiplier havn't changed too, so the only variable in the guest-tsc calculation is the host-tsc.
So this calculation using the guest-tsc can detect backwards going host-tsc as good as the old one. The benefit here is that we can feed consistent values into adjust_tsc_offset().

Quoted 4 lines

While true, this is more complex than the original code. The original code here doesn't try to actually compensate for the guest TSC difference - instead what it does is NULL any discovered host TSC delta:

Why should KVM care about the host-tsc going backwards when the guest-tsc moves forward? I think the bottom-line of this code is to make sure the guest-tsc does not jump around (or even jumps backwards).

I also don't agree that this is additional complexity. With these changes we were able to get rid of the last_host_tsc variable which is actually a simplification.

As I see it, there are two situations here:

1) On hosts without tsc-scaling the value of tsc_delta calculated from the guest-tsc values is the same as when calculated with host-tsc values, because the guest-tsc only differs by an offset from the host-tsc.

2) On hosts with tsc-scaling these two tsc_delta values may be different. If the guest has a different tsc-freq as the host, we can't simply adjust the tsc by an offset calculated from the host-tsc values. So tsc_delta has to be calculated using the guest-tsc.

Quoted 8 lines

         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;
         }
         kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

Quoted 5 lines

Erasing that delta also erases elapsed time since the VCPU has last been run, which isn't desirable, so it then sets tsc_catchup mode, which will restore the proper TSC. The request here triggers code which later updates the TSC offset again.

I just looked again at the tsc_catchup thing. Since the clock-update is forced in the code above, and the clock-update-code adjusts the tsc itself if necessary, is it really necessary to do this here at all?