Re: [Qemu-devel] [patch 2/2] target-i386: block migration and savevm if invariant tsc is exposed

[Eduardo Habkost]

On Tue, Apr 22, 2014 at 04:10:44PM -0300, Marcelo Tosatti wrote:
> Invariant TSC documentation mentions that "invariant TSC will run at a
> constant rate in all ACPI P-, C-. and T-states".
> 
> This is not the case if migration to a host with different TSC frequency 
> is allowed, or if savevm is performed. So block migration/savevm.
> 
> Also do not expose invariant tsc flag by default.

What do you mean "do not expose invtsc by default", exactly? It is
already not exposed by default because the default CPU model is qemu64
and qemu64 doesn't have it enabled.

> 
> Signed-off-by: Marcelo Tosatti <address@hidden>
> 
> Index: qemu-invariant-tsc/target-i386/kvm.c
> ===================================================================
> --- qemu-invariant-tsc.orig/target-i386/kvm.c
> +++ qemu-invariant-tsc/target-i386/kvm.c
> @@ -33,6 +33,8 @@
>  #include "exec/ioport.h"
>  #include <asm/hyperv.h>
>  #include "hw/pci/pci.h"
> +#include "migration/migration.h"
> +#include "qapi/qmp/qerror.h"
>  
>  //#define DEBUG_KVM
>  
> @@ -447,6 +449,8 @@ static bool hyperv_enabled(X86CPU *cpu)
>              cpu->hyperv_relaxed_timing);
>  }
>  
> +Error *invtsc_mig_blocker;
> +
>  #define KVM_MAX_CPUID_ENTRIES  100
>  
>  int kvm_arch_init_vcpu(CPUState *cs)
> @@ -702,6 +706,16 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                                    !!(c->ecx & CPUID_EXT_SMX);
>      }
>  
> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
> +    if (c && (c->edx & 1<<8)) {
> +        /* for migration */
> +        invtsc_mig_blocker = NULL;
> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");
> +        migrate_add_blocker(invtsc_mig_blocker);
> +        /* for savevm */
> +        vmstate_x86_cpu.unmigratable = 1;
> +    }
> +
>      cpuid_data.cpuid.padding = 0;
>      r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
>      if (r) {
> Index: qemu-invariant-tsc/target-i386/cpu-qom.h
> ===================================================================
> --- qemu-invariant-tsc.orig/target-i386/cpu-qom.h
> +++ qemu-invariant-tsc/target-i386/cpu-qom.h
> @@ -116,7 +116,7 @@ static inline X86CPU *x86_env_get_cpu(CP
>  #define ENV_OFFSET offsetof(X86CPU, env)
>  
>  #ifndef CONFIG_USER_ONLY
> -extern const struct VMStateDescription vmstate_x86_cpu;
> +extern struct VMStateDescription vmstate_x86_cpu;
>  #endif
>  
>  /**
> Index: qemu-invariant-tsc/target-i386/machine.c
> ===================================================================
> --- qemu-invariant-tsc.orig/target-i386/machine.c
> +++ qemu-invariant-tsc/target-i386/machine.c
> @@ -613,7 +613,7 @@ static const VMStateDescription vmstate_
>      }
>  };
>  
> -const VMStateDescription vmstate_x86_cpu = {
> +VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
>      .minimum_version_id = 3,
> Index: qemu-invariant-tsc/target-i386/cpu.c
> ===================================================================
> --- qemu-invariant-tsc.orig/target-i386/cpu.c
> +++ qemu-invariant-tsc/target-i386/cpu.c
> @@ -1230,6 +1230,8 @@ static void host_x86_cpu_initfn(Object *
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          FeatureWordInfo *wi = &feature_word_info[w];
> +        if (w == FEAT_8000_0007_EDX)
> +            continue;

This breaks the assumption that "-cpu host" contains all features that
can be enabled in a given host. IIRC, Igor even has plans to make
kvm_check_features_against_host() use the feature data from the "host"
CPU model instead of calling kvm_arch_get_supported_cpuid() directly.

-cpu host is already very likely to break migration, and I was already
willing to block migration when "-cpu host" was used. So I really don't
think that having migration prevented as a side-effect of using "-cpu
host" would be a problem.

In other words: what about simply dropping this hunk and letting invtsc
be enabled when using -cpu host?

>          env->features[w] =
>              kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
>                                           wi->cpuid_reg);
> 
> 

-- 
Eduardo