On Wed, Nov 11, 2015 at 12:57:44AM +0800, Haozhong Zhang wrote:
> On 11/09/15 14:01, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2015 at 08:33:55AM +0800, haozhong.zh...@intel.com wrote:
> > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, haozhong.zh...@intel.com 
> > > > 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?

We shouldn't abort simply because the section is present and tsc-freq is
set (because we will always send the section in the newer
machine-types). We should abort only when we know that the command-line
contradicts what we see in the migration stream.

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

You don't need to abort QEMU. You just need to tell the migration code
that migration can't continue. The exact way to do it depends on where
you are hooking the sanity check code. If you use a
VMStateDescription.post_load hook, you can use error_report() and return
a negative errno value. CCing Quintela in case he has suggestions.

-- 
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to