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

[Haozhong Zhang]

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.

> > +            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