On 11/17/15 11:40, Eduardo Habkost wrote:
> 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 <haozhong.zhang@xxxxxxxxx>
>
> 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.
>
will add
> > + }
> > +
> > 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().
>
will change
> > + 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.
>
will change
Thanks,
Haozhong
> > + }
> > +
> > /*
> > * 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
