Re: [PATCH v3 3/3] target-i386: load the migrated vcpu's TSC rate

2015-11-06 Thread Eduardo Habkost
On Fri, Nov 06, 2015 at 10:32:44AM +0800, Haozhong Zhang wrote:
> On 11/05/15 14:10, Eduardo Habkost wrote:
> > On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > > the migrated rate other than the host TSC rate.
> > > 
> > > Signed-off-by: Haozhong Zhang 
> > > ---
> > >  target-i386/kvm.c | 21 +
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index aae5e58..2be70df 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> > >  int r;
> > >  
> > >  /*
> > > + * If a TSC rate is migrated and the user does not specify the
> > > + * vcpu's TSC rate on the destination, the migrated TSC rate will
> > > + * be used on the destination after the migration.
> > > + */
> > > +if (env->tsc_khz_saved && !env->tsc_khz) {
> > > +if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > > +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> > 
> > Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> > kvm_arch_init_vcpu()?
> >
> 
> Because they are called in different cases and their behaviors on
> failure are different:
>  1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
> is created and a user-specified TSC frequency is given. If it
> fails, QEMU will abort.
>  2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
> destination only when TSC frequency is migrated and no
> user-specified TSC frequency is given. If it fails, QEMU as well
> as the migration will not be aborted.
> 
> However, after reading your comment at the end, they really could be
> merged.

Agreed that the expected behavior ou failure is different. But it looks
like we are now on the same page about not duplicating code, with the
code you suggested below. :)

> 
> > > +if (r < 0) {
> > > +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > 
> > If you want to report errors, please use error_report().
> > 
> > (But I don't think we want to print those warnings. See below.)
> > 
> > > +}
> > > +} else {
> > > +r = -1;
> > > +fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > > +}
> > > +if (r < 0) {
> > > +fprintf(stderr, "Use host TSC frequency instead. "
> > 
> > Did you mean "Using host TSC frequency instead."?
> >
> 
> Yes.
> 
> > > +"Guest TSC may be inaccurate.\n");
> > > +}
> > > +}
> > 
> > This will make QEMU print a warning every single time when migrating to
> > hosts that don't support TSC scaling, even if the source and destination
> > hosts already have the same TSC frequency. That means most users will
> > see a bogus warning, in today's hardware.
> > 
> > Maybe it will be acceptable to print a warning if (and only if) we know
> > that the host TSC is different from the original TSC frequency.
> >
> 
> Agree, I should add such a check to avoid bogus warnings.
> 
> > Considering that we already have code to handle tsc_khz that prints an
> > error, you don't need to duplicate it. You could handle both
> > user-provided and migration tsc_khz cases with the same code. With
> > something like this:
> >
> 
> Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
> some minor changes.
> 
> - if (env->tsc_khz) { /* may be set by the user, or loaded from incoming 
> migration */
> + if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or 
> loaded from incoming migration */
> + int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
> > r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
> - kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
> + kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
> > -ENOTSUP;
> > if (r < 0) {
> > int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
> >kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
> >0;
> > /* If we know the host frequency, print a warning every time
> >  * there's a mismatch.
> >  * If we don't know the host frequency, print a warning only
> >  * if the user asked for a specific TSC frequency.
> >  */
> - if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
> + if ((cur_freq <= 0 && env->tsc_khz) ||
> - (cur_freq > 0 && cur_freq != env->tsc_khz)) {
> + (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
> >

Re: [PATCH v3 3/3] target-i386: load the migrated vcpu's TSC rate

2015-11-05 Thread Eduardo Habkost
On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value if the user does not specify a
> TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> KVM supports TSC scaling, guest programs will observe TSC increasing in
> the migrated rate other than the host TSC rate.
> 
> Signed-off-by: Haozhong Zhang 
> ---
>  target-i386/kvm.c | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index aae5e58..2be70df 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
>  int r;
>  
>  /*
> + * If a TSC rate is migrated and the user does not specify the
> + * vcpu's TSC rate on the destination, the migrated TSC rate will
> + * be used on the destination after the migration.
> + */
> +if (env->tsc_khz_saved && !env->tsc_khz) {
> +if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);

Why are you duplicating the existing KVM_SET_TSC_KHZ code in
kvm_arch_init_vcpu()?

> +if (r < 0) {
> +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");

If you want to report errors, please use error_report().

(But I don't think we want to print those warnings. See below.)

> +}
> +} else {
> +r = -1;
> +fprintf(stderr, "KVM doesn't support TSC scaling\n");
> +}
> +if (r < 0) {
> +fprintf(stderr, "Use host TSC frequency instead. "

Did you mean "Using host TSC frequency instead."?

> +"Guest TSC may be inaccurate.\n");
> +}
> +}

This will make QEMU print a warning every single time when migrating to
hosts that don't support TSC scaling, even if the source and destination
hosts already have the same TSC frequency. That means most users will
see a bogus warning, in today's hardware.

Maybe it will be acceptable to print a warning if (and only if) we know
that the host TSC is different from the original TSC frequency.

Considering that we already have code to handle tsc_khz that prints an
error, you don't need to duplicate it. You could handle both
user-provided and migration tsc_khz cases with the same code. With
something like this:

if (env->tsc_khz) { /* may be set by the user, or loaded from incoming 
migration */
r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
-ENOTSUP;
if (r < 0) {
int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
   kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
   0;
/* If we know the host frequency, print a warning every time
 * there's a mismatch.
 * If we don't know the host frequency, print a warning only
 * if the user asked for a specific TSC frequency.
 */
if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
(cur_freq > 0 && cur_freq != env->tsc_khz)) {
error_report("warning: TSC frequency mismatch between VM and 
host, and TSC scaling unavailable");
if (env->tsc_freq_set_by_user) {
return r;
}
}
}
}

You will just need a new tsc_freq_requested_by_user field to track if
the TSC frequency was explicitly requested by the user.

-- 
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 3/3] target-i386: load the migrated vcpu's TSC rate

2015-11-05 Thread Haozhong Zhang
On 11/05/15 14:10, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:26:43PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value if the user does not specify a
> > TSC rate by cpu option 'tsc-freq' and a migrated TSC rate does exist. If
> > KVM supports TSC scaling, guest programs will observe TSC increasing in
> > the migrated rate other than the host TSC rate.
> > 
> > Signed-off-by: Haozhong Zhang 
> > ---
> >  target-i386/kvm.c | 21 +
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index aae5e58..2be70df 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3042,6 +3042,27 @@ int kvm_arch_setup_tsc_khz(CPUState *cs)
> >  int r;
> >  
> >  /*
> > + * If a TSC rate is migrated and the user does not specify the
> > + * vcpu's TSC rate on the destination, the migrated TSC rate will
> > + * be used on the destination after the migration.
> > + */
> > +if (env->tsc_khz_saved && !env->tsc_khz) {
> > +if (kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL)) {
> > +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz_saved);
> 
> Why are you duplicating the existing KVM_SET_TSC_KHZ code in
> kvm_arch_init_vcpu()?
>

Because they are called in different cases and their behaviors on
failure are different:
 1) KVM_SET_TSC_KHZ in kvm_arch_init_vcpu() is called only when a VM
is created and a user-specified TSC frequency is given. If it
fails, QEMU will abort.
 2) KVM_SET_TSC_KHZ in kvm_arch_setup_tsc_khz() is called on the
destination only when TSC frequency is migrated and no
user-specified TSC frequency is given. If it fails, QEMU as well
as the migration will not be aborted.

However, after reading your comment at the end, they really could be
merged.

> > +if (r < 0) {
> > +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> 
> If you want to report errors, please use error_report().
> 
> (But I don't think we want to print those warnings. See below.)
> 
> > +}
> > +} else {
> > +r = -1;
> > +fprintf(stderr, "KVM doesn't support TSC scaling\n");
> > +}
> > +if (r < 0) {
> > +fprintf(stderr, "Use host TSC frequency instead. "
> 
> Did you mean "Using host TSC frequency instead."?
>

Yes.

> > +"Guest TSC may be inaccurate.\n");
> > +}
> > +}
> 
> This will make QEMU print a warning every single time when migrating to
> hosts that don't support TSC scaling, even if the source and destination
> hosts already have the same TSC frequency. That means most users will
> see a bogus warning, in today's hardware.
> 
> Maybe it will be acceptable to print a warning if (and only if) we know
> that the host TSC is different from the original TSC frequency.
>

Agree, I should add such a check to avoid bogus warnings.

> Considering that we already have code to handle tsc_khz that prints an
> error, you don't need to duplicate it. You could handle both
> user-provided and migration tsc_khz cases with the same code. With
> something like this:
>

Mostly, but as tsc_khz_saved in patch 2 is really needed, I'll make
some minor changes.

- if (env->tsc_khz) { /* may be set by the user, or loaded from incoming 
migration */
+ if (env->tsc_khz || env->tsc_khz_saved) { /* may be set by the user, or 
loaded from incoming migration */
+ int64_t tgt_tsc_khz = env->tsc_khz ? : env->tsc_khz_saved;
> r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL) ?
- kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz) :
+ kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, tgt_tsc_khz) :
> -ENOTSUP;
> if (r < 0) {
> int64_t cur_freq = kvm_check_extension(KVM_CAP_GET_TSC_KHZ)) ?
>kvm_vcpu_ioctl(KVM_GET_TSC_KHZ) :
>0;
> /* If we know the host frequency, print a warning every time
>  * there's a mismatch.
>  * If we don't know the host frequency, print a warning only
>  * if the user asked for a specific TSC frequency.
>  */
- if ((cur_freq <= 0 && env->tsc_freq_requested_by_user) ||
+ if ((cur_freq <= 0 && env->tsc_khz) ||
- (cur_freq > 0 && cur_freq != env->tsc_khz)) {
+ (cur_freq > 0 && cur_freq != tgt_tsc_khz)) {
> error_report("warning: TSC frequency mismatch between VM and 
> host, and TSC scaling unavailable");
- if (env->tsc_freq_set_by_user) {
+ if (env->tsc_khz) {
> return r;
> }
> }
> }
> }
>

Haozhong

> You will just need a new tsc_freq_requested_by_user field to track if
> the TSC frequency was explicitly requested by the user.
> 
> -- 
> Eduardo
--
To