[patch 0/2] expose invariant tsc flag for kvm guests

[patch 0/2] expose invariant tsc flag for kvm guests

[Marcelo Tosatti]

see patches for details.

[patch 1/2] target-i386: support "invariant tsc" flag

[Marcelo Tosatti]

[-- Attachment #1: 01_qemu-cpuid-invariant.patch --]
[-- Type: TEXT/PLAIN, Size: 4214 bytes --]

Expose "Invariant TSC" flag, if KVM is enabled. From Intel documentation:

17.13.1 Invariant TSC The time stamp counter in newer processors may
support an enhancement, referred to as invariant TSC. Processor’s
support for invariant TSC is indicated by CPUID.80000007H:EDX[8].
The invariant TSC will run at a constant rate in all ACPI P-, C-.
and T-states. This is the architectural behavior moving forward. On
processors with invariant TSC support, the OS may use the TSC for wall
clock timer services (instead of ACPI or HPET timers). TSC reads are
much more efficient and do not incur the overhead associated with a ring
transition or access to a platform resource.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-invariant-tsc/target-i386/cpu.c
===================================================================
--- qemu-invariant-tsc.orig/target-i386/cpu.c
+++ qemu-invariant-tsc/target-i386/cpu.c
@@ -262,6 +262,17 @@ static const char *cpuid_7_0_ebx_feature
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+static const char *cpuid_apm_edx_feature_name[] = {
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    "invtsc", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 typedef struct FeatureWordInfo {
     const char **feat_names;
     uint32_t cpuid_eax;   /* Input EAX for CPUID */
@@ -305,6 +316,11 @@ static FeatureWordInfo feature_word_info
         .cpuid_needs_ecx = true, .cpuid_ecx = 0,
         .cpuid_reg = R_EBX,
     },
+    [FEAT_8000_0007_EDX] = {
+        .feat_names = cpuid_apm_edx_feature_name,
+        .cpuid_eax = 0x80000007,
+        .cpuid_reg = R_EDX,
+    },
 };
 
 typedef struct X86RegisterInfo32 {
@@ -1740,6 +1756,7 @@ static void x86_cpu_parse_featurestr(CPU
     env->features[FEAT_1_ECX] |= plus_features[FEAT_1_ECX];
     env->features[FEAT_8000_0001_EDX] |= plus_features[FEAT_8000_0001_EDX];
     env->features[FEAT_8000_0001_ECX] |= plus_features[FEAT_8000_0001_ECX];
+    env->features[FEAT_8000_0007_EDX] |= plus_features[FEAT_8000_0007_EDX];
     env->features[FEAT_C000_0001_EDX] |= plus_features[FEAT_C000_0001_EDX];
     env->features[FEAT_KVM] |= plus_features[FEAT_KVM];
     env->features[FEAT_SVM] |= plus_features[FEAT_SVM];
@@ -1748,6 +1765,7 @@ static void x86_cpu_parse_featurestr(CPU
     env->features[FEAT_1_ECX] &= ~minus_features[FEAT_1_ECX];
     env->features[FEAT_8000_0001_EDX] &= ~minus_features[FEAT_8000_0001_EDX];
     env->features[FEAT_8000_0001_ECX] &= ~minus_features[FEAT_8000_0001_ECX];
+    env->features[FEAT_8000_0007_EDX] &= ~minus_features[FEAT_8000_0007_EDX];
     env->features[FEAT_C000_0001_EDX] &= ~minus_features[FEAT_C000_0001_EDX];
     env->features[FEAT_KVM] &= ~minus_features[FEAT_KVM];
     env->features[FEAT_SVM] &= ~minus_features[FEAT_SVM];
@@ -2333,6 +2351,17 @@ void cpu_x86_cpuid(CPUX86State *env, uin
                (AMD_ENC_ASSOC(L3_ASSOCIATIVITY) << 12) | \
                (L3_LINES_PER_TAG << 8) | (L3_LINE_SIZE);
         break;
+    case 0x80000007:
+        *eax = 0;
+        *ebx = 0;
+        *ecx = 0;
+
+        if (kvm_enabled()) {
+            *edx = env->features[FEAT_8000_0007_EDX];
+        } else {
+            *edx = 0;
+        }
+        break;
     case 0x80000008:
         /* virtual & phys address size in low 2 bytes. */
 /* XXX: This value must match the one used in the MMU code. */
Index: qemu-invariant-tsc/target-i386/cpu.h
===================================================================
--- qemu-invariant-tsc.orig/target-i386/cpu.h
+++ qemu-invariant-tsc/target-i386/cpu.h
@@ -398,6 +398,7 @@ typedef enum FeatureWord {
     FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
     FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
     FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
+    FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
     FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
     FEAT_KVM,           /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
     FEAT_SVM,           /* CPUID[8000_000A].EDX */

Re: [patch 1/2] target-i386: support "invariant tsc" flag

[Paolo Bonzini]

Il 22/04/2014 15:10, Marcelo Tosatti ha scritto:
> +    case 0x80000007:
> +        *eax = 0;
> +        *ebx = 0;
> +        *ecx = 0;
> +
> +        if (kvm_enabled()) {
> +            *edx = env->features[FEAT_8000_0007_EDX];
> +        } else {
> +            *edx = 0;
> +        }

I think TCG is able to eliminate unsupported features too before they 
get in env->features[].  Or are those patches not in yet?  Eduardo?

Paolo

Re: [patch 1/2] target-i386: support "invariant tsc" flag

[Eduardo Habkost]

On Tue, Apr 22, 2014 at 08:26:36PM -0400, Paolo Bonzini wrote:
> Il 22/04/2014 15:10, Marcelo Tosatti ha scritto:
> >+    case 0x80000007:
> >+        *eax = 0;
> >+        *ebx = 0;
> >+        *ecx = 0;
> >+
> >+        if (kvm_enabled()) {
> >+            *edx = env->features[FEAT_8000_0007_EDX];
> >+        } else {
> >+            *edx = 0;
> >+        }
> 
> I think TCG is able to eliminate unsupported features too before they
> get in env->features[].  Or are those patches not in yet?  Eduardo?

They are not in yet, but while they are not included this filtering
(based on TCG_*_FEATURES macros) should be done in the !kvm_enabled()
block of x86_cpu_realizefn().

-- 
Eduardo

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

[Marcelo Tosatti]

[-- Attachment #1: 02_qemu-block-invtsc-migration.patch --]
[-- Type: text/plain, Size: 3058 bytes --]

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.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

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;
         env->features[w] =
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);

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

[Marcelo Tosatti]

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.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 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;
> +    }

Oops, broken with multiple CPUs. Please review the rest.

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

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

[Marcelo Tosatti]

On Tue, Apr 22, 2014 at 05:38:07PM -0300, Eduardo Habkost wrote:
> 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.

Should be "do not expose invtsc by default with -cpu host".

Since it blocks migration, i considered it a special flag that should 
be set when user is aware that migration is going to be blocked.

Makes sense?

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

[Eduardo Habkost]

On Tue, Apr 22, 2014 at 06:27:59PM -0300, Marcelo Tosatti wrote:
> On Tue, Apr 22, 2014 at 05:38:07PM -0300, Eduardo Habkost wrote:
> > 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.
> 
> Should be "do not expose invtsc by default with -cpu host".
> 
> Since it blocks migration, i considered it a special flag that should 
> be set when user is aware that migration is going to be blocked.
> 
> Makes sense?

Not for "-cpu host". If somebody needs migration to work, they shouldn't
be using "-cpu host" anyway (I don't know if you have seen the other
comments in my message?).

-- 
Eduardo

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

[Paolo Bonzini]

Il 22/04/2014 21:14, Eduardo Habkost ha scritto:
> Not for "-cpu host". If somebody needs migration to work, they shouldn't
> be using "-cpu host" anyway (I don't know if you have seen the other
> comments in my message?).

I'm not entirely sure.  If you have hosts with exactly identical 
chipsets, "-cpu host" migration will in all likelihood work.  Marcelo's 
approach is safer.

Paolo

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

[Eduardo Habkost]

On Thu, Apr 24, 2014 at 04:42:33PM -0400, Paolo Bonzini wrote:
> Il 22/04/2014 21:14, Eduardo Habkost ha scritto:
> >Not for "-cpu host". If somebody needs migration to work, they shouldn't
> >be using "-cpu host" anyway (I don't know if you have seen the other
> >comments in my message?).
> 
> I'm not entirely sure.  If you have hosts with exactly identical
> chipsets, "-cpu host" migration will in all likelihood work.
> Marcelo's approach is safer.

If that didn't break other use cases, I would agree.

But "-cpu host" today covers two use cases: 1) enabling everything that
can be enabled, even if it breaks migration; 2) enabling all stuff that
can be safely enabled without breaking migration.

Now we can't do both at the same time[1].

(1) is important for management software;
(2) works only if you are lucky.

Why would it make sense to break (1) to try make (2) work?

[1] I would even argue that we never did both at the same time."-cpu
host" depends on host hardware capabilities, host kernel capabilities,
and host QEMU version (we never took care of keeping guest ABI with
"-cpu host"). If migration did work, it was never supposed to.

-- 
Eduardo

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

[Paolo Bonzini]

Il 24/04/2014 22:57, Eduardo Habkost ha scritto:
> On Thu, Apr 24, 2014 at 04:42:33PM -0400, Paolo Bonzini wrote:
>> Il 22/04/2014 21:14, Eduardo Habkost ha scritto:
>>> Not for "-cpu host". If somebody needs migration to work, they shouldn't
>>> be using "-cpu host" anyway (I don't know if you have seen the other
>>> comments in my message?).
>>
>> I'm not entirely sure.  If you have hosts with exactly identical
>> chipsets, "-cpu host" migration will in all likelihood work.
>> Marcelo's approach is safer.
>
> If that didn't break other use cases, I would agree.
>
> But "-cpu host" today covers two use cases: 1) enabling everything that
> can be enabled, even if it breaks migration; 2) enabling all stuff that
> can be safely enabled without breaking migration.

What does it enable *now* that breaks migration?

> Now we can't do both at the same time[1].
>
> (1) is important for management software;
> (2) works only if you are lucky.

Or if you plan ahead.  With additional logic even invariant TSC in 
principle can be made to work across migration if the host clocks are 
synchronized well enough (PTP accuracy is in the 100-1000 TSC ticks range).

> Why would it make sense to break (1) to try make (2) work?
>
> [1] I would even argue that we never did both at the same time."-cpu
> host" depends on host hardware capabilities, host kernel capabilities,
> and host QEMU version (we never took care of keeping guest ABI with
> "-cpu host"). If migration did work, it was never supposed to.

I think this is where I disagree.  Migration of the PMU is one thing 
that obviously was done with "-cpu host" in mind.

Paolo

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

[Eduardo Habkost]

On Fri, Apr 25, 2014 at 12:57:48AM +0200, Paolo Bonzini wrote:
> Il 24/04/2014 22:57, Eduardo Habkost ha scritto:
> >On Thu, Apr 24, 2014 at 04:42:33PM -0400, Paolo Bonzini wrote:
> >>Il 22/04/2014 21:14, Eduardo Habkost ha scritto:
> >>>Not for "-cpu host". If somebody needs migration to work, they shouldn't
> >>>be using "-cpu host" anyway (I don't know if you have seen the other
> >>>comments in my message?).
> >>
> >>I'm not entirely sure.  If you have hosts with exactly identical
> >>chipsets, "-cpu host" migration will in all likelihood work.
> >>Marcelo's approach is safer.
> >
> >If that didn't break other use cases, I would agree.
> >
> >But "-cpu host" today covers two use cases: 1) enabling everything that
> >can be enabled, even if it breaks migration; 2) enabling all stuff that
> >can be safely enabled without breaking migration.
> 
> What does it enable *now* that breaks migration?

Every single feature it enables can break it. It breaks if you upgrade
to a QEMU version with new feature words. It breaks if you upgrade to a
kernel which supports new features.

A feature that doesn't let you upgrade the kernel isn't a feature I
expect users to be relying upon. libvirt even blocks migration if "-cpu
host" is in use.

> 
> >Now we can't do both at the same time[1].
> >
> >(1) is important for management software;
> >(2) works only if you are lucky.
> 
> Or if you plan ahead.  With additional logic even invariant TSC in
> principle can be made to work across migration if the host clocks are
> synchronized well enough (PTP accuracy is in the 100-1000 TSC ticks
> range).

Yes, it is possible in the future. But we never planned for it, so "-cpu
host" never supported migration.

> 
> >Why would it make sense to break (1) to try make (2) work?
> >
> >[1] I would even argue that we never did both at the same time."-cpu
> >host" depends on host hardware capabilities, host kernel capabilities,
> >and host QEMU version (we never took care of keeping guest ABI with
> >"-cpu host"). If migration did work, it was never supposed to.
> 
> I think this is where I disagree.  Migration of the PMU is one thing
> that obviously was done with "-cpu host" in mind.

We may try to make a reliable implementation of use case (2) some day,
yes. But the choice I see right now is between trying not break a
feature that was never declared to exist, or breaking an existing
interface that is required to solve existing bugs between libvirt and
QEMU.

-- 
Eduardo

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

[Paolo Bonzini]

Il 25/04/2014 01:18, Eduardo Habkost ha scritto:
> On Fri, Apr 25, 2014 at 12:57:48AM +0200, Paolo Bonzini wrote:
>> Il 24/04/2014 22:57, Eduardo Habkost ha scritto:
>>> If that didn't break other use cases, I would agree.
>>>
>>> But "-cpu host" today covers two use cases: 1) enabling everything that
>>> can be enabled, even if it breaks migration; 2) enabling all stuff that
>>> can be safely enabled without breaking migration.
>>
>> What does it enable *now* that breaks migration?
>
> Every single feature it enables can break it. It breaks if you upgrade
> to a QEMU version with new feature words. It breaks if you upgrade to a
> kernel which supports new features.

Yes, but it doesn't break it if you have the same processor, QEMU and 
kernel versions; this is not the case for invtsc (at least without 
improvements that aren't there yet).

> A feature that doesn't let you upgrade the kernel isn't a feature I
> expect users to be relying upon.

It lets you upgrade the kernel as long as you do it for all machines. 
Whether this is acceptable depends on what you're using virt for.

It can be fine for "cattle" VMs that you can throw away at any time. 
Though people who buy into the cattle vs. pet distinction will tell you 
that you don't migrate cattle and this would make this discussion moot.

It can be fine for personal VMs (e.g. Windows VMs) that people use for 
development and will likely throw away at the end of each working day. 
Though for these VMs you also won't bother with migration, doing 
maintenance at night is easier (and if you need "-cpu host" you're 
probably doing other things such as GPU assignment that prevents migration).

So I couldn't indeed think of uses of "-cpu host" together with 
migration.  But if you're sure there is none, block it in QEMU.  There's 
no reason why this should be specific to libvirt.

>> Or if you plan ahead.  With additional logic even invariant TSC in
>> principle can be made to work across migration if the host clocks are
>> synchronized well enough (PTP accuracy is in the 100-1000 TSC ticks
>> range).
>
> Yes, it is possible in the future. But we never planned for it, so "-cpu
> host" never supported migration.

If it wasn't blocked, it was supported.  You can change this, but this 
implies documenting it in release notes, and perhaps warning for a 
couple of releases to give non-libvirt users a chance to tell us what 
they're doing.

> We may try to make a reliable implementation of use case (2) some day,
> yes. But the choice I see right now is between trying not break a
> feature that was never declared to exist, or breaking an existing
> interface that is required to solve existing bugs between libvirt and
> QEMU.

I'm not sure I follow, what existing interface would be broken and how?

Paolo

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

[Eduardo Habkost]

On Fri, Apr 25, 2014 at 11:08:00PM +0200, Paolo Bonzini wrote:
> Il 25/04/2014 01:18, Eduardo Habkost ha scritto:
> >On Fri, Apr 25, 2014 at 12:57:48AM +0200, Paolo Bonzini wrote:
> >>Il 24/04/2014 22:57, Eduardo Habkost ha scritto:
> >>>If that didn't break other use cases, I would agree.
> >>>
> >>>But "-cpu host" today covers two use cases: 1) enabling everything that
> >>>can be enabled, even if it breaks migration; 2) enabling all stuff that
> >>>can be safely enabled without breaking migration.
> >>
> >>What does it enable *now* that breaks migration?
> >
> >Every single feature it enables can break it. It breaks if you upgrade
> >to a QEMU version with new feature words. It breaks if you upgrade to a
> >kernel which supports new features.
> 
> Yes, but it doesn't break it if you have the same processor, QEMU and
> kernel versions; this is not the case for invtsc (at least without
> improvements that aren't there yet).
> 
> >A feature that doesn't let you upgrade the kernel isn't a feature I
> >expect users to be relying upon.
> 
> It lets you upgrade the kernel as long as you do it for all machines.
> Whether this is acceptable depends on what you're using virt for.
> 
> It can be fine for "cattle" VMs that you can throw away at any time.
> Though people who buy into the cattle vs. pet distinction will tell
> you that you don't migrate cattle and this would make this discussion
> moot.
> 
> It can be fine for personal VMs (e.g. Windows VMs) that people use
> for development and will likely throw away at the end of each working
> day. Though for these VMs you also won't bother with migration, doing
> maintenance at night is easier (and if you need "-cpu host" you're
> probably doing other things such as GPU assignment that prevents
> migration).
> 
> So I couldn't indeed think of uses of "-cpu host" together with
> migration.  But if you're sure there is none, block it in QEMU.
> There's no reason why this should be specific to libvirt.

It's not that there are no useful use cases, it is that we simply can't
guarantee it will work because (by design) "-cpu host" enables features
QEMU doesn't know about (so it doesn't know if migration is safe or
not). And that's the main use case for "-cpu host".


> 
> >>Or if you plan ahead.  With additional logic even invariant TSC in
> >>principle can be made to work across migration if the host clocks are
> >>synchronized well enough (PTP accuracy is in the 100-1000 TSC ticks
> >>range).
> >
> >Yes, it is possible in the future. But we never planned for it, so "-cpu
> >host" never supported migration.
> 
> If it wasn't blocked, it was supported.  You can change this, but
> this implies documenting it in release notes, and perhaps warning for
> a couple of releases to give non-libvirt users a chance to tell us
> what they're doing.
> 
> >We may try to make a reliable implementation of use case (2) some day,
> >yes. But the choice I see right now is between trying not break a
> >feature that was never declared to exist, or breaking an existing
> >interface that is required to solve existing bugs between libvirt and
> >QEMU.
> 
> I'm not sure I follow, what existing interface would be broken and how?

Management can use "-cpu host" and the "feature-words" QOM property to
check and report which features are really supported by the host, before
trying to use them. That means management will think "invtsc" is
completely unavailable if it is not present on "-cpu host" by default.

But there's also another point: management may _want_ invtsc to be
unset, because it may expect a feature to be available only if it can be
safely migrated.

So, what about doing the following on QEMU 2.1:

 * "-cpu host,migratable=yes":
   * No invtsc
   * Migration not blocked
   * No feature flag unknown to QEMU will be enabled
 * "-cpu host,migratable=no":
   * invtsc enabled
   * Unknown-to-QEMU features enabled
   * Migration blocked
 * "-cpu host":
   * Same behavior as "-cpu host,migratable=yes"
 * Release notes indicating that "-cpu host,migratable=no" is
   required if the user doesn't care about migration and wants new
   (unknown to QEMU) features to be available

I was going to propose making "migratable=no" the default after a few
releases, as I expect the majority of existing users of "-cpu host" to
not care about migration. But I am not sure, because there's less harm
in _not_ getting new bleeding edge features enabled, and those users
(and management software) can start using "migratable=no" if they really
want the new unmigratable features.

-- 
Eduardo

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

[Paolo Bonzini]

Il 28/04/2014 17:46, Eduardo Habkost ha scritto:
>> So I couldn't indeed think of uses of "-cpu host" together with
>> migration.  But if you're sure there is none, block it in QEMU.
>> There's no reason why this should be specific to libvirt.
>
> It's not that there are no useful use cases, it is that we simply can't
> guarantee it will work because (by design) "-cpu host" enables features
> QEMU doesn't know about (so it doesn't know if migration is safe or
> not). And that's the main use case for "-cpu host".

True, but in practice QEMU and KVM support is added in parallel, and we 
already have full support for Broadwell (IIRC) and are starting to add 
Skylake features.

> So, what about doing the following on QEMU 2.1:
>
>  * "-cpu host,migratable=yes":
>    * No invtsc
>    * Migration not blocked
>    * No feature flag unknown to QEMU will be enabled
>  * "-cpu host,migratable=no":
>    * invtsc enabled
>    * Unknown-to-QEMU features enabled
>    * Migration blocked
>  * "-cpu host":
>    * Same behavior as "-cpu host,migratable=yes"
>  * Release notes indicating that "-cpu host,migratable=no" is
>    required if the user doesn't care about migration and wants new
>    (unknown to QEMU) features to be available
>
> I was going to propose making "migratable=no" the default after a few
> releases, as I expect the majority of existing users of "-cpu host" to
> not care about migration. But I am not sure, because there's less harm
> in _not_ getting new bleeding edge features enabled, and those users
> (and management software) can start using "migratable=no" if they really
> want the new unmigratable features.

Makes sense.  Basically "-cpu host,migratable=yes" is close to libvirt's 
host-model and Alex Graf's proposed "-cpu best".  Should we call it 
"-cpu best" and drop migratability of "-cpu host"?

Paolo

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

[Eduardo Habkost]

On Mon, Apr 28, 2014 at 09:06:42PM +0200, Paolo Bonzini wrote:
> Il 28/04/2014 17:46, Eduardo Habkost ha scritto:
> >>So I couldn't indeed think of uses of "-cpu host" together with
> >>migration.  But if you're sure there is none, block it in QEMU.
> >>There's no reason why this should be specific to libvirt.
> >
> >It's not that there are no useful use cases, it is that we simply can't
> >guarantee it will work because (by design) "-cpu host" enables features
> >QEMU doesn't know about (so it doesn't know if migration is safe or
> >not). And that's the main use case for "-cpu host".
> 
> True, but in practice QEMU and KVM support is added in parallel, and
> we already have full support for Broadwell (IIRC) and are starting to
> add Skylake features.
> 
> >So, what about doing the following on QEMU 2.1:
> >
> > * "-cpu host,migratable=yes":
> >   * No invtsc
> >   * Migration not blocked
> >   * No feature flag unknown to QEMU will be enabled
> > * "-cpu host,migratable=no":
> >   * invtsc enabled
> >   * Unknown-to-QEMU features enabled
> >   * Migration blocked
> > * "-cpu host":
> >   * Same behavior as "-cpu host,migratable=yes"
> > * Release notes indicating that "-cpu host,migratable=no" is
> >   required if the user doesn't care about migration and wants new
> >   (unknown to QEMU) features to be available
> >
> >I was going to propose making "migratable=no" the default after a few
> >releases, as I expect the majority of existing users of "-cpu host" to
> >not care about migration. But I am not sure, because there's less harm
> >in _not_ getting new bleeding edge features enabled, and those users
> >(and management software) can start using "migratable=no" if they really
> >want the new unmigratable features.
> 
> Makes sense.  Basically "-cpu host,migratable=yes" is close to
> libvirt's host-model and Alex Graf's proposed "-cpu best".  Should we
> call it "-cpu best" and drop migratability of "-cpu host"?

"-cpu best" is different from the modes above. It means "use the best
existing CPU model (from the pre-defined table) that can run on this
host".

And it would have the same ambiguities we found on "-cpu host": if a CPU
model in the table have invtsc enabled, should it be considered a
candidate for "-cpu best", or not?

-- 
Eduardo

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

[Paolo Bonzini]

Il 28/04/2014 21:23, Eduardo Habkost ha scritto:
> > Makes sense.  Basically "-cpu host,migratable=yes" is close to
> > libvirt's host-model and Alex Graf's proposed "-cpu best".  Should we
> > call it "-cpu best" and drop migratability of "-cpu host"?
>
> "-cpu best" is different from the modes above. It means "use the best
> existing CPU model (from the pre-defined table) that can run on this
> host".

Yes, it's not exactly the same.  In practice the behavior should be close.

> And it would have the same ambiguities we found on "-cpu host": if a CPU
> model in the table have invtsc enabled, should it be considered a
> candidate for "-cpu best", or not?

No CPU model in the table should have invtsc enabled. :)

Paolo

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

[Eduardo Habkost]

On Tue, Apr 29, 2014 at 08:22:36AM +0200, Paolo Bonzini wrote:
> Il 28/04/2014 21:23, Eduardo Habkost ha scritto:
> >> Makes sense.  Basically "-cpu host,migratable=yes" is close to
> >> libvirt's host-model and Alex Graf's proposed "-cpu best".  Should we
> >> call it "-cpu best" and drop migratability of "-cpu host"?
> >
> >"-cpu best" is different from the modes above. It means "use the best
> >existing CPU model (from the pre-defined table) that can run on this
> >host".
> 
> Yes, it's not exactly the same.  In practice the behavior should be close.
> 
> >And it would have the same ambiguities we found on "-cpu host": if a CPU
> >model in the table have invtsc enabled, should it be considered a
> >candidate for "-cpu best", or not?
> 
> No CPU model in the table should have invtsc enabled. :)

I agree with you, but I wouldn't be so sure this will never happen. We
have had conflicts in the past between having CPU models that are useful
defaults for KVM vs CPU models that match real hardware more closely.

This is already the case with the "monitor" flag (that is not supported
by KVM but is supported by TCG, and is present on many CPU models).

(But this can be solved by adding a kvm_default_disable_features array,
like the kvm_default_features array but for features that should be
_disabled_ by default on KVM. I'm adding that to my TODO-list.)

-- 
Eduardo

[patch 0/2] expose invariant tsc flag for kvm guests (v2)

[Marcelo Tosatti]

v2:
- filter for TCG at cpu realizefn.
- allow invariant tsc by default with "-cpu host".
- fix initialization with multiple vcpus.

[patch 1/2] target-i386: support "invariant tsc" flag

[Marcelo Tosatti]

[-- Attachment #1: 01_qemu-cpuid-invariant.patch --]
[-- Type: TEXT/PLAIN, Size: 4814 bytes --]

Expose "Invariant TSC" flag, if KVM is enabled. From Intel documentation:

17.13.1 Invariant TSC The time stamp counter in newer processors may
support an enhancement, referred to as invariant TSC. Processor’s
support for invariant TSC is indicated by CPUID.80000007H:EDX[8].
The invariant TSC will run at a constant rate in all ACPI P-, C-.
and T-states. This is the architectural behavior moving forward. On
processors with invariant TSC support, the OS may use the TSC for wall
clock timer services (instead of ACPI or HPET timers). TSC reads are
much more efficient and do not incur the overhead associated with a ring
transition or access to a platform resource.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-invariant-tsc/target-i386/cpu.c
===================================================================
--- qemu-invariant-tsc.orig/target-i386/cpu.c
+++ qemu-invariant-tsc/target-i386/cpu.c
@@ -262,6 +262,17 @@ static const char *cpuid_7_0_ebx_feature
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+static const char *cpuid_apm_edx_feature_name[] = {
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    "invtsc", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 typedef struct FeatureWordInfo {
     const char **feat_names;
     uint32_t cpuid_eax;   /* Input EAX for CPUID */
@@ -305,6 +316,11 @@ static FeatureWordInfo feature_word_info
         .cpuid_needs_ecx = true, .cpuid_ecx = 0,
         .cpuid_reg = R_EBX,
     },
+    [FEAT_8000_0007_EDX] = {
+        .feat_names = cpuid_apm_edx_feature_name,
+        .cpuid_eax = 0x80000007,
+        .cpuid_reg = R_EDX,
+    },
 };
 
 typedef struct X86RegisterInfo32 {
@@ -580,6 +596,7 @@ struct X86CPUDefinition {
           CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
           CPUID_7_0_EBX_ERMS, CPUID_7_0_EBX_INVPCID, CPUID_7_0_EBX_RTM,
           CPUID_7_0_EBX_RDSEED */
+#define TCG_APM_FEATURES 0
 
 static X86CPUDefinition builtin_x86_defs[] = {
     {
@@ -1740,6 +1757,7 @@ static void x86_cpu_parse_featurestr(CPU
     env->features[FEAT_1_ECX] |= plus_features[FEAT_1_ECX];
     env->features[FEAT_8000_0001_EDX] |= plus_features[FEAT_8000_0001_EDX];
     env->features[FEAT_8000_0001_ECX] |= plus_features[FEAT_8000_0001_ECX];
+    env->features[FEAT_8000_0007_EDX] |= plus_features[FEAT_8000_0007_EDX];
     env->features[FEAT_C000_0001_EDX] |= plus_features[FEAT_C000_0001_EDX];
     env->features[FEAT_KVM] |= plus_features[FEAT_KVM];
     env->features[FEAT_SVM] |= plus_features[FEAT_SVM];
@@ -1748,6 +1766,7 @@ static void x86_cpu_parse_featurestr(CPU
     env->features[FEAT_1_ECX] &= ~minus_features[FEAT_1_ECX];
     env->features[FEAT_8000_0001_EDX] &= ~minus_features[FEAT_8000_0001_EDX];
     env->features[FEAT_8000_0001_ECX] &= ~minus_features[FEAT_8000_0001_ECX];
+    env->features[FEAT_8000_0007_EDX] &= ~minus_features[FEAT_8000_0007_EDX];
     env->features[FEAT_C000_0001_EDX] &= ~minus_features[FEAT_C000_0001_EDX];
     env->features[FEAT_KVM] &= ~minus_features[FEAT_KVM];
     env->features[FEAT_SVM] &= ~minus_features[FEAT_SVM];
@@ -2333,6 +2352,12 @@ void cpu_x86_cpuid(CPUX86State *env, uin
                (AMD_ENC_ASSOC(L3_ASSOCIATIVITY) << 12) | \
                (L3_LINES_PER_TAG << 8) | (L3_LINE_SIZE);
         break;
+    case 0x80000007:
+        *eax = 0;
+        *ebx = 0;
+        *ecx = 0;
+        *edx = env->features[FEAT_8000_0007_EDX];
+        break;
     case 0x80000008:
         /* virtual & phys address size in low 2 bytes. */
 /* XXX: This value must match the one used in the MMU code. */
@@ -2598,6 +2623,7 @@ static void x86_cpu_realizefn(DeviceStat
             );
         env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
         env->features[FEAT_SVM] &= TCG_SVM_FEATURES;
+        env->features[FEAT_8000_0007_EDX] &= TCG_APM_FEATURES;
     } else {
         KVMState *s = kvm_state;
         if ((cpu->check_cpuid || cpu->enforce_cpuid)
Index: qemu-invariant-tsc/target-i386/cpu.h
===================================================================
--- qemu-invariant-tsc.orig/target-i386/cpu.h
+++ qemu-invariant-tsc/target-i386/cpu.h
@@ -398,6 +398,7 @@ typedef enum FeatureWord {
     FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
     FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
     FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
+    FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
     FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
     FEAT_KVM,           /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
     FEAT_SVM,           /* CPUID[8000_000A].EDX */

Re: [patch 1/2] target-i386: support "invariant tsc" flag

[Eduardo Habkost]

On Wed, Apr 23, 2014 at 03:20:03PM -0300, Marcelo Tosatti wrote:
> Expose "Invariant TSC" flag, if KVM is enabled. From Intel documentation:
> 
> 17.13.1 Invariant TSC The time stamp counter in newer processors may
> support an enhancement, referred to as invariant TSC. Processor’s
> support for invariant TSC is indicated by CPUID.80000007H:EDX[8].
> The invariant TSC will run at a constant rate in all ACPI P-, C-.
> and T-states. This is the architectural behavior moving forward. On
> processors with invariant TSC support, the OS may use the TSC for wall
> clock timer services (instead of ACPI or HPET timers). TSC reads are
> much more efficient and do not incur the overhead associated with a ring
> transition or access to a platform resource.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I plan to rebase this on top of my feature-word-filtering refactor
series ("Support check/enforce flags in TCG mode, too"), and submit it
as part of that series.

-- 
Eduardo

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

[Marcelo Tosatti]

[-- Attachment #1: 02_qemu-block-invtsc-migration.patch --]
[-- Type: text/plain, Size: 2469 bytes --]

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.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

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,15 @@ 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) && invtsc_mig_blocker == NULL) {
+        /* for migration */
+        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,

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

[Eduardo Habkost]

On Wed, Apr 23, 2014 at 03:20:04PM -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.

You probably want to remove the above line?

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 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,15 @@ 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) && invtsc_mig_blocker == NULL) {
> +        /* for migration */
> +        error_set(&invtsc_mig_blocker, QERR_MIGRATION_NOT_SUPPORTED, "cpu");

What about using QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "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,
> 
> 

-- 
Eduardo

target-i386: block migration and savevm if invariant tsc is exposed (v3)

[Marcelo Tosatti]

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.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

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) && invtsc_mig_blocker == NULL) {
+        /* for migration */
+        error_set(&invtsc_mig_blocker,
+                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "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,

Re: target-i386: block migration and savevm if invariant tsc is exposed (v3)

[Eduardo Habkost]

On Wed, Apr 23, 2014 at 06:04:45PM -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.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
[...]
> @@ -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) && invtsc_mig_blocker == NULL) {
> +        /* for migration */
> +        error_set(&invtsc_mig_blocker,
> +                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "cpu");
> +        migrate_add_blocker(invtsc_mig_blocker);
> +        /* for savevm */
> +        vmstate_x86_cpu.unmigratable = 1;

Did you ensure this will always happen before vmstate_register() is
called for vmstate_x86_cpu? I believe kvm_arch_init_vcpu() is called a
long long time after device_set_realized() (which is where
vmstate_register() is called for DeviceState objects).

-- 
Eduardo

Re: target-i386: block migration and savevm if invariant tsc is exposed (v3)

[Marcelo Tosatti]

On Thu, Apr 24, 2014 at 04:21:59PM -0300, Eduardo Habkost wrote:
> On Wed, Apr 23, 2014 at 06:04:45PM -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.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> [...]
> > @@ -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) && invtsc_mig_blocker == NULL) {
> > +        /* for migration */
> > +        error_set(&invtsc_mig_blocker,
> > +                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "cpu");
> > +        migrate_add_blocker(invtsc_mig_blocker);
> > +        /* for savevm */
> > +        vmstate_x86_cpu.unmigratable = 1;
> 
> Did you ensure this will always happen before vmstate_register() is
> called for vmstate_x86_cpu? I believe kvm_arch_init_vcpu() is called a
> long long time after device_set_realized() (which is where
> vmstate_register() is called for DeviceState objects).

x86_cpu_realizefn -> qemu_init_vcpu -> qemu_kvm_start_vcpu ->
qemu_kvm_cpu_thread_fn -> kvm_init_vcpu -> kvm_arch_init_vcpu


@@ -2573,6 +2598,8 @@ static void x86_cpu_realizefn(DeviceState *dev,
Error **errp)
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;

+    printf("%s: dev->realized=%d\n", __func__, dev->realized);
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }



QEMU 1.7.93 monitor - type 'help' for more information
(qemu) x86_cpu_realizefn: dev->realized=0
x86_cpu_realizefn: dev->realized=0
audio: Could not init `oss' audio driver

Re: target-i386: block migration and savevm if invariant tsc is exposed (v3)

[Eduardo Habkost]

On Thu, Apr 24, 2014 at 06:32:42PM -0300, Marcelo Tosatti wrote:
> On Thu, Apr 24, 2014 at 04:21:59PM -0300, Eduardo Habkost wrote:
> > On Wed, Apr 23, 2014 at 06:04:45PM -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.
> > > 
> > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > [...]
> > > @@ -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) && invtsc_mig_blocker == NULL) {
> > > +        /* for migration */
> > > +        error_set(&invtsc_mig_blocker,
> > > +                  QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "invtsc", "cpu");
> > > +        migrate_add_blocker(invtsc_mig_blocker);
> > > +        /* for savevm */
> > > +        vmstate_x86_cpu.unmigratable = 1;
> > 
> > Did you ensure this will always happen before vmstate_register() is
> > called for vmstate_x86_cpu? I believe kvm_arch_init_vcpu() is called a
> > long long time after device_set_realized() (which is where
> > vmstate_register() is called for DeviceState objects).
> 

> x86_cpu_realizefn

x86_cpu_realizefn is called by device_set_realized (via
DeviceClass.realize), but vmstate_register*() is called _after_
DeviceClass.realize.

Continuing:

> -> qemu_init_vcpu -> qemu_kvm_start_vcpu ->
> qemu_kvm_cpu_thread_fn

  ^^^^ this is run in a new thread

> -> kvm_init_vcpu -> kvm_arch_init_vcpu

     ^^^^ this is called after acquiring qemu_global_mutex.

Then it gets tricky: qemu_kvm_start_vcpu() returns only after
cpu->created is set, which is done after kvm_arch_init_vcpu() is called.

BUT:

I was mistaken. This is not where vmstate_x86_cpu is registered. It is at
cpu_exec_init(). And cpu_exec_init() is called much earlier, at
x86_cpu_initfn(). See the printf() results below.

Have you tested if your patch actually blocks savevm?


> 
> 
> @@ -2573,6 +2598,8 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
>      CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
> 
> +    printf("%s: dev->realized=%d\n", __func__, dev->realized);
> +
>      if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
>          env->cpuid_level = 7;
>      }
> 
> 
> 
> QEMU 1.7.93 monitor - type 'help' for more information
> (qemu) x86_cpu_realizefn: dev->realized=0
> x86_cpu_realizefn: dev->realized=0
> audio: Could not init `oss' audio driver

    diff --git a/exec.c b/exec.c
    index 91513c6..1fd3e12 100644
    --- a/exec.c
    +++ b/exec.c
    @@ -505,6 +505,7 @@ void cpu_exec_init(CPUArchState *env)
         assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
     #endif
         if (cc->vmsd != NULL) {
    +        printf("registering CPUClass.vmsd\n");
             vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
         }
     }
    diff --git a/target-i386/kvm.c b/target-i386/kvm.c
    index 73643d7..42c874c 100644
    --- a/target-i386/kvm.c
    +++ b/target-i386/kvm.c
    @@ -468,6 +468,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         int kvm_base = KVM_CPUID_SIGNATURE;
         int r;
     
    +    printf("%s: realized=%d\n", __func__, DEVICE(cpu)->realized);
         memset(&cpuid_data, 0, sizeof(cpuid_data));
     
         cpuid_i = 0;


QEMU 2.0.50 monitor - type 'help' for more information
(qemu) registering CPUClass.vmsd
kvm_arch_init_vcpu: realized=0




-- 
Eduardo

[PATCH] savevm: check vmsd for migratability status

[Marcelo Tosatti]

Check vmsd for unmigratable field, allowing migratibility status
to be modified after vmstate_register.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/savevm.c b/savevm.c
index 22123be..61a25c0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -452,7 +452,7 @@ bool qemu_savevm_state_blocked(Error **errp)
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
-        if (se->no_migrate) {
+        if (se->no_migrate || (se->vmsd && se->vmsd->unmigratable)) {
             error_set(errp, QERR_MIGRATION_NOT_SUPPORTED, se->idstr);
             return true;
         }

Re: [PATCH] savevm: check vmsd for migratability status

[Eduardo Habkost]

On Fri, Apr 25, 2014 at 07:47:09PM -0300, Marcelo Tosatti wrote:
> 
> Check vmsd for unmigratable field, allowing migratibility status
> to be modified after vmstate_register.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/savevm.c b/savevm.c
> index 22123be..61a25c0 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -452,7 +452,7 @@ bool qemu_savevm_state_blocked(Error **errp)
>      SaveStateEntry *se;
>  
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> -        if (se->no_migrate) {
> +        if (se->no_migrate || (se->vmsd && se->vmsd->unmigratable)) {

The only place where se->no_migrate is set to non-zero is when a vmsd is
provided. What about just removing the field and using
(se->vmsd && se->vmsd->unmigratable) only?

-- 
Eduardo

[PATCH] savevm: check vmsd for migratability status (v2)

[Marcelo Tosatti]

Check vmsd for unmigratable field, allowing migratibility status
to be modified after vmstate_register.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/savevm.c b/savevm.c
index da8aa24..c578e42 100644
--- a/savevm.c
+++ b/savevm.c
@@ -232,7 +232,6 @@ typedef struct SaveStateEntry {
     const VMStateDescription *vmsd;
     void *opaque;
     CompatEntry *compat;
-    int no_migrate;
     int is_ram;
 } SaveStateEntry;
 
@@ -292,7 +291,6 @@ int register_savevm_live(DeviceState *dev,
     se->ops = ops;
     se->opaque = opaque;
     se->vmsd = NULL;
-    se->no_migrate = 0;
     /* if this is a live_savem then set is_ram */
     if (ops->save_live_setup != NULL) {
         se->is_ram = 1;
@@ -383,7 +381,6 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
     se->opaque = opaque;
     se->vmsd = vmsd;
     se->alias_id = alias_id;
-    se->no_migrate = vmsd->unmigratable;
 
     if (dev) {
         char *id = qdev_get_dev_path(dev);
@@ -452,7 +449,7 @@ bool qemu_savevm_state_blocked(Error **errp)
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
-        if (se->no_migrate) {
+        if (se->vmsd && se->vmsd->unmigratable) {
             error_setg(errp, "State blocked by non-migratable device '%s'",
                        se->idstr);
             return true;

Re: [Qemu-devel] [PATCH] savevm: check vmsd for migratability status (v2)

[Eduardo Habkost]

(CCing Juan)

On Tue, Apr 29, 2014 at 09:39:03PM -0300, Marcelo Tosatti wrote:
> Check vmsd for unmigratable field, allowing migratibility status
> to be modified after vmstate_register.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> 
> diff --git a/savevm.c b/savevm.c
> index da8aa24..c578e42 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -232,7 +232,6 @@ typedef struct SaveStateEntry {
>      const VMStateDescription *vmsd;
>      void *opaque;
>      CompatEntry *compat;
> -    int no_migrate;
>      int is_ram;
>  } SaveStateEntry;
>  
> @@ -292,7 +291,6 @@ int register_savevm_live(DeviceState *dev,
>      se->ops = ops;
>      se->opaque = opaque;
>      se->vmsd = NULL;
> -    se->no_migrate = 0;
>      /* if this is a live_savem then set is_ram */
>      if (ops->save_live_setup != NULL) {
>          se->is_ram = 1;
> @@ -383,7 +381,6 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>      se->opaque = opaque;
>      se->vmsd = vmsd;
>      se->alias_id = alias_id;
> -    se->no_migrate = vmsd->unmigratable;
>  
>      if (dev) {
>          char *id = qdev_get_dev_path(dev);
> @@ -452,7 +449,7 @@ bool qemu_savevm_state_blocked(Error **errp)
>      SaveStateEntry *se;
>  
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> -        if (se->no_migrate) {
> +        if (se->vmsd && se->vmsd->unmigratable) {
>              error_setg(errp, "State blocked by non-migratable device '%s'",
>                         se->idstr);
>              return true;
> 

-- 
Eduardo

Re: target-i386: block migration and savevm if invariant tsc is exposed (v3)

[Marcelo Tosatti]

On Fri, Apr 25, 2014 at 05:38:08PM -0300, Eduardo Habkost wrote:
> Have you tested if your patch actually blocks savevm?

Yes, with -smp 2.

Can you please include the savevm.c patch in your queue?
TIA.