On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> On 11/04/15 19:42, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:42PM +0800, Haozhong Zhang wrote:
> > > The value of the migrated vcpu's TSC rate is determined as below.
> > > 1. If a TSC rate is specified by the cpu option 'tsc-freq', then this
> > > user-specified value will be used.
> > > 2. If neither a user-specified TSC rate nor a migrated TSC rate is
> > > present, we will use the TSC rate from KVM (returned by
> > > KVM_GET_TSC_KHZ).
> > > 3. Otherwise, we will use the migrated TSC rate.
> > >
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > [...]
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 64046cb..aae5e58 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3034,3 +3034,36 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> > > {
> > > abort();
> > > }
> > > +
> > > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > > +{
> > > + X86CPU *cpu = X86_CPU(cs);
> > > + CPUX86State *env = &cpu->env;
> > > + int r;
> > > +
> > > + /*
> > > + * Prepare vcpu's TSC rate to be migrated.
> > > + *
> > > + * - If the user specifies the TSC rate by cpu option 'tsc-freq',
> > > + * we will use the user-specified value.
> > > + *
> > > + * - If there is neither user-specified TSC rate nor migrated TSC
> > > + * rate, we will ask KVM for the TSC rate by calling
> > > + * KVM_GET_TSC_KHZ.
> > > + *
> > > + * - Otherwise, if there is a migrated TSC rate, we will use the
> > > + * migrated value.
> > > + */
> > > + if (env->tsc_khz) {
> > > + env->tsc_khz_saved = env->tsc_khz;
> > > + } else if (!env->tsc_khz_saved) {
> > > + r = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > + if (r < 0) {
> > > + fprintf(stderr, "KVM_GET_TSC_KHZ failed\n");
> > > + return r;
> > > + }
> >
> > The lack of KVM_CAP_GET_TSC_KHZ should make QEMU abort, unless the user
> > is explicitly requesting a more strict mode where the TSC frequency will
> > be guaranteed to never change.
> >
>
> I agree KVM_CAP_GET_TSC_KHZ should be checked before KVM_GET_TSC_KHZ,
> but I don't think the lack of it should abort QEMU.
Oops, I meant to write: "the lack of KVM_CAP_GET_TSC_KHZ should not
abort QEMU".
> This piece of code
> on the source machine is just to get the TSC frequency to be
> migrated. If it fails, it will leave env->tsc_khz_saved be 0. And
> according to tsc_khz_needed() in patch 1, if env->tsc_khz_saved == 0,
> no TSC frequency will be migrated. So the lack of KVM_CAP_GET_TSC_KHZ
> only hurts the migration and does not need to abort QEMU on the source
> machine.
The lack of KVM_CAP_GET_TSC_KHZ shouldn't prevent migration either. but
it looks your code is not doing that: errors from
kvm_arch_setup_tsc_khz() are being ignored by
do_kvm_cpu_synchronize_post_init(), sorry for the noise.
>
> > > + env->tsc_khz_saved = r;
> > > + }
> >
> > Why do you need a separate tsc_khz_saved field, and don't simply use
> > tsc_khz? It would have the additional feature of letting QMP clients
> > query the current TSC rate by asking for the tsc-freq property on CPU
> > objects.
> >
>
> It's to avoid overriding env->tsc_khz on the destination in the
> migration. I can change this line to
> env->tsc_khz = env->tsc_khz_saved = r;
You are already avoiding overriding env->tsc_khz, because you use
KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see why
you need a tsc_khz_saved field that requires duplicating the SET_TSC_KHZ
code, if you could just do this:
if (!env->tsc_khz) {
env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
}
>
> For the additional QMP feature, will the value of tsc-freq property be
> env->tsc_khz? If yes, I guess the above change would be fine?
It may work, but I still don't see the point of duplicating the field
and duplicating the existing SET_TSC_KHZ code.
--
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
