| Message ID | 1446456403-29909-4-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote: > Set vcpu's TSC rate to the migrated value if the user does not specify a > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If > KVM supports TSC scaling, guest programs will observe TSC increasing in > the migrated rate other than the host TSC rate. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > target-i386/kvm.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index aae5e58..2be70df 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs) > int r; > > /* > + * If a TSC rate is migrated and the user does not specify the > + * vcpu's TSC rate on the destination, the migrated TSC rate will > + * be used on the destination after the migration. > + */ > + if (env->tsc_khz_saved && !env->tsc_khz) { > + if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) { > + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved); Why are you duplicating the existing KVM_SET_TSC_KHZ code in kvm_arch_init_vcpu()? > + if (r < 0) { > + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); If you want to report errors, please use error_report(). (But I don't think we want to print those warnings. See below.) > + } > + } else { > + r = -1; > + fprintf(stderr, "KVM doesn't support TSC scaling\n"); > + } > + if (r < 0) { > + fprintf(stderr, "Use host TSC frequency instead. " Did you mean "Using host TSC frequency instead."? > + "Guest TSC may be inaccurate.\n"); > + } > + } This will make QEMU print a warning every single time when migrating to hosts that don't support TSC scaling, even if the source and destination hosts already have the same TSC frequency. That means most users will see a bogus warning, in today's hardware. Maybe it will be acceptable to print a warning if (and only if) we know that the host TSC is different from the original TSC frequency. Considering that we already have code to handle tsc_khz that prints an error, you don't need to duplicate it. You could handle both user-provided and migration tsc_khz cases with the same code. With something like this: if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */ r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ? kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) : -ENOTSUP; if (r < 0) { int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ? kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) : 0; /* If we know the host frequency, print a warning every time * there's a mismatch. * If we don't know the host frequency, print a warning only * if the user asked for a specific TSC frequency. */ if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) || (cur_freq > 0 && cur_freq != env->tsc_khz)) { error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable"); if (env->tsc_freq_set_by_user) { return r; } } } } You will just need a new tsc_freq_requested_by_user field to track if the TSC frequency was explicitly requested by the user.
On 11/05/15 14:10, Eduardo Habkost wrote: > On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote: > > Set vcpu's TSC rate to the migrated value if the user does not specify a > > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If > > KVM supports TSC scaling, guest programs will observe TSC increasing in > > the migrated rate other than the host TSC rate. > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > --- > > target-i386/kvm.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > index aae5e58..2be70df 100644 > > --- a/target-i386/kvm.c > > +++ b/target-i386/kvm.c > > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs) > > int r; > > > > /* > > + * If a TSC rate is migrated and the user does not specify the > > + * vcpu's TSC rate on the destination, the migrated TSC rate will > > + * be used on the destination after the migration. > > + */ > > + if (env->tsc_khz_saved && !env->tsc_khz) { > > + if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) { > > + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved); > > Why are you duplicating the existing KVM_SET_TSC_KHZ code in > kvm_arch_init_vcpu()? > Because they are called in different cases and their behaviors on failure are different: 1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM is created and a user-specified TSC frequency is given. If it fails, QEMU will abort. 2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the destination only when TSC frequency is migrated and no user-specified TSC frequency is given. If it fails, QEMU as well as the migration will not be aborted. However, after reading your comment at the end, they really could be merged. > > + if (r < 0) { > > + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); > > If you want to report errors, please use error_report(). > > (But I don't think we want to print those warnings. See below.) > > > + } > > + } else { > > + r = -1; > > + fprintf(stderr, "KVM doesn't support TSC scaling\n"); > > + } > > + if (r < 0) { > > + fprintf(stderr, "Use host TSC frequency instead. " > > Did you mean "Using host TSC frequency instead."? > Yes. > > + "Guest TSC may be inaccurate.\n"); > > + } > > + } > > This will make QEMU print a warning every single time when migrating to > hosts that don't support TSC scaling, even if the source and destination > hosts already have the same TSC frequency. That means most users will > see a bogus warning, in today's hardware. > > Maybe it will be acceptable to print a warning if (and only if) we know > that the host TSC is different from the original TSC frequency. > Agree, I should add such a check to avoid bogus warnings. > Considering that we already have code to handle tsc_khz that prints an > error, you don't need to duplicate it. You could handle both > user-provided and migration tsc_khz cases with the same code. With > something like this: > Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make some minor changes. - if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */ + if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */ + int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved; > r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ? - kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) : + kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) : > -ENOTSUP; > if (r < 0) { > int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ? > kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) : > 0; > /* If we know the host frequency, print a warning every time > * there's a mismatch. > * If we don't know the host frequency, print a warning only > * if the user asked for a specific TSC frequency. > */ - if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) || + if ((cur_freq <= 0 && env->tsc_khz) || - (cur_freq > 0 && cur_freq != env->tsc_khz)) { + (cur_freq > 0 && cur_freq != tgt_tsc_khz)) { > error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable"); - if (env->tsc_freq_set_by_user) { + if (env->tsc_khz) { > return r; > } > } > } > } > Haozhong > You will just need a new tsc_freq_requested_by_user field to track if > the TSC frequency was explicitly requested by the user. > > -- > 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, Nov 06, 2015 at 10:32:44AM +0800, Haozhong Zhang wrote: > On 11/05/15 14:10, Eduardo Habkost wrote: > > On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote: > > > Set vcpu's TSC rate to the migrated value if the user does not specify a > > > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If > > > KVM supports TSC scaling, guest programs will observe TSC increasing in > > > the migrated rate other than the host TSC rate. > > > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > > --- > > > target-i386/kvm.c | 21 +++++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > > index aae5e58..2be70df 100644 > > > --- a/target-i386/kvm.c > > > +++ b/target-i386/kvm.c > > > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs) > > > int r; > > > > > > /* > > > + * If a TSC rate is migrated and the user does not specify the > > > + * vcpu's TSC rate on the destination, the migrated TSC rate will > > > + * be used on the destination after the migration. > > > + */ > > > + if (env->tsc_khz_saved && !env->tsc_khz) { > > > + if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) { > > > + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved); > > > > Why are you duplicating the existing KVM_SET_TSC_KHZ code in > > kvm_arch_init_vcpu()? > > > > Because they are called in different cases and their behaviors on > failure are different: > 1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM > is created and a user-specified TSC frequency is given. If it > fails, QEMU will abort. > 2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the > destination only when TSC frequency is migrated and no > user-specified TSC frequency is given. If it fails, QEMU as well > as the migration will not be aborted. > > However, after reading your comment at the end, they really could be > merged. Agreed that the expected behavior ou failure is different. But it looks like we are now on the same page about not duplicating code, with the code you suggested below. :) > > > > + if (r < 0) { > > > + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); > > > > If you want to report errors, please use error_report(). > > > > (But I don't think we want to print those warnings. See below.) > > > > > + } > > > + } else { > > > + r = -1; > > > + fprintf(stderr, "KVM doesn't support TSC scaling\n"); > > > + } > > > + if (r < 0) { > > > + fprintf(stderr, "Use host TSC frequency instead. " > > > > Did you mean "Using host TSC frequency instead."? > > > > Yes. > > > > + "Guest TSC may be inaccurate.\n"); > > > + } > > > + } > > > > This will make QEMU print a warning every single time when migrating to > > hosts that don't support TSC scaling, even if the source and destination > > hosts already have the same TSC frequency. That means most users will > > see a bogus warning, in today's hardware. > > > > Maybe it will be acceptable to print a warning if (and only if) we know > > that the host TSC is different from the original TSC frequency. > > > > Agree, I should add such a check to avoid bogus warnings. > > > Considering that we already have code to handle tsc_khz that prints an > > error, you don't need to duplicate it. You could handle both > > user-provided and migration tsc_khz cases with the same code. With > > something like this: > > > > Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make > some minor changes. > > - if (env->tsc_khz) { /* may be set by the user, or loaded from incoming migration */ > + if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or loaded from incoming migration */ > + int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved; > > r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ? > - kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) : > + kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) : > > -ENOTSUP; > > if (r < 0) { > > int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ? > > kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) : > > 0; > > /* If we know the host frequency, print a warning every time > > * there's a mismatch. > > * If we don't know the host frequency, print a warning only > > * if the user asked for a specific TSC frequency. > > */ > - if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) || > + if ((cur_freq <= 0 && env->tsc_khz) || > - (cur_freq > 0 && cur_freq != env->tsc_khz)) { > + (cur_freq > 0 && cur_freq != tgt_tsc_khz)) { > > error_report("warning: TSC frequency mismatch between VM and host, and TSC scaling unavailable"); > - if (env->tsc_freq_set_by_user) { > + if (env->tsc_khz) { > > return r; > > } > > } > > } > > } > > If your assumption that tsc_khz_saved is necessary is correct, the changes above look good. But I am still not sure it is really needed (we can continue the discussoin in the other thread).
diff --git a/target-i386/kvm.c b/target-i386/kvm.c index aae5e58..2be70df 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs) int r; /* + * If a TSC rate is migrated and the user does not specify the + * vcpu's TSC rate on the destination, the migrated TSC rate will + * be used on the destination after the migration. + */ + if (env->tsc_khz_saved && !env->tsc_khz) { + if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) { + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved); + if (r < 0) { + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); + } + } else { + r = -1; + fprintf(stderr, "KVM doesn't support TSC scaling\n"); + } + if (r < 0) { + fprintf(stderr, "Use host TSC frequency instead. " + "Guest TSC may be inaccurate.\n"); + } + } + + /* * Prepare vcpu's TSC rate to be migrated. * * - If the user specifies the TSC rate by cpu option 'tsc-freq',
Set vcpu's TSC rate to the migrated value if the user does not specify a TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If KVM supports TSC scaling, guest programs will observe TSC increasing in the migrated rate other than the host TSC rate. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- target-i386/kvm.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)