qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate t


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated
Date: Thu, 5 Nov 2015 14:05:01 -0200
User-agent: Mutt/1.5.23 (2014-03-12)

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);
    }


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]