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

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

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index de0931c..9f5dd40 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;

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

[Paolo Bonzini]

Il 14/05/2014 09:26, Marcelo Tosatti ha scritto:
>
> 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 <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de0931c..9f5dd40 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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;

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

>  	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

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

[Marcelo Tosatti]

On Wed, May 14, 2014 at 11:09:36AM +0200, Paolo Bonzini wrote:
> Il 14/05/2014 09:26, Marcelo Tosatti ha scritto:
> >
> >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 <mtosatti@redhat.com>
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index de0931c..9f5dd40 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -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;
> 
> Use an && instead of an "if"?  (Also, why "&" in the other
> assignment of ka->use_master_clock)?

I was asking myself the same.

> > 	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.

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

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

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index de0931c..32eec77 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;

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

[Paolo Bonzini]

Il 14/05/2014 12:06, Marcelo Tosatti ha scritto:
>
> 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 <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de0931c..32eec77 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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;

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

Paolo

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

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

[Marcelo Tosatti]

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 <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index de0931c..c098728 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;

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

[Paolo Bonzini]

Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:
>
> 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 <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de0931c..c098728 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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;
>

Applying to kvm/master, thanks.

Paolo

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

[Alexander Graf]

On 14.05.14 18:00, Paolo Bonzini wrote:
> Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:
>>
>> 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 <mtosatti@redhat.com>
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index de0931c..c098728 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -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;
>>
>
> Applying to kvm/master, thanks.

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


Alex

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

[Paolo Bonzini]

Il 21/05/2014 10:26, Alexander Graf ha scritto:
>
> On 14.05.14 18:00, Paolo Bonzini wrote:
>> Il 14/05/2014 17:43, Marcelo Tosatti ha scritto:
>>>
>>> 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 <mtosatti@redhat.com>
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index de0931c..c098728 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -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;
>>>
>>
>> Applying to kvm/master, thanks.
>
> Thanks. Can you please make sure to CC qemu-stable when sending a pull
> request?

Now I will.

Paolo :)