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

[Haozhong Zhang]

On 11/16/15 13:35, Eduardo Habkost wrote:
> 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 <address@hidden>
> [...]
> > > > 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.
>

Agree on using the same function to set TSC rate. I'll change in the
next version.

> 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:
> <address@hidden>).
>

I'll check whether env->tsc_khz == KVM_GET_TSC_KHZ in the next
version.

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

I'll use tsc_khz only in the next version.

Thanks,
Haozhong