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

[Eduardo Habkost]

On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> 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.
> 
> Agree, but I would like to relax the abort condition to "abort the
> migration only if QEMU on the destination uses a different TSC
> frequency than the migrated one" so that the following usages would be
> still valid:
>  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
>     the same value of the migrated one.
>  2) Only QEMU on the source has 'tsc-freq' option.
>  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
>     same value.
> In all above usages, TSC frequency on the destination is the same as
> both the value on the source and the value explicitly expected by
> users on the destination (by 'tsc-freq' on the destination).

Yes, that's probably all we can do because we don't really know
the command-line on the source. All we know is that the user is
asking for a TSC frequency that doesn't match what we see in the
migration stream.

> 
> And I still need tsc_khz_saved to tell on the destination whether
>  1) both tsc-freq option and migrated TSC frequency are present, and
>  2) above two values are the same.
> Even though we restrictively requires QEMU on both sides use the same
> CPU options, tsc_khz_saved is still needed because of 1).
> 

So, it looks like we need an extra field because of two things:
the migration sanity check, and SET_TSC_KHZ error handling. But
what bothers me is that we have two redundant fields that could
contradict each other, and it is not clear which one we should
use on which case. Sometimes only tsc_khz is valid, sometimes
only tsc_khz is valid.

What if we set/use tsc_khz on all code (no exceptions), but add a
new field just to indicate if tsc-freq was set by the user?
Something like:

* SET_TSC_KHZ code uses tsc_khz only;
* "tsc-freq" property getter returns tsc_khz;
* VMState saves/loads tsc_khz only;
* Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
  case tsc_khz is not set.

And a new user_tsc_khz field would be added and used only for
error checking:
* "tsc-freq" property setter changes both tsc_khz and
  user_tsc_khz;
* Sanity check code post migration-load ensures that
  (!user_tsc_khz || tsc_khz == user_tsc_khz);
* SET_TSC_KHZ error handling returns error if user_tsc_khz
  is set.

-- 
Eduardo