| Message ID | 1445325774-7195-4-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > > 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@intel.com> > --- > 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.)
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? > > > > > 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@intel.com> > > --- > > 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? > I can make kvm_setup_tsc_khz() called in do_kvm_cpu_synchronize_post_init() and also make empty stubs for other targets. > > + > > + 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.) > Really should abort QEMU here for both tsc-freq and migration. > -- > Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote: > 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. > I don't get your argument about user expectations. We can't read the user's mind, but let's enumerate all possible scenarios: * Host has TSC scaling, user expect TSC frequency to be set: * We set it. The user is happy. * Host has TSC scaling, user doesn't expect TSC frequency to be set: * We still set it. VM behaves better, guest doesn't see changing TSC frequency. User didn't expect it but won't be unhappy. * No TSC scaling, user expect TSC frequency to be set: * We won't set it, user will be unhappy. But I believe we all agree we shouldn't make QEMU abort migration by default on all hosts that don't support TSC scaling. * No TSC scaling, user doesn't expect TSC frequency to be set: * We don't set it. User is happy. Could you clarify on which items you disagree above, exactly?
On Fri, Oct 23, 2015 at 12:58:02PM -0200, Eduardo Habkost wrote: > On Fri, Oct 23, 2015 at 11:14:48AM +0800, Haozhong Zhang wrote: > > 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. > > > Sorry for the confusion, I changed my mind. The semantics of 'load-tsc-freq' is really confusing and we should not need it at all. Now, what I want to implement is to migrate the TSC frequency as much as possible. If it could not, QEMU does not abort the migration. > I don't get your argument about user expectations. We can't read the > user's mind, but let's enumerate all possible scenarios: > > * Host has TSC scaling, user expect TSC frequency to be set: > * We set it. The user is happy. Agree. > * Host has TSC scaling, user doesn't expect TSC frequency to be > set: > * We still set it. VM behaves better, guest doesn't see changing TSC > frequency. User didn't expect it but won't be unhappy. Agree. > * No TSC scaling, user expect TSC frequency to be set: > * We won't set it, user will be unhappy. But I believe we all agree > we shouldn't make QEMU abort migration by default on all hosts that > don't support TSC scaling. Agree and display warning messages. > * No TSC scaling, user doesn't expect TSC frequency to be set: > * We don't set it. User is happy. Agree. This is the current QEMU's behavior, so it's still acceptable. Thanks, Haozhong > > Could you clarify on which items you disagree above, exactly? > > -- > Eduardo > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + } + + 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; + } + } + + /* * Prepare the vcpu's TSC rate (ie. env->tsc_khz_incoming) to be migrated. * 1. If no user-specified value is provided, we will use the value from * KVM;
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. 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@intel.com> --- target-i386/cpu.c | 1 + target-i386/cpu.h | 1 + target-i386/kvm.c | 28 +++++++++++++++++++--------- 3 files changed, 21 insertions(+), 9 deletions(-)