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

[Eduardo Habkost]

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()?

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

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

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:

    if (env->tsc_khz) { /* may be set by the user, or loaded from incoming 
migration */
        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) {
            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) ||
                (cur_freq > 0 && cur_freq != env->tsc_khz)) {
                error_report("warning: TSC frequency mismatch between VM and 
host, and TSC scaling unavailable");
                if (env->tsc_freq_set_by_user) {
                    return r;
                }
            }
        }
    }

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