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