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 <haozhong.zhang@xxxxxxxxx> > [...] > > > > 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: > <20151106023244.GB24388@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>). > 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 -- 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