[PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[Ladi Prosek]

The backwards_tsc_observed global introduced in commit 16a9602 is never
reset to false. If a VM happens to be running while the host is suspended
(a common source of the TSC jumping backwards), master clock will never
be enabled again for any VM. In contrast, if no VM is running while the
host is suspended, master clock is unaffected. This is inconsistent and
unnecessarily strict. Let's track the backwards_tsc_observed variable
separately and let each VM start with a clean slate.

Real world impact: My Windows VMs get slower after my laptop undergoes a
suspend/resume cycle. The only way to get the perf back is unloading and
reloading the kvm module.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/x86.c              | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 695605e..d8259c3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -799,6 +799,7 @@ struct kvm_arch {
 	int audit_point;
 	#endif
 
+	bool backwards_tsc_observed;
 	bool boot_vcpu_runs_old_kvmclock;
 	u32 bsp_vcpu_id;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 87d3cb9..8586ec6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
 static bool __read_mostly vector_hashing = true;
 module_param(vector_hashing, bool, S_IRUGO);
 
-static bool __read_mostly backwards_tsc_observed = false;
-
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {
@@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 					&ka->master_cycle_now);
 
 	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
-				&& !backwards_tsc_observed
+				&& !ka->backwards_tsc_observed
 				&& !ka->boot_vcpu_runs_old_kvmclock;
 
 	if (ka->use_master_clock)
@@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
 	 */
 	if (backwards_tsc) {
 		u64 delta_cyc = max_tsc - local_tsc;
-		backwards_tsc_observed = true;
 		list_for_each_entry(kvm, &vm_list, vm_list) {
+			kvm->arch.backwards_tsc_observed = true;
 			kvm_for_each_vcpu(i, vcpu, kvm) {
 				vcpu->arch.tsc_offset_adjustment += delta_cyc;
 				vcpu->arch.last_host_tsc = local_tsc;
-- 
2.9.3

Re: [PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[Marcelo Tosatti]

Hi Ladi,

Can you disable masterclock if suspend-to-RAM is performed, in 
case backwards_tsc_observed was observed for any VM ? 
(which indicates its likely to happen again, backwards TSC). 

Because a backwards TSC event, with masterclock in place, is likely
to cause the guest to see time going backwards.

What masterclock requires is that TSCs are:

	1) In sync across CPUs and TSC clocksource is in use by the host.
	2) kvmclock regions updated on VM-entry, after a suspend from RAM (in case it 
	   goes backwards): kvm_gen_update_masterclock.

Is the first condition met?

On Mon, Jun 26, 2017 at 09:56:43AM +0200, Ladi Prosek wrote:
> The backwards_tsc_observed global introduced in commit 16a9602 is never
> reset to false. If a VM happens to be running while the host is suspended
> (a common source of the TSC jumping backwards), master clock will never
> be enabled again for any VM. In contrast, if no VM is running while the
> host is suspended, master clock is unaffected. This is inconsistent and
> unnecessarily strict. Let's track the backwards_tsc_observed variable
> separately and let each VM start with a clean slate.
> 
> Real world impact: My Windows VMs get slower after my laptop undergoes a
> suspend/resume cycle. The only way to get the perf back is unloading and
> reloading the kvm module.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/x86.c              | 6 ++----
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 695605e..d8259c3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -799,6 +799,7 @@ struct kvm_arch {
>  	int audit_point;
>  	#endif
>  
> +	bool backwards_tsc_observed;
>  	bool boot_vcpu_runs_old_kvmclock;
>  	u32 bsp_vcpu_id;
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 87d3cb9..8586ec6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
>  static bool __read_mostly vector_hashing = true;
>  module_param(vector_hashing, bool, S_IRUGO);
>  
> -static bool __read_mostly backwards_tsc_observed = false;
> -
>  #define KVM_NR_SHARED_MSRS 16
>  
>  struct kvm_shared_msrs_global {
> @@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
>  					&ka->master_cycle_now);
>  
>  	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
> -				&& !backwards_tsc_observed
> +				&& !ka->backwards_tsc_observed
>  				&& !ka->boot_vcpu_runs_old_kvmclock;
>  
>  	if (ka->use_master_clock)
> @@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
>  	 */
>  	if (backwards_tsc) {
>  		u64 delta_cyc = max_tsc - local_tsc;
> -		backwards_tsc_observed = true;
>  		list_for_each_entry(kvm, &vm_list, vm_list) {
> +			kvm->arch.backwards_tsc_observed = true;
>  			kvm_for_each_vcpu(i, vcpu, kvm) {
>  				vcpu->arch.tsc_offset_adjustment += delta_cyc;
>  				vcpu->arch.last_host_tsc = local_tsc;
> -- 
> 2.9.3

Re: [PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[Ladi Prosek]

Hi Marcelo,

On Thu, Jun 29, 2017 at 5:59 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> Hi Ladi,
>
> Can you disable masterclock if suspend-to-RAM is performed, in
> case backwards_tsc_observed was observed for any VM ?
> (which indicates its likely to happen again, backwards TSC).

Not quite sure I understand the question. High-level, I am not content
with the lasting effect of backwards_tsc_observed. If it's happened in
the past, it's likely to happen again, sure, but presumably the way we
handle it is correct, so it's not clear what remembering the flag buys
us. Having KVM behave differently by rmmod/insmod'ing it again, to
make it drop some host-wide state, is just ugly.

> Because a backwards TSC event, with masterclock in place, is likely
> to cause the guest to see time going backwards.

Got it. Hence the reasoning that guests whose time hasn't started
because they don't exist yet should be unaffected.

> What masterclock requires is that TSCs are:
>
>         1) In sync across CPUs and TSC clocksource is in use by the host.
>         2) kvmclock regions updated on VM-entry, after a suspend from RAM (in case it
>            goes backwards): kvm_gen_update_masterclock.
>
> Is the first condition met?

Yes, the first condition is met. What happens after resume is that in
pvclock_update_vm_gtod_copy when assigning to ka->use_master_clock,
all conditions except for !backwards_tsc_observed are met.

And because ka->use_master_clock is false, vcpu->hv_clock.flags
doesn't have the PVCLOCK_TSC_STABLE_BIT bit set (per the logic in
kvm_guest_time_update) and compute_tsc_page_parameters completely
gives up on updating the TSC page.

The next thing to look into was going to be what's wrong with this
chain of events. TSC jumping backwards is something we should be able
to compensate for in the TSC page so it's not clear why don't do it.
Maybe I started from the wrong end :/

Thanks!
Ladi

> On Mon, Jun 26, 2017 at 09:56:43AM +0200, Ladi Prosek wrote:
>> The backwards_tsc_observed global introduced in commit 16a9602 is never
>> reset to false. If a VM happens to be running while the host is suspended
>> (a common source of the TSC jumping backwards), master clock will never
>> be enabled again for any VM. In contrast, if no VM is running while the
>> host is suspended, master clock is unaffected. This is inconsistent and
>> unnecessarily strict. Let's track the backwards_tsc_observed variable
>> separately and let each VM start with a clean slate.
>>
>> Real world impact: My Windows VMs get slower after my laptop undergoes a
>> suspend/resume cycle. The only way to get the perf back is unloading and
>> reloading the kvm module.
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h | 1 +
>>  arch/x86/kvm/x86.c              | 6 ++----
>>  2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 695605e..d8259c3 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -799,6 +799,7 @@ struct kvm_arch {
>>       int audit_point;
>>       #endif
>>
>> +     bool backwards_tsc_observed;
>>       bool boot_vcpu_runs_old_kvmclock;
>>       u32 bsp_vcpu_id;
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 87d3cb9..8586ec6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
>>  static bool __read_mostly vector_hashing = true;
>>  module_param(vector_hashing, bool, S_IRUGO);
>>
>> -static bool __read_mostly backwards_tsc_observed = false;
>> -
>>  #define KVM_NR_SHARED_MSRS 16
>>
>>  struct kvm_shared_msrs_global {
>> @@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
>>                                       &ka->master_cycle_now);
>>
>>       ka->use_master_clock = host_tsc_clocksource && vcpus_matched
>> -                             && !backwards_tsc_observed
>> +                             && !ka->backwards_tsc_observed
>>                               && !ka->boot_vcpu_runs_old_kvmclock;
>>
>>       if (ka->use_master_clock)
>> @@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
>>        */
>>       if (backwards_tsc) {
>>               u64 delta_cyc = max_tsc - local_tsc;
>> -             backwards_tsc_observed = true;
>>               list_for_each_entry(kvm, &vm_list, vm_list) {
>> +                     kvm->arch.backwards_tsc_observed = true;
>>                       kvm_for_each_vcpu(i, vcpu, kvm) {
>>                               vcpu->arch.tsc_offset_adjustment += delta_cyc;
>>                               vcpu->arch.last_host_tsc = local_tsc;
>> --
>> 2.9.3

Re: [PATCH] KVM: x86: make backwards_tsc_observed a per-VM variable

[Marcelo Tosatti]

On Fri, Jun 30, 2017 at 10:20:28AM +0200, Ladi Prosek wrote:
> Hi Marcelo,
> 
> On Thu, Jun 29, 2017 at 5:59 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > Hi Ladi,
> >
> > Can you disable masterclock if suspend-to-RAM is performed, in
> > case backwards_tsc_observed was observed for any VM ?
> > (which indicates its likely to happen again, backwards TSC).
> 
> Not quite sure I understand the question. High-level, I am not content
> with the lasting effect of backwards_tsc_observed. If it's happened in
> the past, it's likely to happen again, sure, but presumably the way we
> handle it is correct, so it's not clear what remembering the flag buys
> us. Having KVM behave differently by rmmod/insmod'ing it again, to
> make it drop some host-wide state, is just ugly.

See points 1) and 2) below to answer the question.

> > Because a backwards TSC event, with masterclock in place, is likely
> > to cause the guest to see time going backwards.
> 
> Got it. Hence the reasoning that guests whose time hasn't started
> because they don't exist yet should be unaffected.

Sure.

> > What masterclock requires is that TSCs are:
> >
> >         1) In sync across CPUs and TSC clocksource is in use by the host.
> >         2) kvmclock regions updated on VM-entry, after a suspend from RAM (in case it
> >            goes backwards): kvm_gen_update_masterclock.
> >
> > Is the first condition met?
> 
> Yes, the first condition is met. What happens after resume is that in
> pvclock_update_vm_gtod_copy when assigning to ka->use_master_clock,
> all conditions except for !backwards_tsc_observed are met.
> 
> And because ka->use_master_clock is false, vcpu->hv_clock.flags
> doesn't have the PVCLOCK_TSC_STABLE_BIT bit set (per the logic in
> kvm_guest_time_update) and compute_tsc_page_parameters completely
> gives up on updating the TSC page.
> 
> The next thing to look into was going to be what's wrong with this
> chain of events. TSC jumping backwards is something we should be able
> to compensate for in the TSC page so it's not clear why don't do it.
> Maybe I started from the wrong end :/

Of condition 1 and 2 are met, just call kvm_gen_update_masterclock()
before resume and you should be fine (even for guests running before
S1 event).
> 
> Thanks!
> Ladi
> 
> > On Mon, Jun 26, 2017 at 09:56:43AM +0200, Ladi Prosek wrote:
> >> The backwards_tsc_observed global introduced in commit 16a9602 is never
> >> reset to false. If a VM happens to be running while the host is suspended
> >> (a common source of the TSC jumping backwards), master clock will never
> >> be enabled again for any VM. In contrast, if no VM is running while the
> >> host is suspended, master clock is unaffected. This is inconsistent and
> >> unnecessarily strict. Let's track the backwards_tsc_observed variable
> >> separately and let each VM start with a clean slate.
> >>
> >> Real world impact: My Windows VMs get slower after my laptop undergoes a
> >> suspend/resume cycle. The only way to get the perf back is unloading and
> >> reloading the kvm module.
> >>
> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> ---
> >>  arch/x86/include/asm/kvm_host.h | 1 +
> >>  arch/x86/kvm/x86.c              | 6 ++----
> >>  2 files changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >> index 695605e..d8259c3 100644
> >> --- a/arch/x86/include/asm/kvm_host.h
> >> +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -799,6 +799,7 @@ struct kvm_arch {
> >>       int audit_point;
> >>       #endif
> >>
> >> +     bool backwards_tsc_observed;
> >>       bool boot_vcpu_runs_old_kvmclock;
> >>       u32 bsp_vcpu_id;
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 87d3cb9..8586ec6 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -134,8 +134,6 @@ module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
> >>  static bool __read_mostly vector_hashing = true;
> >>  module_param(vector_hashing, bool, S_IRUGO);
> >>
> >> -static bool __read_mostly backwards_tsc_observed = false;
> >> -
> >>  #define KVM_NR_SHARED_MSRS 16
> >>
> >>  struct kvm_shared_msrs_global {
> >> @@ -1719,7 +1717,7 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
> >>                                       &ka->master_cycle_now);
> >>
> >>       ka->use_master_clock = host_tsc_clocksource && vcpus_matched
> >> -                             && !backwards_tsc_observed
> >> +                             && !ka->backwards_tsc_observed
> >>                               && !ka->boot_vcpu_runs_old_kvmclock;
> >>
> >>       if (ka->use_master_clock)
> >> @@ -7827,8 +7825,8 @@ int kvm_arch_hardware_enable(void)
> >>        */
> >>       if (backwards_tsc) {
> >>               u64 delta_cyc = max_tsc - local_tsc;
> >> -             backwards_tsc_observed = true;
> >>               list_for_each_entry(kvm, &vm_list, vm_list) {
> >> +                     kvm->arch.backwards_tsc_observed = true;
> >>                       kvm_for_each_vcpu(i, vcpu, kvm) {
> >>                               vcpu->arch.tsc_offset_adjustment += delta_cyc;
> >>                               vcpu->arch.last_host_tsc = local_tsc;
> >> --
> >> 2.9.3