| Message ID | 1447737639-6139-3-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote: > Following two changes are made to the TSC rate setting code in > kvm_arch_init_vcpu(): > * The code is moved to a new function kvm_arch_set_tsc_khz(). > * If setting user-specified TSC rate fails and the host TSC rate is > inconsistent with the user-specified one, print a warning message. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> This matches what I was expecting, and now I see that we don't even need the user_tsc_khz field. > --- > target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 38 insertions(+), 7 deletions(-) > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 9e4d27f..6a1acb4 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu) > cpu->hyperv_runtime); > } > > +/** > + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz. > + * > + * Returns: 0 if successful; > + * -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable; > + * -EIO if KVM_SET_TSC_KHZ fails. If KVM_SET_TSC_KHZ fails, the error code will be useful to understand what went wrong. It's better to return the error code returned by KVM instead of -EIO. > + */ > +static int kvm_arch_set_tsc_khz(CPUState *cs) > +{ > + X86CPU *cpu = X86_CPU(cs); > + CPUX86State *env = &cpu->env; > + int has_tsc_control, r = 0; > + > + if (!env->tsc_khz) { > + return 0; > + } > + > + has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); > + if (has_tsc_control) { > + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); > + } > + > + if (!has_tsc_control || r < 0) { > + r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ? > + kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP; > + if (r <= 0 || r != env->tsc_khz) { > + error_report("warning: TSC frequency mismatch between " > + "VM and host, and TSC scaling unavailable"); > + return has_tsc_control ? -EIO : -ENOTSUP; > + } > + } What about: 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) { /* If KVM_SET_TSC_KHZ fails, it is an error only if the * current TSC frequency doesn't match the one we want. */ int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ? kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP; if (cur_freq <= 0 || cur_freq != env->tsc_khz) { error_report("warning: TSC frequency mismatch between " "VM and host, and TSC scaling unavailable"); return r; } } return 0; > + > + return 0; > +} > + > static Error *invtsc_mig_blocker; > > #define KVM_MAX_CPUID_ENTRIES 100 > @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs) > return r; > } > > - r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); > - if (r && env->tsc_khz) { > - r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); > - if (r < 0) { > - fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); > - return r; > - } > + if (kvm_arch_set_tsc_khz(cs) == -EIO) { > + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); Now kvm_arch_set_tsc_khz() prints an error message, we can remove this one. > + return -EIO; To keep the previous behavior without losing the error code returned by KVM, this could be written as: r = kvm_arch_set_tsc_khz(cs); if (r < 0 && r != -ENOTSUP) { return r; } But I belive the previous behavior was wrong. If tsc-freq was explicitly set by the user, we should abort if KVM_CAP_TSC_CONTROL is unavailable. So I suggest we simply do this: r = kvm_arch_set_tsc_khz(cs); if (r < 0) { return r; } (In case you implement this behavior change in the same patch, please mention that in the commit message.)
On 11/17/15 11:32, Eduardo Habkost wrote: > On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote: > > Following two changes are made to the TSC rate setting code in > > kvm_arch_init_vcpu(): > > * The code is moved to a new function kvm_arch_set_tsc_khz(). > > * If setting user-specified TSC rate fails and the host TSC rate is > > inconsistent with the user-specified one, print a warning message. > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > This matches what I was expecting, and now I see that we don't > even need the user_tsc_khz field. > I guess you mean the user_tsc_khz field is not needed when setting TSC rate. It's still needed in patch 3 to check if the migrated TSC rate is consistent with the user-specified TSC rate (and of course it's not in kvm_arch_set_tsc_khz()). > > --- > > target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 38 insertions(+), 7 deletions(-) > > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > index 9e4d27f..6a1acb4 100644 > > --- a/target-i386/kvm.c > > +++ b/target-i386/kvm.c > > @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu) > > cpu->hyperv_runtime); > > } > > > > +/** > > + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz. > > + * > > + * Returns: 0 if successful; > > + * -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable; > > + * -EIO if KVM_SET_TSC_KHZ fails. > > If KVM_SET_TSC_KHZ fails, the error code will be useful to > understand what went wrong. It's better to return the error code > returned by KVM instead of -EIO. > Yes, I'll change in the next version. > > + */ > > +static int kvm_arch_set_tsc_khz(CPUState *cs) > > +{ > > + X86CPU *cpu = X86_CPU(cs); > > + CPUX86State *env = &cpu->env; > > + int has_tsc_control, r = 0; > > + > > + if (!env->tsc_khz) { > > + return 0; > > + } > > + > > + has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); > > + if (has_tsc_control) { > > + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); > > + } > > + > > + if (!has_tsc_control || r < 0) { > > + r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ? > > + kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP; > > + if (r <= 0 || r != env->tsc_khz) { > > + error_report("warning: TSC frequency mismatch between " > > + "VM and host, and TSC scaling unavailable"); > > + return has_tsc_control ? -EIO : -ENOTSUP; > > + } > > + } > > What about: > > 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) { > /* If KVM_SET_TSC_KHZ fails, it is an error only if the > * current TSC frequency doesn't match the one we want. > */ > int cur_freq = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ? > kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : > -ENOTSUP; > if (cur_freq <= 0 || cur_freq != env->tsc_khz) { > error_report("warning: TSC frequency mismatch between " > "VM and host, and TSC scaling unavailable"); > return r; > } > } > > return 0; > Yes, your suggestion is better. > > + > > + return 0; > > +} > > + > > static Error *invtsc_mig_blocker; > > > > #define KVM_MAX_CPUID_ENTRIES 100 > > @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs) > > return r; > > } > > > > - r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); > > - if (r && env->tsc_khz) { > > - r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); > > - if (r < 0) { > > - fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); > > - return r; > > - } > > + if (kvm_arch_set_tsc_khz(cs) == -EIO) { > > + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); > > Now kvm_arch_set_tsc_khz() prints an error message, we can remove > this one. will remove in the next version. > > > + return -EIO; > > To keep the previous behavior without losing the error code > returned by KVM, this could be written as: > > r = kvm_arch_set_tsc_khz(cs); > if (r < 0 && r != -ENOTSUP) { > return r; > } > > But I belive the previous behavior was wrong. If tsc-freq was > explicitly set by the user, we should abort if > KVM_CAP_TSC_CONTROL is unavailable. > > So I suggest we simply do this: > > r = kvm_arch_set_tsc_khz(cs); > if (r < 0) { > return r; > } > Yes, I also prefer this one. I'll change and update the commit message in the next version. Thanks, Haozhong > (In case you implement this behavior change in the same patch, > please mention that in the commit message.) > > -- > Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 17, 2015 at 10:07:53PM +0800, Haozhong Zhang wrote: > On 11/17/15 11:32, Eduardo Habkost wrote: > > On Tue, Nov 17, 2015 at 01:20:38PM +0800, Haozhong Zhang wrote: > > > Following two changes are made to the TSC rate setting code in > > > kvm_arch_init_vcpu(): > > > * The code is moved to a new function kvm_arch_set_tsc_khz(). > > > * If setting user-specified TSC rate fails and the host TSC rate is > > > inconsistent with the user-specified one, print a warning message. > > > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > > > > This matches what I was expecting, and now I see that we don't > > even need the user_tsc_khz field. > > > > I guess you mean the user_tsc_khz field is not needed when setting TSC > rate. It's still needed in patch 3 to check if the migrated TSC rate > is consistent with the user-specified TSC rate (and of course it's not > in kvm_arch_set_tsc_khz()). Yes, I was looking only at the error-checking logic in kvm_arch_init_vcpu(). Then I noticed user_tsc_khz was added for the migration sanity check (which makes sense).
diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 9e4d27f..6a1acb4 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -524,6 +524,41 @@ static bool hyperv_enabled(X86CPU *cpu) cpu->hyperv_runtime); } +/** + * Ask KVM to set vcpu's TSC rate to X86_CPU(cs)->env.tsc_khz. + * + * Returns: 0 if successful; + * -ENOTSUP if KVM_CAP_TSC_CONTROL is unavailable; + * -EIO if KVM_SET_TSC_KHZ fails. + */ +static int kvm_arch_set_tsc_khz(CPUState *cs) +{ + X86CPU *cpu = X86_CPU(cs); + CPUX86State *env = &cpu->env; + int has_tsc_control, r = 0; + + if (!env->tsc_khz) { + return 0; + } + + has_tsc_control = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); + if (has_tsc_control) { + r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); + } + + if (!has_tsc_control || r < 0) { + r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ? + kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP; + if (r <= 0 || r != env->tsc_khz) { + error_report("warning: TSC frequency mismatch between " + "VM and host, and TSC scaling unavailable"); + return has_tsc_control ? -EIO : -ENOTSUP; + } + } + + return 0; +} + static Error *invtsc_mig_blocker; #define KVM_MAX_CPUID_ENTRIES 100 @@ -823,13 +858,9 @@ int kvm_arch_init_vcpu(CPUState *cs) return r; } - r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL); - if (r && env->tsc_khz) { - r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz); - if (r < 0) { - fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); - return r; - } + if (kvm_arch_set_tsc_khz(cs) == -EIO) { + fprintf(stderr, "KVM_SET_TSC_KHZ failed\n"); + return -EIO; } /*
Following two changes are made to the TSC rate setting code in kvm_arch_init_vcpu(): * The code is moved to a new function kvm_arch_set_tsc_khz(). * If setting user-specified TSC rate fails and the host TSC rate is inconsistent with the user-specified one, print a warning message. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- target-i386/kvm.c | 45 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-)