Re: [Qemu-devel] [PATCH] kvm: clear guest TSC on reset

Re: [Qemu-devel] [PATCH] kvm: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

I realized that the TSC reset should be done in QEMU
so I will be replying with a QEMU patch instead of a
KVM one.

- Fernando


On 12/03/2013 05:04 PM, Fernando Luis Vázquez Cao wrote:
> 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
>
>
> On 12/03/2013 04:08 PM, Fernando Luis Vázquez Cao wrote:
>> 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: stable@vger.kernel.org
>> Cc: Will Auld <will.auld@intel.com>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
>> ---
>>
>> 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
>> @@ -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;
>>
>>
>

[Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[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: stable@vger.kernel.org
Cc: Will Auld <will.auld@intel.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

--- qemu-orig/target-i386/kvm.c	2013-11-28 07:02:45.000000000 +0900
+++ qemu/target-i386/kvm.c	2013-12-05 14:47:03.085738175 +0900
@@ -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);

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 05/12/2013 07:15, Fernando Luis Vázquez Cao ha scritto:
> 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: stable@vger.kernel.org
> Cc: Will Auld <will.auld@intel.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>

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

> ---
> 
> --- qemu-orig/target-i386/kvm.c	2013-11-28 07:02:45.000000000 +0900
> +++ qemu/target-i386/kvm.c	2013-12-05 14:47:03.085738175 +0900
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vazquez Cao]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

(2013/12/05 18:28), Paolo Bonzini wrote:
> Il 05/12/2013 07:15, Fernando Luis Vázquez Cao ha scritto:
>> 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: stable@vger.kernel.org
>> Cc: Will Auld <will.auld@intel.com>
>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> 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.

Yes, that is certainly cleaner (I should try not to take shortcuts...). 
I am attaching
an updated patch (I apologize for not sending it inline - for reasons 
better left
untold I am writing this on a problematic email client :) ).



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

Can you pick my original fix first? I can do what you suggest in a follow-up
patch.

Thanks,
Fernando

[-- Attachment #2: target-i386-clear-guest-TSC-on-reset-v2.patch --]
[-- Type: text/plain, Size: 3041 bytes --]

[PATCH v2] target-i386: clear guest TSC on reset

From: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>

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: Will Auld <will.auld@intel.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp qemu-orig/target-i386/cpu.c qemu/target-i386/cpu.c
--- qemu-orig/target-i386/cpu.c	2013-11-28 07:02:45.000000000 +0900
+++ qemu/target-i386/cpu.c	2013-12-05 21:45:19.980156320 +0900
@@ -2446,6 +2446,9 @@ static void x86_cpu_reset(CPUState *s)
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
 
+    env->tsc_adjust = 0;
+    env->tsc = 0;
+
 #if !defined(CONFIG_USER_ONLY)
     /* We hard-wire the BSP to the first CPU. */
     if (s->cpu_index == 0) {
diff -urNp qemu-orig/target-i386/kvm.c qemu/target-i386/kvm.c
--- qemu-orig/target-i386/kvm.c	2013-11-28 07:02:45.000000000 +0900
+++ qemu/target-i386/kvm.c	2013-12-05 21:45:28.900200552 +0900
@@ -1139,22 +1139,20 @@ 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) {
         /*
          * 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);

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
>          /*
>           * 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);
>          }

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.

Though you would need one change here:

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

Instead you can make kvm_{,arch_}update_ticks() and pass
"cpu_get_ticks() + env->tsc_offset" to KVM_SET_MSRS (looping across all
VCPUs).  Assuming the TSC is synchronized to begin with on host CPUs,
and the latency is similar for all CPUs from the invocation of the ioctl
to the time TSC_OFFSET is written, the synchronization should be decent.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vazquez Cao]

(2013/12/05 22:53), Paolo Bonzini wrote:
> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
>>          /*
>>           * 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);
>>          }
> 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?


> 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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto:
> (2013/12/05 22:53), Paolo Bonzini wrote:
>> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
>>>          /*
>>>           * 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);
>>>          }
>> 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.

> 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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Thu, Dec 05, 2013 at 10:28:18AM +0100, Paolo Bonzini wrote:
> Il 05/12/2013 07:15, Fernando Luis Vázquez Cao ha scritto:
> > 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: stable@vger.kernel.org
> > Cc: Will Auld <will.auld@intel.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> 
> 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()

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.

Not sure what is the point of your idea.

> 
> Can you do this?
> 
> Thanks,
> 
> Paolo
> 
> > ---
> > 
> > --- qemu-orig/target-i386/kvm.c	2013-11-28 07:02:45.000000000 +0900
> > +++ qemu/target-i386/kvm.c	2013-12-05 14:47:03.085738175 +0900
> > @@ -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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Fri, Dec 06, 2013 at 12:42:44AM +0900, Fernando Luis Vazquez Cao wrote:
> (2013/12/05 22:53), Paolo Bonzini wrote:
> > Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
> >>          /*
> >>           * 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);
> >>          }
> > 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?

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.

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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 05/12/2013 17:12, Marcelo Tosatti ha scritto:
>> > - 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.

QEMU already tracks the TSC in cpu_get_ticks().  So far this is used
only for TCG, but for example the code is there that preserves the TSC
when you stop/resume the VM and when you migrate the VM.  Reset is not
yet there, which is a bug similar to the one Fernando is trying to solve
for KVM.

So, from QEMU's point of view the TSC should be a global value across
the whole system (timer_state.cpu_ticks_offset) + a per-VCPU TSC offset
(env->tsc_adjust).  When talking to KVM, the per-VCPU TSC offset in turn
has two parts, both set with KVM_SET_MSRS: one is computed from
MSR_IA32_TSC, the other comes from MSR_IA32_TSC_ADJUST.

The point here would be to treat it as such.

With this change, env->tsc need not be migrated.  The global value
timer_state.cpu_ticks_offset is migrated already.  The host-side TSC
adjust can be computed from rdtsc()-timer_state.cpu_ticks_offset on the
destination machine and/or at reset time.  The guest-side TSC adjust is
env->tsc_adjust as it is now.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:
>> > 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.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Thu, Dec 05, 2013 at 05:02:02PM +0100, Paolo Bonzini wrote:
> Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto:
> > (2013/12/05 22:53), Paolo Bonzini wrote:
> >> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
> >>>          /*
> >>>           * 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);
> >>>          }
> >> 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.
> 
> > 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

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.

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Thu, Dec 05, 2013 at 02:40:00PM -0200, Marcelo Tosatti wrote:
> On Thu, Dec 05, 2013 at 05:02:02PM +0100, Paolo Bonzini wrote:
> > Il 05/12/2013 16:42, Fernando Luis Vazquez Cao ha scritto:
> > > (2013/12/05 22:53), Paolo Bonzini wrote:
> > >> Il 05/12/2013 14:15, Fernando Luis Vazquez Cao ha scritto:
> > >>>          /*
> > >>>           * 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);
> > >>>          }
> > >> 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.
> > 
> > > 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
> 
> 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.

The TSCs should start synchronized, and remain synchronized across
reboot and migration for stable TSC host case.

It is not necessary to test the unstable TSC host case.

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

On 12/06/2013 01:38 AM, Paolo Bonzini wrote:
> Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:
>>>> 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.

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


> 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

[Qemu-devel] [PATCH 1//2 v3] target-i386: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

VCPU TSC is not cleared by a warm reset (*), which leaves some types of Linux
 guests (non-pvops guests and those with the kernel parameter no-kvmclock set)
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 such 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: Will Auld <will.auld@intel.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp qemu-orig/target-i386/cpu.c qemu/target-i386/cpu.c
--- qemu-orig/target-i386/cpu.c	2013-11-28 07:02:45.000000000 +0900
+++ qemu/target-i386/cpu.c	2013-12-05 21:45:19.980156320 +0900
@@ -2446,6 +2446,9 @@ static void x86_cpu_reset(CPUState *s)
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
 
+    env->tsc_adjust = 0;
+    env->tsc = 0;
+
 #if !defined(CONFIG_USER_ONLY)
     /* We hard-wire the BSP to the first CPU. */
     if (s->cpu_index == 0) {
diff -urNp qemu-orig/target-i386/kvm.c qemu/target-i386/kvm.c
--- qemu-orig/target-i386/kvm.c	2013-11-28 07:02:45.000000000 +0900
+++ qemu/target-i386/kvm.c	2013-12-05 21:45:28.900200552 +0900
@@ -1139,22 +1139,20 @@ 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) {
         /*
          * 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);

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 06/12/2013 09:24, Fernando Luis Vázquez Cao ha scritto:
> 
> 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.

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

Paolo

[Qemu-devel] [PATCH 2/2] target-i386: do not special case TSC writeback

[Fernando Luis Vázquez Cao]

Newer kernels are capable of synchronizing TSC values of multiple VCPUs
on writeback, but we were excluding the power up case, which is not needed
anymore.

Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp qemu-orig/target-i386/kvm.c qemu/target-i386/kvm.c
--- qemu-orig/target-i386/kvm.c	2013-12-06 16:12:03.201953736 +0900
+++ qemu/target-i386/kvm.c	2013-12-06 16:13:53.897955184 +0900
@@ -1144,15 +1144,7 @@ static int kvm_put_msrs(X86CPU *cpu, int
      * for normal writeback. Limit them to reset or full state updates.
      */
     if (level >= KVM_PUT_RESET_STATE) {
-        /*
-         * 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 || level == KVM_PUT_RESET_STATE) {
-            kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
-        }
+        kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
         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);

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

On 12/06/2013 05:36 PM, Paolo Bonzini wrote:
> Il 06/12/2013 09:24, Fernando Luis Vázquez Cao ha scritto:
>> 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.
> 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.

- Fernando

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 06/12/2013 09:56, Fernando Luis Vázquez Cao ha scritto:
>>>
>>>
>>> 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.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vazquez Cao]

On 2013年12月06日 18:08, Paolo Bonzini wrote:
> Il 06/12/2013 09:56, Fernando Luis Vázquez Cao ha scritto:
>>>>
>>>> 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...

Well it fixes and existing bug (without desynchronizing) and
I think it does not make the existing code uglier (it even gets
rid of one if statement! :) ). I guess the latter depends on
the eye of the beholder though.


> I'll try to　test it myself.

Thank you. Same here if time permits.

- Fernando

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Fri, Dec 06, 2013 at 05:24:18PM +0900, Fernando Luis Vázquez Cao wrote:
> On 12/06/2013 01:38 AM, Paolo Bonzini wrote:
> >Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:
> >>>>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.
> 
> 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)?

Unfortunately there is no clean way to detect kernels that support TSC
write synchronization (not directly at least).

However the combination of recent qemu, pre-2010 kernel, and no kvmclock
Linux guest can be ignored in my POV.

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

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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

On 12/06/2013 11:22 PM, Marcelo Tosatti wrote:
> On Fri, Dec 06, 2013 at 05:24:18PM +0900, Fernando Luis Vázquez Cao wrote:
>> 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.

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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Fernando Luis Vázquez Cao]

On 12/09/2013 05:50 PM, Fernando Luis Vázquez Cao wrote:
> On 12/06/2013 11:22 PM, Marcelo Tosatti wrote:
>> On Fri, Dec 06, 2013 at 05:24:18PM +0900, Fernando Luis Vázquez Cao 
>> wrote:
>>> 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.
>
> 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?

Thanks,
Fernando


> 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

Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Paolo Bonzini]

Il 12/12/2013 03:52, Fernando Luis Vázquez Cao ha scritto:
> On 12/09/2013 05:50 PM, Fernando Luis Vázquez Cao wrote:
>> On 12/06/2013 11:22 PM, Marcelo Tosatti wrote:
>>> On Fri, Dec 06, 2013 at 05:24:18PM +0900, Fernando Luis Vázquez Cao
>>> wrote:
>>>> 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.
>>
>> 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?

No, I now "swapped" them (reversed the order) and applied them to uq/master.

Paolo