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

[Eduardo Habkost]

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 <address@hidden>
> > > ---
> > >  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