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

[Haozhong Zhang]

On 11/11/15 13:23, Eduardo Habkost wrote:
> 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.
>

Got it, only abort when there are contradicts.

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

Yes, I really should put the sanity check in cpu_post_load().

Thanks,
Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this 

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

[Eduardo Habkost]

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

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

[Haozhong Zhang]

On 11/11/15 12:54, Eduardo Habkost wrote:
> 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, 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.
> > 
> > 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).
> > 

Re: [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, 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.
> 
> 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 

Re: [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, 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?

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

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

[Eduardo Habkost]

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.

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

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

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabk...@redhat.com) 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.

Yes, if it's a bad config please abort the migration and put a clear
message in the log so we can tell easily.

Dave

> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
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

Re: [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, 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.
> 
> -- 
> Eduardo

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

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

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

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

[haozhong . zhang]

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.

Haozhong

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

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

[Eduardo Habkost]

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.

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

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

[Haozhong Zhang]

On 11/05/15 09:05, Christian Borntraeger wrote:
> Am 02.11.2015 um 10:40 schrieb James Hogan:
> > 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 
> >> ---
> >>  include/sysemu/kvm.h |  2 ++
> >>  kvm-all.c|  1 +
> >>  target-arm/kvm.c |  5 +
> >>  target-i386/kvm.c| 33 +
> >>  target-mips/kvm.c|  5 +
> >>  target-ppc/kvm.c |  5 +
> >>  target-s390x/kvm.c   |  5 +
> >>  7 files changed, 56 insertions(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 461ef65..0ec8b98 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct 
> >> kvm_irq_routing_entry *route,
> >>  
> >>  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >>  
> >> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> >> +
> >>  int kvm_set_irq(KVMState *s, int irq, int level);
> >>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >>  
> >> diff --git a/kvm-all.c b/kvm-all.c
> >> index c442838..1ecaf04 100644
> >> --- a/kvm-all.c
> >> +++ b/kvm-all.c
> >> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void 
> >> *arg)
> >>  {
> >>  CPUState *cpu = arg;
> >>  
> >> +kvm_arch_setup_tsc_khz(cpu);
> > 
> > Sorry if this is a stupid question, but why aren't you doing this from
> > the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> > than introducing x86 specifics to the generic KVM api?
> > 
> > Cheers
> > James
> 
> I agree. We should try to keep this in x86 code.
> 
> 

As in another reply, I'm going to move the above line to
kvm_arch_put_registers() of target-i386 so that it will not pollute
other targets.

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

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

[Christian Borntraeger]

Am 02.11.2015 um 10:40 schrieb James Hogan:
> 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 
>> ---
>>  include/sysemu/kvm.h |  2 ++
>>  kvm-all.c|  1 +
>>  target-arm/kvm.c |  5 +
>>  target-i386/kvm.c| 33 +
>>  target-mips/kvm.c|  5 +
>>  target-ppc/kvm.c |  5 +
>>  target-s390x/kvm.c   |  5 +
>>  7 files changed, 56 insertions(+)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 461ef65..0ec8b98 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct 
>> kvm_irq_routing_entry *route,
>>  
>>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>>  
>> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
>> +
>>  int kvm_set_irq(KVMState *s, int irq, int level);
>>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>>  
>> diff --git a/kvm-all.c b/kvm-all.c
>> index c442838..1ecaf04 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>>  {
>>  CPUState *cpu = arg;
>>  
>> +kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James

I agree. We should try to keep this in x86 code.


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

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

[Eduardo Habkost]

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 
> > [...]
> > > 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 = >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
--
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

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

[haozhong . zhang]

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 
> > > [...]
> > > > 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 = >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 

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

[Eduardo Habkost]

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 
[...]
> 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 = >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.

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


> +
> +return 0;
> +}

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

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

[Haozhong Zhang]

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 
> [...]
> > 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 = >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. 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.

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

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?

Haozhong

> 
> > +
> > +return 0;
> > +}
> 
> -- 
> 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
--
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

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

[Haozhong Zhang]

On Mon, Nov 02, 2015 at 09:40:18AM +, James Hogan 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 
> > ---
> >  include/sysemu/kvm.h |  2 ++
> >  kvm-all.c|  1 +
> >  target-arm/kvm.c |  5 +
> >  target-i386/kvm.c| 33 +
> >  target-mips/kvm.c|  5 +
> >  target-ppc/kvm.c |  5 +
> >  target-s390x/kvm.c   |  5 +
> >  7 files changed, 56 insertions(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 461ef65..0ec8b98 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct 
> > kvm_irq_routing_entry *route,
> >  
> >  int kvm_arch_msi_data_to_gsi(uint32_t data);
> >  
> > +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> > +
> >  int kvm_set_irq(KVMState *s, int irq, int level);
> >  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
> >  
> > diff --git a/kvm-all.c b/kvm-all.c
> > index c442838..1ecaf04 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void 
> > *arg)
> >  {
> >  CPUState *cpu = arg;
> >  
> > +kvm_arch_setup_tsc_khz(cpu);
> 
> Sorry if this is a stupid question, but why aren't you doing this from
> the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
> than introducing x86 specifics to the generic KVM api?
> 
> Cheers
> James
>

Yes, I could call kvm_arch_setup_tsc_khz() in kvm_arch_put_registers()
of target-i386 when level == KVM_PUT_FULL_STATE, so that I will not need
to make kvm_arch_setup_tsc_khz() a generic KVM API (which looks weird
for targets other than i386).

Thanks, James!

Haozhong

> >  kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> >  cpu->kvm_vcpu_dirty = false;
> >  }
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index 79ef4c6..a724f6d 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >  return (data - 32) & 0x;
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +return 0;
> > +}
> > 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 = >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;
> > +}
> > +env->tsc_khz_saved = r;
> > +}
> > +
> > +return 0;
> > +}
> > diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> > index 12d7db3..fb26d7e 100644
> > --- a/target-mips/kvm.c
> > +++ b/target-mips/kvm.c
> > @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >  abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +return 0;
> > +}
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index ac70f08..c429f0c 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
> >  
> >  return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +return 0;
> > +}
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index c3be180..db5d436 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >  {
> >  abort();
> >  }
> > +
> > +int kvm_arch_setup_tsc_khz(CPUState *cs)
> > +{
> > +return 0;
> > +}
> > -- 
> > 2.4.8
> > 


--
To 

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

[James Hogan]

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 
> ---
>  include/sysemu/kvm.h |  2 ++
>  kvm-all.c|  1 +
>  target-arm/kvm.c |  5 +
>  target-i386/kvm.c| 33 +
>  target-mips/kvm.c|  5 +
>  target-ppc/kvm.c |  5 +
>  target-s390x/kvm.c   |  5 +
>  7 files changed, 56 insertions(+)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 461ef65..0ec8b98 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -328,6 +328,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry 
> *route,
>  
>  int kvm_arch_msi_data_to_gsi(uint32_t data);
>  
> +int kvm_arch_setup_tsc_khz(CPUState *cpu);
> +
>  int kvm_set_irq(KVMState *s, int irq, int level);
>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index c442838..1ecaf04 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1757,6 +1757,7 @@ static void do_kvm_cpu_synchronize_post_init(void *arg)
>  {
>  CPUState *cpu = arg;
>  
> +kvm_arch_setup_tsc_khz(cpu);

Sorry if this is a stupid question, but why aren't you doing this from
the i386 kvm_arch_put_registers when level == KVM_PUT_FULL_STATE, rather
than introducing x86 specifics to the generic KVM api?

Cheers
James

>  kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
>  cpu->kvm_vcpu_dirty = false;
>  }
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..a724f6d 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -614,3 +614,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>  return (data - 32) & 0x;
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +return 0;
> +}
> 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 = >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;
> +}
> +env->tsc_khz_saved = r;
> +}
> +
> +return 0;
> +}
> diff --git a/target-mips/kvm.c b/target-mips/kvm.c
> index 12d7db3..fb26d7e 100644
> --- a/target-mips/kvm.c
> +++ b/target-mips/kvm.c
> @@ -687,3 +687,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>  abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +return 0;
> +}
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index ac70f08..c429f0c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2510,3 +2510,8 @@ int kvmppc_enable_hwrng(void)
>  
>  return kvmppc_enable_hcall(kvm_state, H_RANDOM);
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +return 0;
> +}
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index c3be180..db5d436 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -2248,3 +2248,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>  abort();
>  }
> +
> +int kvm_arch_setup_tsc_khz(CPUState *cs)
> +{
> +return 0;
> +}
> -- 
> 2.4.8
> 


signature.asc
Description: Digital signature