Re: [Qemu-ppc] [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

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 <address@hidden>
> [...]
> > 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. 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.

> > +        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;

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?

Haozhong

> 
> > +
> > +    return 0;
> > +}
> 
> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html