kvm: clear guest TSC on reset

VCPU TSC is not cleared by a warm reset (*), which leaves many Linux guests vulnerable to the overflow in cyc2ns_offset fixed by upstream commit 9993bc635d01a6ee7f6b833b4ee65ce7c06350b1 ("sched/x86: Fix overflow in cyc2ns_offset").

To put it in a nutshell, if a Linux guest without the patch above applied has been up more than 208 days and attempts a warm reset chances are that the newly booted kernel will panic or hang.

(*) Intel Xeon E5 processors show the same broken behavior due to
the errata "TSC is Not Affected by Warm Reset" (Intel® Xeon® Processor E5 Family Specification Update - August 2013): "The TSC (Time Stamp Counter MSR 10H) should be cleared on reset. Due to this erratum the TSC is not affected by warm reset."

Cc [email protected]

Cc Will Auld <[email protected]>

Signed-off-by Fernando Luis Vazquez Cao <[email protected]>

---

diff -urNp linux-3.13-rc2-orig/arch/x86/kvm/x86.c linux-3.13-rc2/arch/x86/kvm/x86.c

arch/x86/kvm/x86.c +15 -5

@@ -6716,18 +6716,24 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu
  	return r;
  }
 
-int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
+static void kvm_tsc_reset(struct kvm_vcpu *vcpu)
  {
-	int r;
  	struct msr_data msr;
 
-	r = vcpu_load(vcpu);
-	if (r)
-		return r;
  	msr.data = 0x0;
  	msr.index = MSR_IA32_TSC;
  	msr.host_initiated = true;
  	kvm_write_tsc(vcpu, &msr);
+}
+
+int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+	int r;
+
+	r = vcpu_load(vcpu);
+	if (r)
+		return r;
+	kvm_tsc_reset(vcpu);
  	vcpu_put(vcpu);
 
  	return r;

@@ -6770,6 +6776,10 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcp
 
  	kvm_pmu_reset(vcpu);
 
+	kvm_tsc_reset(vcpu);
+	if (guest_cpuid_has_tsc_adjust(vcpu))
+		vcpu->arch.ia32_tsc_adjust_msr = 0x0;
+
  	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
  	vcpu->arch.regs_avail = ~0;
  	vcpu->arch.regs_dirty = ~0;

I think there is a problem with the current patch, so please ignore for the moment. I will be replying with an update ASAP.

Sorry for the noise.

- Fernando

Quoted 64 lines · Fernando Luis Vázquez Cao

VCPU TSC is not cleared by a warm reset (*), which leaves many Linux guests vulnerable to the overflow in cyc2ns_offset fixed by upstream commit 9993bc635d01a6ee7f6b833b4ee65ce7c06350b1 ("sched/x86: Fix overflow in cyc2ns_offset").

To put it in a nutshell, if a Linux guest without the patch above applied has been up more than 208 days and attempts a warm reset chances are that the newly booted kernel will panic or hang.

(*) Intel Xeon E5 processors show the same broken behavior due to
the errata "TSC is Not Affected by Warm Reset" (Intel® Xeon® Processor E5 Family Specification Update - August 2013): "The TSC (Time Stamp Counter MSR 10H) should be cleared on reset. Due to this erratum the TSC is not affected by warm reset."

Cc [email protected]

Cc Will Auld <[email protected]>

Signed-off-by Fernando Luis Vazquez Cao <[email protected]>

---

diff -urNp linux-3.13-rc2-orig/arch/x86/kvm/x86.c linux-3.13-rc2/arch/x86/kvm/x86.c
--- linux-3.13-rc2-orig/arch/x86/kvm/x86.c 2013-11-30
05:57:14.000000000 +0900
+++ linux-3.13-rc2/arch/x86/kvm/x86.c 2013-12-03
14:51:53.747600839 +0900

arch/x86/kvm/x86.c +14 -3

@@ -6716,18 +6716,24 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu
      return r;
  }
  -int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
+static void kvm_tsc_reset(struct kvm_vcpu *vcpu)
  {
-    int r;
      struct msr_data msr;
  -    r = vcpu_load(vcpu);
-    if (r)
-        return r;
      msr.data = 0x0;
      msr.index = MSR_IA32_TSC;
      msr.host_initiated = true;
      kvm_write_tsc(vcpu, &msr);
+}
+
+int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+    int r;
+
+    r = vcpu_load(vcpu);
+    if (r)
+        return r;
+    kvm_tsc_reset(vcpu);
      vcpu_put(vcpu);
        return r;

@@ -6770,6 +6776,10 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcp
        kvm_pmu_reset(vcpu);
  +    kvm_tsc_reset(vcpu);
+    if (guest_cpuid_has_tsc_adjust(vcpu))
+        vcpu->arch.ia32_tsc_adjust_msr = 0x0;
+
      memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
      vcpu->arch.regs_avail = ~0;
      vcpu->arch.regs_dirty = ~0;
 

VCPU TSC is not cleared by a warm reset (*), which leaves many Linux guests vulnerable to the overflow in cyc2ns_offset fixed by upstream commit 9993bc635d01a6ee7f6b833b4ee65ce7c06350b1 ("sched/x86: Fix overflow in cyc2ns_offset").

To put it in a nutshell, if a Linux guest without the patch above applied has been up more than 208 days and attempts a warm reset chances are that the newly booted kernel will panic or hang.

(*) Intel Xeon E5 processors show the same broken behavior due to
the errata "TSC is Not Affected by Warm Reset" (Intel® Xeon® Processor E5 Family Specification Update - August 2013): "The TSC (Time Stamp Counter MSR 10H) should be cleared on reset. Due to this erratum the TSC is not affected by warm reset."

Cc [email protected]

Cc Will Auld <[email protected]>

Cc Marcelo Tosatti <[email protected]>

Signed-off-by Fernando Luis Vazquez Cao <[email protected]>

---

target-i386/kvm.c +10 -9

@@ -1125,6 +1125,8 @@ static int kvm_put_msrs(X86CPU *cpu, int
         kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave);
     }
     if (has_msr_tsc_adjust) {
+        if (level == KVM_PUT_RESET_STATE)
+            env->tsc_adjust = 0;
         kvm_msr_entry_set(&msrs[n++], MSR_TSC_ADJUST, env->tsc_adjust);
     }
     if (has_msr_misc_enable) {

@@ -1139,22 +1141,22 @@ static int kvm_put_msrs(X86CPU *cpu, int
         kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
     }
 #endif
-    if (level == KVM_PUT_FULL_STATE) {
+    /*
+     * The following MSRs have side effects on the guest or are too heavy
+     * for normal writeback. Limit them to reset or full state updates.
+     */
+    if (level >= KVM_PUT_RESET_STATE) {
+        if (level == KVM_PUT_RESET_STATE)
+            env->tsc = 0;
         /*
          * KVM is yet unable to synchronize TSC values of multiple VCPUs on
          * writeback. Until this is fixed, we only write the offset to SMP
          * guests after migration, desynchronizing the VCPUs, but avoiding
          * huge jump-backs that would occur without any writeback at all.
          */
-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }
-    }
-    /*
-     * The following MSRs have side effects on the guest or are too heavy
-     * for normal writeback. Limit them to reset or full state updates.
-     */
-    if (level >= KVM_PUT_RESET_STATE) {
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,
                           env->system_time_msr);
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
 
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Il 05/12/2013 07:15, Fernando Luis Vázquez Cao ha scritto:

Quoted 16 lines

VCPU TSC is not cleared by a warm reset (*), which leaves many Linux guests vulnerable to the overflow in cyc2ns_offset fixed by upstream commit 9993bc635d01a6ee7f6b833b4ee65ce7c06350b1 ("sched/x86: Fix overflow in cyc2ns_offset").

To put it in a nutshell, if a Linux guest without the patch above applied has been up more than 208 days and attempts a warm reset chances are that the newly booted kernel will panic or hang.

(*) Intel Xeon E5 processors show the same broken behavior due to
the errata "TSC is Not Affected by Warm Reset" (Intel® Xeon® Processor E5 Family Specification Update - August 2013): "The TSC (Time Stamp Counter MSR 10H) should be cleared on reset. Due to this erratum the TSC is not affected by warm reset."

Quoted 4 lines

Cc [email protected]

Cc Will Auld <[email protected]>

Cc Marcelo Tosatti <[email protected]>

Signed-off-by Fernando Luis Vazquez Cao <[email protected]>

I agree that the bug is in QEMU. One small nit in your patch is that you should reset env->tsc_adjust and env->tsc in x86_cpu_reset. This would already be pretty good.

However, a bigger problem is that env->tsc is a useless duplicate of "cpu_get_ticks() + env->tsc_adjust". It would be nice to drop env->tsc completely except for migration backwards compatibility. Thus you can:

- fill in env->tsc as mentioned above from target-i386/machine.c's cpu_pre_save function. This guarantees backwards compatibility.

- add a function cpu_set_ticks(int64_t ticks) to cpus.c. The function does nothing if use_icount is true, otherwise it needs to have (roughly) the opposite logic compared to cpu_get_ticks. You then call this function from x86_cpu_reset instead of setting env->tsc. You can similarly call this function from kvm_get_msrs.

- add a function kvm_set_ticks(int64_t ticks) to kvm-all.c and kvm-stub.c. For kvm-all.c it calls kvm_arch_set_ticks(CPUState *cpu, int64_t ticks) in target-*/kvm.c. The kvm_arch_set_tsc() function has a dummy implementation for all architectures except x86. For x86 it calls KVM_SET_MSRS passing "ticks + env->tsc_offset".

- call kvm_set_ticks() from cpu_set_ticks() and cpu_enable_ticks()

Can you do this?

- add a function kvm_set_ticks(int64_t ticks) to kvm-all.c and kvm-stub.c. For kvm-all.c it calls kvm_arch_set_ticks(CPUState *cpu, int64_t ticks) in target-*/kvm.c. The kvm_arch_set_tsc() function has a dummy implementation for all architectures except x86. For x86 it calls KVM_SET_MSRS passing "ticks + env->tsc_offset".

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:

target-i386/kvm.c static int kvm_put_msrs(X86CPU *cpu, int · line 1155

Quoted 10 lines

/*
* KVM is yet unable to synchronize TSC values of multiple VCPUs on
* writeback. Until this is fixed, we only write the offset to SMP
* guests after migration, desynchronizing the VCPUs, but avoiding
* huge jump-backs that would occur without any writeback at all. */

target-i386/kvm.c +1 -1

-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }

This is still a bit ugly, and desynchronizes the VCPUs on reset.

The main point of my outlined solution is that you only have one value that is tracked, not one per VCPU (which in the case of migration adds unpredictable latencies---for example due to emptying the migration buffers). We already save that value; all that's left is to use it instead of env->tsc.

Quoted 11 lines · Paolo Bonzini

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:

Quoted 10 lines

/*
* KVM is yet unable to synchronize TSC values of multiple VCPUs on
* writeback. Until this is fixed, we only write the offset to SMP
* guests after migration, desynchronizing the VCPUs, but avoiding
* huge jump-backs that would occur without any writeback at all. */

target-i386/kvm.c +1 -1

-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }

Quoted 1 line

This is still a bit ugly, and desynchronizes the VCPUs on reset.

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

I understand the benefits of what you are proposing but, since it is wider is scope and it would be more difficult to backport, I would prefer to implement it as a follow-up patch, unless you think that the current patch as a standalone fix does more harm than good.

Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto:

Quoted 12 lines

Quoted 11 lines · Paolo Bonzini

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:

Quoted 10 lines

/*
* KVM is yet unable to synchronize TSC values of multiple VCPUs on
* writeback. Until this is fixed, we only write the offset to SMP
* guests after migration, desynchronizing the VCPUs, but avoiding
* huge jump-backs that would occur without any writeback at all. */

target-i386/kvm.c +1 -1

-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }

Quoted 7 lines

Quoted 1 line

This is still a bit ugly, and desynchronizes the VCPUs on reset.

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

No, probably not.

Quoted 4 lines

I understand the benefits of what you are proposing but, since it is wider is scope and it would be more difficult to backport, I would prefer to implement it as a follow-up patch, unless you think that the current patch as a standalone fix does more harm than good.

It does some harm in that it introduces a case where KVM_PUT_RESET_STATE restores something, but KVM_PUT_FULL_STATE doesn't.

If it really usually works, there shouldn't be a need for this "if" statement at all.

Marcelo, what do you think?

Paolo

Quoted 31 lines · Paolo Bonzini

Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto:

Quoted 12 lines

Quoted 11 lines · Paolo Bonzini

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:

Quoted 10 lines

/*
* KVM is yet unable to synchronize TSC values of multiple VCPUs on
* writeback. Until this is fixed, we only write the offset to SMP
* guests after migration, desynchronizing the VCPUs, but avoiding
* huge jump-backs that would occur without any writeback at all. */

target-i386/kvm.c +1 -1

-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }

Quoted 7 lines

Quoted 1 line

This is still a bit ugly, and desynchronizes the VCPUs on reset.

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

No, probably not.

Quoted 4 lines

I understand the benefits of what you are proposing but, since it is wider is scope and it would be more difficult to backport, I would prefer to implement it as a follow-up patch, unless you think that the current patch as a standalone fix does more harm than good.

It does some harm in that it introduces a case where KVM_PUT_RESET_STATE restores something, but KVM_PUT_FULL_STATE doesn't.

Quoted 6 lines

If it really usually works, there shouldn't be a need for this "if" statement at all.

Marcelo, what do you think?

Paolo

Its OK to drop it, provided the following is tested on SMP guests:

1. initialization.
2. reboot.
3. migration.

With both stable and unstable TSC hosts (use wrmsr tool to write TSC on a given host CPU, to make it an unstable TSC host). (A=rdmsr ; sleep 1s; wrmsr A).

To make sure the code is not securing against a kvm_write_tsc cornercase.

Quoted 11 lines · Paolo Bonzini

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:

Quoted 10 lines

/*
* KVM is yet unable to synchronize TSC values of multiple VCPUs on
* writeback. Until this is fixed, we only write the offset to SMP
* guests after migration, desynchronizing the VCPUs, but avoiding
* huge jump-backs that would occur without any writeback at all. */

target-i386/kvm.c +1 -1

-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }

Quoted 1 line

This is still a bit ugly, and desynchronizes the VCPUs on reset.

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

Quoted 5 lines

The main point of my outlined solution is that you only have one value that is tracked, not one per VCPU (which in the case of migration adds unpredictable latencies---for example due to emptying the migration buffers). We already save that value; all that's left is to use it instead of env->tsc.

I understand the benefits of what you are proposing but, since it is wider is scope and it would be more difficult to backport, I would prefer to implement it as a follow-up patch, unless you think that the current patch as a standalone fix does more harm than good.

- Fernando

Quoted 5 lines

Quoted 5 lines

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

Right, modern kernels (see kvm_write_tsc) perform synchronization, so in theory the "KVM is yet unable to synchronize ..." code is not necessary anymore.

I vote for dropping the thing entirely.

Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:

Quoted 10 lines

Quoted 5 lines

Quoted 5 lines

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

Right, modern kernels (see kvm_write_tsc) perform synchronization, so in theory the "KVM is yet unable to synchronize ..." code is not necessary anymore.

I vote for dropping the thing entirely.

If it can be dropped entirely, I certainly have no problem with starting with a simple patch first.

Could we start with the patch that I already sent? It's been tested, it is conservative in the sense that it does the minimum necessary to fix an existing bug, and should be easy to backport. I will be replying to this email with an updated version that has a more appropriate and less scary patch description.

I will also be sending a patch that makes the TSC writeback unconditional, but this one should probably be kept on hold until it is properly tested.

Il 06/12/2013 09:24, Fernando Luis Vázquez Cao ha scritto:

Quoted 7 lines

Could we start with the patch that I already sent? It's been tested, it is conservative in the sense that it does the minimum necessary to fix an existing bug, and should be easy to backport. I will be replying to this email with an updated version that has a more appropriate and less scary patch description.

Quoted 3 lines

I will also be sending a patch that makes the TSC writeback unconditional, but this one should probably be kept on hold until it is properly tested.

If you test it, I can drop the "if" myself from your patch.

Quoted 6 lines

Quoted 5 lines

I will also be sending a patch that makes the TSC writeback unconditional, but this one should probably be kept on hold until it is properly tested.

If you test it, I can drop the "if" myself from your patch.

Unfortunately I will not have time to test patch 2 properly for a while, so I think it would make sense to merge patch 1 first.

Il 06/12/2013 09:56, Fernando Luis Vázquez Cao ha scritto:

Quoted 8 lines

Quoted 5 lines

Quoted 4 lines

I will also be sending a patch that makes the TSC writeback unconditional, but this one should probably be kept on hold until it is properly tested.

If you test it, I can drop the "if" myself from your patch.

Unfortunately I will not have time to test patch 2 properly for a while, so I think it would make sense to merge patch 1 first.

As I said, I'm not sure that patch 1 is an improvement...

I'll try to　test it myself.

Quoted 10 lines · Paolo Bonzini

Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:

Quoted 9 lines

Quoted 5 lines

Quoted 5 lines

I agree it is a bit ugly, but in my testing QEMU seemed to loop over all the VCPUS fast enough for the kernel side kvm_write_tsc() to do a reasonable job of matching the offsets (the Linux guest did not mark the TSC unstable due to the TSCs being unsynchronized). Am I missing something?

Right, modern kernels (see kvm_write_tsc) perform synchronization, so in theory the "KVM is yet unable to synchronize ..." code is not necessary anymore.

Quoted 1 line

Quoted 1 line

I vote for dropping the thing entirely.

When I was writing the original patch I was tempted to do that, but I feared that it could break older kernels that do not have TSC synchronization code. Should we care about such uses (recent QEMU user space + old kernel)?

I also wanted to make sure that the initialization that we do in kvm_arch_vcpu_postcreate on power up and the subsequent TSC writeback work well together, but I didn't have time to test it (reading the code, I would say that the TSC generation counter may end up being increased a few times but the TSCs would eventually converge).

Quoted 2 lines

If it can be dropped entirely, I certainly have no problem with starting with a simple patch first.

Could we start with the patch that I already sent? It's been tested, it is conservative in the sense that it does the minimum necessary to fix an existing bug, and should be easy to backport. I will be replying to this email with an updated version that has a more appropriate and less scary patch description.

I will also be sending a patch that makes the TSC writeback unconditional, but this one should probably be kept on hold until it is properly tested.

As a follow-up effort we can work on Paolo's suggestions.

Is this an acceptable way forward?

Thanks,
Fernando

Quoted 6 lines · Fernando Luis Vázquez Cao

I also wanted to make sure that the initialization that we do in kvm_arch_vcpu_postcreate on power up and the subsequent TSC writeback work well together, but I didn't have time to test it (reading the code, I would say that the TSC generation counter may end up being increased a few times but the TSCs would eventually converge).

A basic test should be fine, because the matching code is in use today.

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines · Fernando Luis Vázquez Cao

I also wanted to make sure that the initialization that we do in kvm_arch_vcpu_postcreate on power up and the subsequent TSC writeback work well together, but I didn't have time to test it (reading the code, I would say that the TSC generation counter may end up being increased a few times but the TSCs would eventually converge).

Quoted 2 lines

A basic test should be fine, because the matching code is in use today.

I applied my two patches to QEMU and I did some testing with SMP guests (4 VCPUs).

When the host's TSC is stable all the TSC offsets are matched as expected:

   [ 4544.779699] kvm: VCPU 0 new tsc generation 1, clock 0
   [ 4544.799691] kvm: VCPU 1 matched tsc offset for 0
   [ 4544.835687] kvm: VCPU 2 matched tsc offset for 0
   [ 4544.882229] kvm: VCPU 3 matched tsc offset for 0
   [ 4544.983740] kvm: VCPU 0 matched tsc offset for 0
   [ 4545.015727] kvm: VCPU 1 matched tsc offset for 0
   [ 4545.031762] kvm: VCPU 2 matched tsc offset for 0
   [ 4545.043756] kvm: VCPU 3 matched tsc offset for 0
   [ 4545.382113] kvm: VCPU 0 matched tsc offset for 0
   [ 4545.382138] kvm: VCPU 1 matched tsc offset for 0
   [ 4545.382155] kvm: VCPU 2 matched tsc offset for 0
   [ 4545.382171] kvm: VCPU 3 matched tsc offset for 0

Regarding unstable TSC hosts, I created one by executing

   TSCBSP=`rdmsr -d -p 0 16`; sleep 1s; wrmsr -p 0 16 $TSCBSP

as you suggested. After doing this KVM will adjust the TSC offsets to make up for the deltas on the host side:

   [ 5232.172074] kvm: VCPU 0 new tsc generation 1, clock 0
   [ 5232.180759] kvm: SMP vm created on host with unstable TSC; guest 
TSC will not be reliable
   [ 5232.204069] kvm: VCPU 1 adjusted tsc offset by 105344
   [ 5232.268070] kvm: VCPU 2 adjusted tsc offset by 210721
   [ 5232.332066] kvm: VCPU 3 adjusted tsc offset by 210708
   [ 5232.444127] kvm: VCPU 0 adjusted tsc offset by 368959
   [ 5232.458448] kvm: VCPU 1 adjusted tsc offset by 47158
   [ 5232.470400] kvm: VCPU 2 adjusted tsc offset by 39352
   [ 5232.482359] kvm: VCPU 3 adjusted tsc offset by 39371
   [ 5232.875343] kvm: VCPU 0 adjusted tsc offset by 1293878
   [ 5232.875371] kvm: VCPU 1 adjusted tsc offset by 121
   [ 5232.875392] kvm: VCPU 2 adjusted tsc offset by 69
   [ 5232.875412] kvm: VCPU 3 adjusted tsc offset by 62

But despite KVM's efforts on the guest side the check in check_tsc_sync_source() fails and the TSC is marked unstable:

tsc: Marking TSC unstable due to check_tsc_sync_source failed

By the way, without my patches applied to QEMU the end result is the same:

   [  266.300068] kvm: VCPU 0 new tsc generation 1, clock 0
   [  266.308746] kvm: SMP vm created on host with unstable TSC; guest 
TSC will not be reliable
   [  266.332067] kvm: VCPU 1 adjusted tsc offset by 105354
   [  266.396065] kvm: VCPU 2 adjusted tsc offset by 210714
   [  266.428273] kvm: VCPU 3 adjusted tsc offset by 106045

In other words, things seem to be working as expected.

- Fernando

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines

Quoted 6 lines · Fernando Luis Vázquez Cao

I also wanted to make sure that the initialization that we do in kvm_arch_vcpu_postcreate on power up and the subsequent TSC writeback work well together, but I didn't have time to test it (reading the code, I would say that the TSC generation counter may end up being increased a few times but the TSCs would eventually converge).

Quoted 5 lines

Quoted 2 lines

A basic test should be fine, because the matching code is in use today.

I applied my two patches to QEMU and I did some testing with SMP guests (4 VCPUs).

By the way, do you want me to merge the two patches I sent into one and resend?

Il 05/12/2013 07:15, Fernando Luis Vázquez Cao ha scritto:

Quoted 16 lines

VCPU TSC is not cleared by a warm reset (*), which leaves many Linux guests vulnerable to the overflow in cyc2ns_offset fixed by upstream commit 9993bc635d01a6ee7f6b833b4ee65ce7c06350b1 ("sched/x86: Fix overflow in cyc2ns_offset").

To put it in a nutshell, if a Linux guest without the patch above applied has been up more than 208 days and attempts a warm reset chances are that the newly booted kernel will panic or hang.

(*) Intel Xeon E5 processors show the same broken behavior due to
the errata "TSC is Not Affected by Warm Reset" (Intel® Xeon® Processor E5 Family Specification Update - August 2013): "The TSC (Time Stamp Counter MSR 10H) should be cleared on reset. Due to this erratum the TSC is not affected by warm reset."

Quoted 4 lines

Cc [email protected]

Cc Will Auld <[email protected]>

Cc Marcelo Tosatti <[email protected]>

Signed-off-by Fernando Luis Vazquez Cao <[email protected]>

I agree that the bug is in QEMU. One small nit in your patch is that you should reset env->tsc_adjust and env->tsc in x86_cpu_reset. This would already be pretty good.

However, a bigger problem is that env->tsc is a useless duplicate of "cpu_get_ticks() + env->tsc_adjust". It would be nice to drop env->tsc completely except for migration backwards compatibility. Thus you can:

- fill in env->tsc as mentioned above from target-i386/machine.c's cpu_pre_save function. This guarantees backwards compatibility.

- add a function cpu_set_ticks(int64_t ticks) to cpus.c. The function does nothing if use_icount is true, otherwise it needs to have (roughly) the opposite logic compared to cpu_get_ticks. You then call this function from x86_cpu_reset instead of setting env->tsc. You can similarly call this function from kvm_get_msrs.

- add a function kvm_set_ticks(int64_t ticks) to kvm-all.c and kvm-stub.c. For kvm-all.c it calls kvm_arch_set_ticks(CPUState *cpu, int64_t ticks) in target-*/kvm.c. The kvm_arch_set_tsc() function has a dummy implementation for all architectures except x86. For x86 it calls KVM_SET_MSRS passing "ticks + env->tsc_offset".

- call kvm_set_ticks() from cpu_set_ticks() and cpu_enable_ticks()

Can you do this?

Thanks,

Paolo

Quoted 51 lines

---

target-i386/kvm.c +10 -9

@@ -1125,6 +1125,8 @@ static int kvm_put_msrs(X86CPU *cpu, int
         kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave);
     }
     if (has_msr_tsc_adjust) {
+        if (level == KVM_PUT_RESET_STATE)
+            env->tsc_adjust = 0;
         kvm_msr_entry_set(&msrs[n++], MSR_TSC_ADJUST, env->tsc_adjust);
     }
     if (has_msr_misc_enable) {

@@ -1139,22 +1141,22 @@ static int kvm_put_msrs(X86CPU *cpu, int
         kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
     }
 #endif
-    if (level == KVM_PUT_FULL_STATE) {
+    /*
+     * The following MSRs have side effects on the guest or are too heavy
+     * for normal writeback. Limit them to reset or full state updates.
+     */
+    if (level >= KVM_PUT_RESET_STATE) {
+        if (level == KVM_PUT_RESET_STATE)
+            env->tsc = 0;
         /*
          * KVM is yet unable to synchronize TSC values of multiple VCPUs on
          * writeback. Until this is fixed, we only write the offset to SMP
          * guests after migration, desynchronizing the VCPUs, but avoiding
          * huge jump-backs that would occur without any writeback at all.
          */
-        if (smp_cpus == 1 || env->tsc != 0) {
+        if (smp_cpus == 1 || env->tsc != 0 || level == KVM_PUT_RESET_STATE) {
             kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         }
-    }
-    /*
-     * The following MSRs have side effects on the guest or are too heavy
-     * for normal writeback. Limit them to reset or full state updates.
-     */
-    if (level >= KVM_PUT_RESET_STATE) {
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,
                           env->system_time_msr);
         kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
 
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line

Quoted 1 line

- call kvm_set_ticks() from cpu_set_ticks() and cpu_enable_ticks()

env->tsc is just a placeholder for the vcpu TSC.

A vcpus TSC from QEMU's point of view is a register initialized to zero, which requires read/write from KVM, and migration.