Re: [Qemu-devel] [PATCH v5 2/3] target-i386: reorganize TSC rate setting code

[Haozhong Zhang]

On 11/17/15 11:32, Eduardo Habkost wrote:
> On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote:
> > Following two changes are made to the TSC rate setting code in
> > kvm_arch_init_vcpu():
> >  * The code is moved to a new function kvm_arch_set_tsc_khz().
> >  * If setting user-specified TSC rate fails and the host TSC rate is
> >    inconsistent with the user-specified one, print a warning message.
> > 
> > Signed-off-by: Haozhong Zhang <address@hidden>
> 
> This matches what I was expecting, and now I see that we don't
> even need the user_tsc_khz field.
>

I guess you mean the user_tsc_khz field is not needed when setting TSC
rate. It's still needed in patch 3 to check if the migrated TSC rate
is consistent with the user-specified TSC rate (and of course it's not
in kvm_arch_set_tsc_khz()).

> > ---
> >  target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 9e4d27f..6a1acb4 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu)
> >              cpu->hyperv_runtime);
> >  }
> >  
> > +/**
> > + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz.
> > + *
> > + * Returns: 0        if successful;
> > + *          -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable;
> > + *          -EIO     if KVM_SET_TSC_KHZ fails.
> 
> If KVM_SET_TSC_KHZ fails, the error code will be useful to
> understand what went wrong. It's better to return the error code
> returned by KVM instead of -EIO.
>

Yes, I'll change in the next version.

> > + */
> > +static int kvm_arch_set_tsc_khz(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int has_tsc_control, r = 0;
> > +
> > +    if (!env->tsc_khz) {
> > +        return 0;
> > +    }
> > +
> > +    has_tsc_control = kvm_check_extension(cs->kvm_state, 
> > KVM_CAP_TSC_CONTROL);
> > +    if (has_tsc_control) {
> > +        r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +    }
> > +
> > +    if (!has_tsc_control || r < 0) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> > +        if (r <= 0 || r != env->tsc_khz) {
> > +            error_report("warning: TSC frequency mismatch between "
> > +                         "VM and host, and TSC scaling unavailable");
> > +            return has_tsc_control ? -EIO : -ENOTSUP;
> > +        }
> > +    }
> 
> What about:
> 
>     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) {
>         /* If KVM_SET_TSC_KHZ fails, it is an error only if the
>          * current TSC frequency doesn't match the one we want.
>          */
>         int cur_freq = kvm_check_extension(cs->kvm_state, 
> KVM_CAP_GET_TSC_KHZ) ?
>                        kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) :
>                        -ENOTSUP;
>        if (cur_freq <= 0 || cur_freq != env->tsc_khz) {
>            error_report("warning: TSC frequency mismatch between "
>                         "VM and host, and TSC scaling unavailable");
>            return r;
>        }
>     }
> 
>     return 0;
>

Yes, your suggestion is better.

> > +
> > +    return 0;
> > +}
> > +
> >  static Error *invtsc_mig_blocker;
> >  
> >  #define KVM_MAX_CPUID_ENTRIES  100
> > @@ -823,13 +858,9 @@ 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_arch_set_tsc_khz(cs) == -EIO) {
> > +        fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> Now kvm_arch_set_tsc_khz() prints an error message, we can remove
> this one.

will remove in the next version.

> 
> > +        return -EIO;
> 
> To keep the previous behavior without losing the error code
> returned by KVM, this could be written as:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0 && r != -ENOTSUP) {
>         return r;
>     }
> 
> But I belive the previous behavior was wrong. If tsc-freq was
> explicitly set by the user, we should abort if
> KVM_CAP_TSC_CONTROL is unavailable.
> 
> So I suggest we simply do this:
> 
>     r = kvm_arch_set_tsc_khz(cs);
>     if (r < 0) {
>         return r;
>     }
>

Yes, I also prefer this one. I'll change and update the commit message
in the next version.

Thanks,
Haozhong

> (In case you implement this behavior change in the same patch,
> please mention that in the commit message.)
> 
> -- 
> Eduardo