Re: [Qemu-devel] [PATCH v4 17/18] target-i386: block migration and savevm if invariant tsc is exposed

[Eduardo Habkost]

On Fri, May 16, 2014 at 11:05:28AM +0200, Andreas Färber wrote:
> Am 15.05.2014 22:22, schrieb Andreas Färber:
> > Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> >> From: Marcelo Tosatti <address@hidden>
> >>
> >> 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.
> >>
> >> Cc: Juan Quintela <address@hidden>
> >> Signed-off-by: Marcelo Tosatti <address@hidden>
> >> Reviewed-by: Eduardo Habkost <address@hidden>
> >> Signed-off-by: Eduardo Habkost <address@hidden>
> >> ---
> >>  target-i386/cpu-qom.h |  2 +-
> >>  target-i386/kvm.c     | 13 +++++++++++++
> >>  target-i386/machine.c |  2 +-
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >> index 016f90d..473d803 100644
> >> --- a/target-i386/cpu-qom.h
> >> +++ b/target-i386/cpu-qom.h
> >> @@ -121,7 +121,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> >>  #define ENV_OFFSET offsetof(X86CPU, env)
> >>  
> >>  #ifndef CONFIG_USER_ONLY
> >> -extern const struct VMStateDescription vmstate_x86_cpu;
> >> +extern struct VMStateDescription vmstate_x86_cpu;
> >>  #endif
> >>  
> >>  /**
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 4389959..99cc7e3 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/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;
> > 
> > This should be static, even if no zero-initialization is needed below.
> > 
> >> +
> >>  #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");
> > 
> > This does not compile for me. error_setg()? With what text?
> 
> http://git.qemu-project.org/?p=qemu.git;a=blobdiff;f=include/qapi/qmp/qerror.h;h=01d1d0661c607ace1c5d3831e5c79eeab851f6b7;hp=a72bbc98503fe261bb8d2c407220252b1e6a85a4;hb=f231b88db14f13ee9a41599896f57f3594c1ca8b;hpb=d73f0beadb57f885e678bffc362864f4401262e0
> 
> -#define QERR_MIGRATION_NOT_SUPPORTED \
> -    ERROR_CLASS_GENERIC_ERROR, "State blocked by non-migratable device
> '%s'"
> 
> Suggesting something nicer than "device 'cpu'":
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 1fe8512..9b09a1a 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -710,7 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      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");
> +        error_setg(&invtsc_mig_blocker,
> +                   "State blocked by non-migratable CPU device");
>          migrate_add_blocker(invtsc_mig_blocker);
>          /* for savevm */
>          vmstate_x86_cpu.unmigratable = 1;

v3 sent by Marcelo had a different error message, but my queue had v2. I
suggest this:

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f9ffa4b..7099a51 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -710,8 +710,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0);
     if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) {
         /* for migration */
-        error_setg(&invtsc_mig_blocker,
-                   "State blocked by non-migratable CPU device");
+        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;

Requiring the user to look at QEMU source code to find out which feature
is blocking migration would just cause confusion.

BTW, applying this patch without applying "Set migratable=yes by
default" will break existing setups where users expect migration to
work. See the discussion at:
http://marc.info/?l=qemu-devel&m=139838802220184&w=2

-- 
Eduardo