Re: [PATCH v4 2/2] target-i386: add support to migrate vcpu's TSC rate

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Nov 16, 2015 at 10:30:08PM +0800, Haozhong Zhang wrote:
> On 11/16/15 11:43, Eduardo Habkost wrote:
> > On Mon, Nov 16, 2015 at 04:04:08PM +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@intel.com>
[...]
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 9084b29..8448248 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -2334,8 +2334,30 @@ static void kvm_arch_set_tsc_khz(CPUState *cs)
> > >      int r;
> > >  
> > >      /*
> > > -     * If no user-specified TSC frequency is present, we will try to
> > > -     * set env->tsc_khz to the value used by KVM.
> > > +     * For other cases of env->tsc_khz and env->user_tsc_khz:
> > > +     *
> > > +     * - We have eliminated all cases that satisfy
> > > +     *       env->tsc_khz && env->user_tsc_khz &&
> > > +     *       env->tsc_khz != env->user_tsc_khz
> > > +     *   in cpu_post_load().
> > > +     *
> > > +     * - If env->user_tsc_khz is not zero, then it must be equal to
> > > +     *   env->tsc_khz (if the latter is not zero) and has been set in
> > > +     *   kvm_arch_init_vcpu().
> > > +     */
> > > +    if (env->tsc_khz && !env->user_tsc_khz) {
> > > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> > > +            kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) : -ENOTSUP;
> > 
> > Please don't duplicate the code from kvm_arch_init_vcpu(). We can
> > handle both cases in kvm_arch_put_registers(KVM_PUT_FULL_STATE),
> > can't we?
> >
> 
> No. If KVM_SET_TSC_KHZ fails in kvm_arch_init_vcpu(), QEMU will
> exit. But if KVM_SET_TSC_KHZ fails in the migration, QEMU will not
> abort. And because the return value of
> kvm_arch_put_registers(KVM_PUT_FULL_STATE) is not checked by its
> caller do_kvm_cpu_synchronize_post_init(), I have to handle them in
> different ways.

Reporting errors back in kvm_put_registers() may be difficult, I
see, so handling user-provided TSC frequency in
kvm_arch_init_vcpu() makes sense. But you can still avoid code
duplication. Just reuse the same function in kvm_arch_init_vcpu()
and kvm_put_registers(), but return errors back to the caller in
kvm_arch_init_vcpu() in case env->user_tsc_khz is set.

kvm_put_registers() can ignore the error, and just print a
warning. But (on both cases) we should print a warning only if
env->tsc_khz doesn't match KVM_GET_TSC_KHZ, because we don't want
to print spurious warnings on every migration when TSC scaling
isn't supported. (You even suggested changes to the example code
that does that, at Message-ID:
<20151106023244.GB24388@hzzhang-OptiPlex-9020.sh.intel.com>).

Also, I believe it won't be a problem if we call KVM_SET_TSC_KHZ
twice in the case of incoming migration, so there's no need to
check user_tsc_khz in the kvm_arch_put_registers() path. Keeping
the code simple is more important than avoiding one extra ioctl()
on incoming migration, IMO.

-- 
Eduardo