Re: [Qemu-devel] [PATCH v5 3/3] target-i386: add support to migrate vcpu's TSC rate

[Eduardo Habkost]

Hi,

On Tue, Nov 17, 2015 at 01:20:39PM +0800, Haozhong Zhang wrote:
> This patch enables migrating vcpu's TSC rate. If KVM on the destination
> machine supports TSC scaling, guest programs will observe a consistent
> TSC rate across the migration.
> 
> If TSC scaling is not supported on the destination machine, the
> migration will not be aborted and QEMU on the destination will not set
> vcpu's TSC rate to the migrated value.
> 
> If vcpu's TSC rate specified by CPU option 'tsc-freq' on the destination
> machine is inconsistent with the migrated TSC rate, the migration will
> be aborted.
> 
> For backwards compatibility, the migration of vcpu's TSC rate is
> disabled on pc-*-2.4 and older machine types.
> 
> Signed-off-by: Haozhong Zhang <address@hidden>

Now the logic in this patch (and the rest of the series) looks
good to me. All my suggestions are only related to code comments
and error handling:

[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6a1acb4..6856899 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2384,6 +2384,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>          }
>      }
>  
> +    if (level == KVM_PUT_FULL_STATE) {
> +        kvm_arch_set_tsc_khz(cpu);

Please add a comment here indicating that errors are being
ignored, and explaining why.

> +    }
> +
>      ret = kvm_getput_regs(x86_cpu, 1);
>      if (ret < 0) {
>          return ret;
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index a18e16e..3c5d24b 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -331,6 +331,13 @@ static int cpu_post_load(void *opaque, int version_id)
>      CPUX86State *env = &cpu->env;
>      int i;
>  
> +    if (env->tsc_khz && env->user_tsc_khz &&
> +        env->tsc_khz != env->user_tsc_khz) {
> +        fprintf(stderr, "Mismatch between user-specified TSC frequency and "
> +                "migrated TSC frequency\n");

Please use error_report() instead of fprintf().

> +        return -1;

Please return a valid -errno value. Other post_load functions
that implement sanity checks use -EINVAL (e.g.
global_state_post_load(), configuration_post_load()), so that's
probably what we should do here.

> +    }
> +
>      /*
>       * Real mode guest segments register DPL should be zero.
>       * Older KVM version were setting it wrongly.
> @@ -775,6 +782,26 @@ static const VMStateDescription vmstate_xss = {
>      }
>  };
>  
> +static bool tsc_khz_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> +    return env->tsc_khz && pcmc->save_tsc_khz;
> +}
> +
> +static const VMStateDescription vmstate_tsc_khz = {
> +    .name = "cpu/tsc_khz",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = tsc_khz_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT64(env.tsc_khz, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  VMStateDescription vmstate_x86_cpu = {
>      .name = "cpu",
>      .version_id = 12,
> @@ -895,6 +922,7 @@ VMStateDescription vmstate_x86_cpu = {
>          &vmstate_msr_hyperv_runtime,
>          &vmstate_avx512,
>          &vmstate_xss,
> +        &vmstate_tsc_khz,
>          NULL
>      }
>  };
> -- 
> 2.4.8
> 

-- 
Eduardo