Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate

[Haozhong Zhang]

On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> > 
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
> 
> Why do we need an option? Why can't we enable loading unconditionally?
>

If TSC scaling is not supported by KVM and CPU, unconditionally
enabling this loading will not take effect which would be different
from users' expectation. 'load-tsc-freq' is introduced to allow users
to enable the loading of migrated TSC frequency if they do know the
underlying KVM and CPU have TSC scaling support.

> > 
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> > 
> > Signed-off-by: Haozhong Zhang <address@hidden>
> > ---
> >  target-i386/cpu.c |  1 +
> >  target-i386/cpu.h |  1 +
> >  target-i386/kvm.c | 28 +++++++++++++++++++---------
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> >      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >      DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +    DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> >      int64_t tsc_khz;
> >      int64_t tsc_khz_incoming;
> >      bool save_tsc_khz;
> > +    bool load_tsc_khz;
> >      void *kvm_xsave_buf;
> >  
> >      uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          return r;
> >      }
> >  
> > -    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > -    if (r && env->tsc_khz) {
> > -        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > -        if (r < 0) {
> > -            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > -            return r;
> > -        }
> > -    }
> > -
> >      if (kvm_has_xsave()) {
> >          env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> >      }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> >          return 0;
> >  
> >      /*
> > +     * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate 
> > in the
> > +     * migrated state will be used and the overrides the user-specified 
> > vcpu's
> > +     * TSC rate (if any).
> > +     */
> > +    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +        env->load_tsc_khz && env->tsc_khz_incoming) {
> > +        env->tsc_khz = env->tsc_khz_incoming;
> > +    }
> 
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
> 
> > +
> > +    r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > +    if (r && env->tsc_khz) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +        if (r < 0) {
> > +            fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > +            return r;
> > +        }
> > +    }
> 
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
> 
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
> 
> -- 
> Eduardo