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@xxxxxxxxx>
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.)
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
