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

Eduardo Habkost
2017-01-04T13:40:23Z

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