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

[haozhong . zhang]

On 11/05/15 14:05, Eduardo Habkost wrote:
> 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 <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.
> 
> 
> 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);
>     }
>

Consider an example that we migrate a VM from machine A to machine B
and then to machine C, and QEMU on machine B is launched with the cpu
option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
beginning):
 1) In the migration from B to C, the user-specified TSC frequency by
    'tsc-freq' on B is expected to be migrated to C. That is, the
    value of env->tsc_khz on B is migrated.
 2) If TSC frequency is migrated through env->tsc_khz, then
    env->tsc_khz on B will be overrode in the migration from A to B
    before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
    different than the user-specified TSC frequency on B, the
    expectation in 1) will not be satisfied anymore.

So, I introduce the field tsc_khz_saved to migrate TSC frequency and
(in patch 3) let the destination decide if this migrated one will be
used.

And, adding a flag like tsc_freq_requested_by_user in your comments on
patch 3 does not solve the problem.

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

The reason to duplicate tsc_khz is explained above. The reason to
duplicate of SET_TSC_KHZ code will be in the reply to your comments on
patch 3.

Haozhong

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