Re: [Qemu-devel] [PATCH v4 2/2] target-i386: add support to migrate vcpu's TSC rate

[Haozhong Zhang]

On 11/16/15 11:43, Eduardo Habkost wrote:
> On Mon, Nov 16, 2015 at 04:04:08PM +0800, Haozhong Zhang wrote:
> > This patch enables migrating vcpu's TSC rate. If KVM on the destination
> > machine supports TSC scaling, guest programs will observe a consistent
> > TSC rate across the migration.
> > 
> > If TSC scaling is not supported on the destination machine, the
> > migration will not be aborted and QEMU on the destination will not set
> > vcpu's TSC rate to the migrated value.
> > 
> > If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> > machine is inconsistent with the migrated TSC rate, the migration will
> > be aborted.
> > 
> > For backwards compatibility, the migration of vcpu's TSC rate is
> > disabled on pc-*-2.4 and older machine types.
> > 
> > Signed-off-by: Haozhong Zhang <address@hidden>
> > ---
> >  hw/i386/pc.c          |  1 +
> >  hw/i386/pc_piix.c     |  1 +
> >  hw/i386/pc_q35.c      |  1 +
> >  include/hw/i386/pc.h  |  1 +
> >  target-i386/cpu.c     |  2 +-
> >  target-i386/cpu.h     |  1 +
> >  target-i386/kvm.c     | 26 ++++++++++++++++++++++++--
> >  target-i386/machine.c | 28 ++++++++++++++++++++++++++++
> >  8 files changed, 58 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 0cb8afd..2f2fc93 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1952,6 +1952,7 @@ static void pc_machine_class_init(ObjectClass *oc, 
> > void *data)
> >      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >  
> >      pcmc->get_hotplug_handler = mc->get_hotplug_handler;
> > +    pcmc->save_tsc_khz = true;
> >      mc->get_hotplug_handler = pc_get_hotpug_handler;
> >      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
> >      mc->default_boot_order = "cad";
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 07d0baa..7c5b0d2 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -489,6 +489,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass 
> > *m)
> >      m->alias = NULL;
> >      m->is_default = 0;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 0fdae09..fd8efe3 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -387,6 +387,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >      m->hw_version = "2.4.0";
> >      m->alias = NULL;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->save_tsc_khz = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 4bbc0ff..fea0f28 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -60,6 +60,7 @@ struct PCMachineClass {
> >  
> >      /*< public >*/
> >      bool broken_reserved_end;
> > +    bool save_tsc_khz;
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >                                             DeviceState *dev);
> >  };
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e5f1c5b..98c6a4c 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1724,7 +1724,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> > Visitor *v, void *opaque,
> >          return;
> >      }
> >  
> > -    cpu->env.tsc_khz = value / 1000;
> > +    cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> >  }
> >  
> >  static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index fc4a605..1ad1da8 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -973,6 +973,7 @@ typedef struct CPUX86State {
> >      uint32_t sipi_vector;
> >      bool tsc_valid;
> >      int64_t tsc_khz;
> > +    int64_t user_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 9084b29..8448248 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2334,8 +2334,30 @@ static void kvm_arch_set_tsc_khz(CPUState *cs)
> >      int r;
> >  
> >      /*
> > -     * If no user-specified TSC frequency is present, we will try to
> > -     * set env->tsc_khz to the value used by KVM.
> > +     * For other cases of env->tsc_khz and env->user_tsc_khz:
> > +     *
> > +     * - We have eliminated all cases that satisfy
> > +     *       env->tsc_khz && env->user_tsc_khz &&
> > +     *       env->tsc_khz != env->user_tsc_khz
> > +     *   in cpu_post_load().
> > +     *
> > +     * - If env->user_tsc_khz is not zero, then it must be equal to
> > +     *   env->tsc_khz (if the latter is not zero) and has been set in
> > +     *   kvm_arch_init_vcpu().
> > +     */
> > +    if (env->tsc_khz && !env->user_tsc_khz) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> > +            kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) : -ENOTSUP;
> 
> Please don't duplicate the code from kvm_arch_init_vcpu(). We can
> handle both cases in kvm_arch_put_registers(KVM_PUT_FULL_STATE),
> can't we?
>

No. If KVM_SET_TSC_KHZ fails in kvm_arch_init_vcpu(), QEMU will
exit. But if KVM_SET_TSC_KHZ fails in the migration, QEMU will not
abort. And because the return value of
kvm_arch_put_registers(KVM_PUT_FULL_STATE) is not checked by its
caller do_kvm_cpu_synchronize_post_init(), I have to handle them in
different ways.

Haozhong

> > +        if (r < 0) {
> > +            error_report("warning: TSC frequency mismatch between VM and 
> > host, "
> > +                         "and TSC scaling unavailable");
> > +        }
> > +    }
> > +
> > +    /*
> > +     * If neither the user-specified TSC frequency nor the migrated
> > +     * TSC frequency is present, we will try to set env->tsc_khz
> > +     * to the value used by KVM.
> >       */
> >      if (!env->tsc_khz) {
> >          r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index a18e16e..eb062d2 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
> >      CPUX86State *env = &cpu->env;
> >      int i;
> >  
> > +    if (env->tsc_khz && env->user_tsc_khz &&
> > +        env->tsc_khz != env->user_tsc_khz) {
> > +        fprintf(stderr, "Mismatch between user-specified TSC frequency and 
> > "
> > +                "migrated TSC frequency");
> > +        return -1;
> > +    }
> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
> >      }
> >  };
> >  
> > +static bool tsc_khz_needed(void *opaque)
> > +{
> > +    X86CPU *cpu = opaque;
> > +    CPUX86State *env = &cpu->env;
> > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> > +    return env->tsc_khz && pcmc->save_tsc_khz;
> > +}
> > +
> > +static const VMStateDescription vmstate_tsc_khz = {
> > +    .name = "cpu/tsc_khz",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = tsc_khz_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  VMStateDescription vmstate_x86_cpu = {
> >      .name = "cpu",
> >      .version_id = 12,
> > @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
> >          &vmstate_msr_hyperv_runtime,
> >          &vmstate_avx512,
> >          &vmstate_xss,
> > +        &vmstate_tsc_khz,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo