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@xxxxxxxxx>
> > > ---
> > > 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).
--
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
