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

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 11/26/15 12:19, Eduardo Habkost wrote:
> On Tue, Nov 24, 2015 at 11:33:57AM +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 <haozhong.zhang@intel.com>
> 
> Assuming the PC compat code will be moved to
> pc_*_2_5_machine_options(), because the patch will be included
> after QEMU 2.5.0:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> One comment below:

Hi Eduardo,

Thank you for reviewing!

Besides the comment, should I submit a new version which updates the
compat code after pc-*-2.6 machine types are added?

Haozhong

> 
> > ---
> [...]
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 1e811ee..2a0fd54 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2381,6 +2381,28 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >          }
> >      }
> >  
> > +    if (level == KVM_PUT_FULL_STATE) {
> > +        /* kvm_arch_set_tsc_khz() below can be called in two control flows and
> > +         * we don't need to handle its errors in both of them.
> > +         *
> > +         * One is the control flow that creates a vcpu, where
> > +         * kvm_arch_set_tsc_khz() has already been called once before by
> > +         * kvm_arch_init_vcpu(). The latter will abort the control flow if there
> > +         * are any errors of kvm_arch_set_tsc_khz(). Thus, in this control flow,
> > +         * kvm_arch_set_tsc_khz() below never fails and we can safely ignore its
> > +         * return values here.
> > +         *
> > +         * Another is the control flow of migration that sets vcpu's TSC
> > +         * frequency on the destination. The only error that can fail the
> > +         * migration is the mismatch between the migrated and the user-specified
> > +         * TSC frequencies, which has been handled by cpu_post_load(). Other
> > +         * errors, i.e. those from kvm_arch_set_tsc_khz(), never fail the
> > +         * migration, so we also safely ignore its return values in this control
> > +         * flow.
> > +         */
> 
> This could be more succint. Something like:
> 
> /* 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).
>  */
> 
> No need to resubmit because of that, though. The comment can be
> changed when applying the patch.
> 
> > +        kvm_arch_set_tsc_khz(cpu);
> > +    }
> > +
> >      ret = kvm_getput_regs(x86_cpu, 1);
> >      if (ret < 0) {
> >          return ret;
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index a18e16e..e560ca3 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -6,6 +6,8 @@
> >  #include "cpu.h"
> >  #include "sysemu/kvm.h"
> >  
> > +#include "qemu/error-report.h"
> > +
> >  static const VMStateDescription vmstate_segment = {
> >      .name = "segment",
> >      .version_id = 1,
> > @@ -331,6 +333,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) {
> > +        error_report("Mismatch between user-specified TSC frequency and "
> > +                     "migrated TSC frequency");
> > +        return -EINVAL;
> > +    }
> > +
> >      /*
> >       * Real mode guest segments register DPL should be zero.
> >       * Older KVM version were setting it wrongly.
> > @@ -775,6 +784,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 +924,7 @@ VMStateDescription vmstate_x86_cpu = {
> >          &vmstate_msr_hyperv_runtime,
> >          &vmstate_avx512,
> >          &vmstate_xss,
> > +        &vmstate_tsc_khz,
> >          NULL
> >      }
> >  };
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo