[Qemu-devel] Re: [PATCH] make tsc stable over migration and machine start

[Jan Kiszka]

On 2011-02-01 20:17, Glauber Costa wrote:
> If the machine is stopped, we should not record two different tsc values
> upon a save operation. The same problem happens with kvmclock.
> 
> But kvmclock is taking a different diretion, being now seen as a separate
> device. Since this is unlikely to happen with the tsc, I am taking the
> approach here of simply registering a handler for state change, and
> using a per-CPUState variable that prevents double updates for the TSC.
> 
> Signed-off-by: Glauber Costa <address@hidden>
> ---
>  target-i386/cpu.h |    1 +
>  target-i386/kvm.c |   19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 6d619e8..7f1c4f8 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -732,6 +732,7 @@ typedef struct CPUX86State {
>      uint32_t sipi_vector;
>      uint32_t cpuid_kvm_features;
>      uint32_t cpuid_svm_features;
> +    uint8_t  update_tsc;

bool please.

>      
>      /* in order to simplify APIC support, we leave this pointer to the
>         user */
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ecb8405..c3925be 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -302,6 +302,16 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank, 
> uint64_t status,
>  
>  static int _kvm_arch_init_vcpu(CPUState *env);
>  
> +static void cpu_update_state(void *opaque, int running, int reason)
> +{
> +    CPUState *env = opaque;
> +
> +    if (!running) {
> +        env->update_tsc = 1;
> +    }
> +}
> +
> +

Additional blank line.

>  int kvm_arch_init_vcpu(CPUState *env)
>  {
>      int r;
> @@ -444,6 +454,8 @@ int kvm_arch_init_vcpu(CPUState *env)
>      }
>  #endif
>  
> +    qemu_add_vm_change_state_handler(cpu_update_state, env);
> +
>      return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
>  }
>  
> @@ -1093,7 +1105,12 @@ static int kvm_get_msrs(CPUState *env)
>       msrs[n++].index = MSR_STAR;
>      if (kvm_has_msr_hsave_pa(env))
>          msrs[n++].index = MSR_VM_HSAVE_PA;
> -    msrs[n++].index = MSR_IA32_TSC;
> +
> +    if (env->update_tsc) {
> +        msrs[n++].index = MSR_IA32_TSC;
> +        env->update_tsc = 0;
> +    }
> +
>  #ifdef TARGET_X86_64
>      if (lm_capable_kernel) {
>          msrs[n++].index = MSR_CSTAR;

Not quite the logic I'm using for kvmclock:

cpu_update_state()
        if running
                tsc_valid = false;

kvm_get_msrs()
        ...
        if (!tsc_valid)
                read_tsc
                tsc_valid = !vm_running;

That ensure we always read the tsc while the VM is running, and not only
after it was stopped (might otherwise be "surprising" when once
visualizing the MSRs).

Jan

signature.asc
Description: OpenPGP digital signature