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 <haozhong.zhang@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
