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