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 <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);
> }
>
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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
