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

[Eduardo Habkost]

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.

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

> + */
> +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;

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

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

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

-- 
Eduardo