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

[Haozhong Zhang]

On 11/09/15 14:01, Eduardo Habkost wrote:
> On Mon, Nov 09, 2015 at 08:33:55AM +0800, address@hidden wrote:
> > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, address@hidden wrote:
> > > > 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:
> > > [...]
> > > > > > > > +        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.
> > > 
> > > Setting tsc-freq on B when tsc-freq was not used on A is invalid usage.
> > > This is not different from changing the CPU model and adding or removing
> > > CPU flags when migrating, which is also incorrect. The command-line
> > > parameters defining the VM must be the same when you migrate.
> > >
> > 
> > Good to know it's an invalid usage. Then the question is what QEMU is
> > expected to do for this invalid usage?
> > 
> >  1) Abort the migration? But I find that the current QEMU does not
> >     abort the migration between different CPU models (e.g. Nehalem and
> >     Haswell).
> > 
> >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> >     tsc_khz_saved will be not needed.
> 
> My first choice is to abort migration. If we decide to abort today and
> find it to cause problems, we can easily fix it. If we decide to
> continue without aborting, it is difficult to change that behavior
> without breaking existing setups.
>

Two additional questions:

 1) Existing QEMU allows 'tsc-freq' on the destination in the
    migration. If we decided to abort when both 'tsc-freq' and
    migrated TSC were present on the destination, it would break some
    existing usages. Considering backward compatibility, would above
    choice 2) be better?

 2) If we do decide to abort, could I use abort()? Or are there other
    clean approaches to abort?

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