Re: [PATCH 4/4] kvm: Allow migration with invtsc

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Jan 04, 2017 at 09:56:56AM -0200, Marcelo Tosatti wrote:
> On Tue, Dec 27, 2016 at 05:21:20PM -0200, Eduardo Habkost wrote:
> > Instead of blocking migration on the source when invtsc is
> > enabled, rely on the migration destination to ensure there's no
> > TSC frequency mismatch.
> > 
> > We can't allow migration unconditionally because we don't know if
> > the destination is a QEMU version that is really going to ensure
> > there's no TSC frequency mismatch. To ensure we are migrating to
> > a destination that won't ignore SET_TSC_KHZ errors, allow invtsc
> > migration only on pc-*-2.9 and newer.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  include/hw/i386/pc.h |  7 ++++++-
> >  target/i386/cpu.h    |  1 +
> >  target/i386/cpu.c    |  1 +
> >  target/i386/kvm.c    | 15 +++++++++------
> >  4 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index ceeacca..4270923 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -375,7 +375,12 @@ int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> >  #define PC_COMPAT_2_8 \
> > -    HW_COMPAT_2_8
> > +    HW_COMPAT_2_8 \
> > +    {\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "invtsc-migration",\
> > +        .value    = "off",\
> > +    },
> >  
> >  #define PC_COMPAT_2_7 \
> >      HW_COMPAT_2_7 \
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index a7f2f60..ec8cdbc 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1208,6 +1208,7 @@ struct X86CPU {
> >      bool expose_kvm;
> >      bool migratable;
> >      bool host_features;
> > +    bool invtsc_migration;
> >      uint32_t apic_id;
> >  
> >      /* if true the CPUID code directly forward host cache leaves to the guest */
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index b0640f1..cc93b81 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -3678,6 +3678,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
> >      DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
> >      DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
> > +    DEFINE_PROP_BOOL("invtsc-migration", X86CPU, invtsc_migration, true),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 6a51399..2c3ee7b 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -962,7 +962,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          has_msr_mcg_ext_ctl = has_msr_feature_control = true;
> >      }
> >  
> > -    if (!env->user_tsc_khz) {
> > +    if (!cpu->invtsc_migration && !env->user_tsc_khz) {
> >          if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
> >              invtsc_mig_blocker == NULL) {
> >              /* for migration */
> > @@ -972,6 +972,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >              migrate_add_blocker(invtsc_mig_blocker);
> >              /* for savevm */
> >              vmstate_x86_cpu.unmigratable = 1;
> > +        }
> >      }
> >  
> >      cpuid_data.cpuid.padding = 0;
> > @@ -2655,12 +2656,14 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >      }
> >  
> >      if (level == KVM_PUT_FULL_STATE) {
> > -        /* We don't check for kvm_arch_set_tsc_khz() errors here,
> > -         * because TSC frequency mismatch shouldn't abort migration,
> > -         * unless the user explicitly asked for a more strict TSC
> > -         * setting (e.g. using an explicit "tsc-freq" option).
> > +        /* Migration TSC frequency mismatch is fatal only if we are
> > +         * actually reporting Invariant TSC to the guest.
> >           */
> > -        kvm_arch_set_tsc_khz(cpu);
> > +        ret = kvm_arch_set_tsc_khz(cpu);
> > +        if ((x86_cpu->env.features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
> > +            ret < 0) {
> > +            return ret;
> > +        }
> >      }
> 
> Will the guest continue in the source in this case?
> 
> I think this is past the point where migration has been declared
> successful. 
> 
> Otherwise looks good.

Good point. I will make additional tests and see if there's some
other place where the kvm_arch_set_tsc_khz() call can be moved
to.

-- 
Eduardo