KVM: x86: disable master clock if TSC is reset during suspend

PATCH Marcelo Tosatti 3 participants 9 messages May 14, 2014 – May 21, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2014-05-14 07:41 UTC #0 lore raw

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.

Disable master clock in case its necessary to update it from the resume path.

Signed-off-by Marcelo Tosatti <[email protected]>

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

index de0931c..9f5dd40 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool suspend_backwards_tsc = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

@@ -1472,6 +1474,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_cycle_now);
 
 	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	if (suspend_backwards_tsc)
+		ka->use_master_clock = 0;
 
 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6943,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		suspend_backwards_tsc = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Paolo Bonzini

maintainer

2014-05-14 09:09 UTC #1 lore raw

Il 14/05/2014 09:26, Marcelo Tosatti ha scritto:

Quoted 24 lines

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.
Disable master clock in case its necessary to update it from the resume path.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +2 -0
index de0931c..9f5dd40 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool suspend_backwards_tsc = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

arch/x86/kvm/x86.c static void pvclock_update_vm_gtod_copy(struct kvm *kvm) · line 1478

Quoted 5 lines

arch/x86/kvm/x86.c +2 -0

@@ -1472,6 +1474,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_cycle_now);
 
 	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	if (suspend_backwards_tsc)
+		ka->use_master_clock = 0;

Use an && instead of an "if"? (Also, why "&" in the other assignment of ka->use_master_clock)?

arch/x86/kvm/x86.c int kvm_arch_hardware_enable(void *garbage) · line 6949

Quoted 11 lines

arch/x86/kvm/x86.c +1 -0

 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6943,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		suspend_backwards_tsc = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Why a global and not a variable inside kvm? The same question holds for kvm_guest_has_master_clock.

Paolo

Marcelo Tosatti

author

2014-05-14 10:06 UTC #2 lore raw

Quoted 27 lines · Paolo Bonzini

Il 14/05/2014 09:26, Marcelo Tosatti ha scritto:
Quoted 26 lines
Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.
Disable master clock in case its necessary to update it from the resume path.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +2 -0
index de0931c..9f5dd40 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool suspend_backwards_tsc = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {
@@ -1472,6 +1474,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_cycle_now);
 

Quoted 6 lines

arch/x86/kvm/x86.c static void pvclock_update_vm_gtod_copy(struct kvm *kvm) · line 1478
Quoted 3 lines
arch/x86/kvm/x86.c +2 -0
 	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	if (suspend_backwards_tsc)
+		ka->use_master_clock = 0;
Use an && instead of an "if"? (Also, why "&" in the other assignment of ka->use_master_clock)?

I was asking myself the same.

Quoted 6 lines

arch/x86/kvm/x86.c +0 -0

 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6943,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_arch_hardware_enable(void *garbage) · line 6949
Quoted 5 lines
arch/x86/kvm/x86.c +1 -0
+		suspend_backwards_tsc = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;
Why a global and not a variable inside kvm? The same question holds for kvm_guest_has_master_clock.

Because these are global properties: reset-tsc-on-suspend is global, kvm_guest_has_master_clock is global.

Marcelo Tosatti

author

2014-05-14 10:06 UTC #3 lore raw

KVM: x86: disable master clock if TSC is reset during suspend (v2)

In reply to Paolo Bonzini

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.

Disable master clock in case its necessary to update it from the resume path.

Signed-off-by Marcelo Tosatti <[email protected]>

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

index de0931c..32eec77 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool suspend_backwards_tsc = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_kernel_ns,
 					&ka->master_cycle_now);
 
-	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+				&& suspend_backwards_tsc;
 
 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		suspend_backwards_tsc = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Paolo Bonzini

maintainer

2014-05-14 10:21 UTC #4 lore raw

Re: KVM: x86: disable master clock if TSC is reset during suspend (v2)

Il 14/05/2014 12:06, Marcelo Tosatti ha scritto:

Quoted 24 lines

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.
Disable master clock in case its necessary to update it from the resume path.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +2 -0
index de0931c..32eec77 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool suspend_backwards_tsc = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

arch/x86/kvm/x86.c static void pvclock_update_vm_gtod_copy(struct kvm *kvm) · line 1477

Quoted 6 lines

arch/x86/kvm/x86.c +2 -1

@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_kernel_ns,
 					&ka->master_cycle_now);
 
-	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+				&& suspend_backwards_tsc;

Missing "!" here. Also, perhaps "backwards_tsc_observed" is a better name for the variable?

Paolo

Quoted 12 lines

arch/x86/kvm/x86.c +1 -0

 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		suspend_backwards_tsc = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Marcelo Tosatti

author

2014-05-14 15:43 UTC #5 lore raw

KVM: x86: disable master clock if TSC is reset during suspend (v3)

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.

Disable master clock in case its necessary to update it from the resume path.

Signed-off-by Marcelo Tosatti <[email protected]>

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

index de0931c..c098728 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool backwards_tsc_observed = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_kernel_ns,
 					&ka->master_cycle_now);
 
-	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+				&& !backwards_tsc_observed;
 
 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		backwards_tsc_observed = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Paolo Bonzini

maintainer

2014-05-14 16:00 UTC #6 lore raw

Re: KVM: x86: disable master clock if TSC is reset during suspend (v3)

Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:

Quoted 35 lines

Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.

Disable master clock in case its necessary to update it from the resume path.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +4 -1

index de0931c..c098728 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool backwards_tsc_observed = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {

@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_kernel_ns,
 					&ka->master_cycle_now);
 
-	ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+				&& !backwards_tsc_observed;
 
 	if (ka->use_master_clock)
 		atomic_set(&kvm_guest_has_master_clock, 1);

arch/x86/kvm/x86.c int kvm_arch_hardware_enable(void *garbage) · line 6948

Quoted 7 lines

arch/x86/kvm/x86.c +1 -0

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
+		backwards_tsc_observed = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;

Applying to kvm/master, thanks.

Paolo

Alexander Graf

2014-05-21 08:26 UTC #7 lore raw

Re: KVM: x86: disable master clock if TSC is reset during suspend (v3)

Quoted 33 lines · Paolo Bonzini

Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:
Quoted 32 lines
Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.
Disable master clock in case its necessary to update it from the resume path.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +4 -1
index de0931c..c098728 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool backwards_tsc_observed = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {
@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct 
 kvm *kvm)
                     &ka->master_kernel_ns,
                     &ka->master_cycle_now);
 
-    ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+    ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+                && !backwards_tsc_observed;
 

Quoted 13 lines

arch/x86/kvm/x86.c int kvm_arch_hardware_enable(void *garbage) · line 6948

Quoted 11 lines

arch/x86/kvm/x86.c +0 -0

     if (ka->use_master_clock)

         atomic_set(&kvm_guest_has_master_clock, 1);

arch/x86/kvm/x86.c +1 -0

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
      */
     if (backwards_tsc) {
         u64 delta_cyc = max_tsc - local_tsc;
+        backwards_tsc_observed = true;
         list_for_each_entry(kvm, &vm_list, vm_list) {
             kvm_for_each_vcpu(i, vcpu, kvm) {
                 vcpu->arch.tsc_offset_adjustment += delta_cyc;

Applying to kvm/master, thanks.

Thanks. Can you please make sure to CC qemu-stable when sending a pull request?

Alex

Paolo Bonzini

maintainer

2014-05-21 10:01 UTC #8 lore raw

Re: KVM: x86: disable master clock if TSC is reset during suspend (v3)

Il 21/05/2014 10:26, Alexander Graf ha scritto:

Quoted 49 lines · Paolo Bonzini

Quoted 33 lines

Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:
Quoted 32 lines
Updating system_time from the kernel clock once master clock has been enabled can result in time backwards event, in case kernel clock frequency is lower than TSC frequency.
Disable master clock in case its necessary to update it from the resume path.
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/x86.c +4 -1
index de0931c..c098728 100644
@@ -106,6 +106,8 @@ EXPORT_SYMBOL_GPL(kvm_max_guest_tsc_khz);
 static u32 tsc_tolerance_ppm = 250;
 module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 
+static bool backwards_tsc_observed = false;
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {
@@ -1471,7 +1473,8 @@ static void pvclock_update_vm_gtod_copy(struct
 kvm *kvm)
                     &ka->master_kernel_ns,
                     &ka->master_cycle_now);
 
-    ka->use_master_clock = host_tsc_clocksource & vcpus_matched;
+    ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+                && !backwards_tsc_observed;
 

Quoted 13 lines

arch/x86/kvm/x86.c int kvm_arch_hardware_enable(void *garbage) · line 6948

Quoted 11 lines

arch/x86/kvm/x86.c +0 -0

     if (ka->use_master_clock)

         atomic_set(&kvm_guest_has_master_clock, 1);

arch/x86/kvm/x86.c +1 -0

@@ -6939,6 +6942,7 @@ int kvm_arch_hardware_enable(void *garbage)
      */
     if (backwards_tsc) {
         u64 delta_cyc = max_tsc - local_tsc;
+        backwards_tsc_observed = true;
         list_for_each_entry(kvm, &vm_list, vm_list) {
             kvm_for_each_vcpu(i, vcpu, kvm) {
                 vcpu->arch.tsc_offset_adjustment += delta_cyc;

Applying to kvm/master, thanks.

Quoted 2 lines

Thanks. Can you please make sure to CC qemu-stable when sending a pull request?

Now I will.

Paolo :)