Re: [Qemu-devel] [PATCH v4 1/2] target-i386: fallback vcpu's TSC rate to value returned by KVM

[Haozhong Zhang]

On 11/16/15 11:39, Eduardo Habkost wrote:
> On Mon, Nov 16, 2015 at 04:04:07PM +0800, Haozhong Zhang wrote:
> > If no user-specified TSC rate is present, we will try to set
> > env->tsc_khz to the value returned by KVM_GET_TSC_KHZ.
> > 
> > Signed-off-by: Haozhong Zhang <address@hidden>
> > ---
> >  target-i386/kvm.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 2a9953b..9084b29 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2327,6 +2327,27 @@ static int kvm_get_debugregs(X86CPU *cpu)
> >      return 0;
> >  }
> >  
> > +static void kvm_arch_set_tsc_khz(CPUState *cs)
> 
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +    int r;
> > +
> > +    /*
> > +     * If no user-specified TSC frequency is present, we will try to
> > +     * set env->tsc_khz to the value used by KVM.
> > +     */
> > +    if (!env->tsc_khz) {
> > +        r = kvm_check_extension(cs->kvm_state, KVM_CAP_GET_TSC_KHZ) ?
> > +            kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ) : -ENOTSUP;
> 
> Can't we do this on kvm_arch_init_vcpu()? kvm_arch_put_registers()'s purpose 
> is
> to just copy data from QEMU to KVM, not the other way around.
>

I'll move this to kvm_arch_init_vcpu().

> 
> > +        if (r < 0) {
> > +            error_report("warning: KVM_GET_TSC_KHZ failed");
> 
> Having a kernel that doesn't support KVM_CAP_GET_TSC_KHZ shouldn't trigger a
> warning every time we run QEMU, unless the user is explicitly asking for a
> feature that requires KVM_GET_TSC_KHZ.
>

I'll remove the warning.

> > +        } else {
> > +            env->tsc_khz = r;
> > +        }
> > +    }
> > +}
> > +
> >  int kvm_arch_put_registers(CPUState *cpu, int level)
> >  {
> >      X86CPU *x86_cpu = X86_CPU(cpu);
> > @@ -2341,6 +2362,10 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >          }
> >      }
> >  
> > +    if (level == KVM_PUT_FULL_STATE) {
> > +        kvm_arch_set_tsc_khz(cpu);
> > +    }
> 
> I see that kvm_arch_set_tsc_khz() will be extended to call
> KVM_SET_TSC_KHZ in the next patch, so the kvm_arch_set_tsc_khz()
> seems to belong here. But the KVM_GET_TSC_KHZ call doesn't seem
> to belong in kvm_arch_set_tsc_khz()
> 
> kvm_arch_put_registers() callers don't expect any QEMU-side data
> to change, but just that KVM data is updated according to the
> QEMU-side data.
>

Good to know this. As above, I'll move the KVM_GET_TSC_KHZ call to
kvm_arch_init_vcpu().

Haozhong

> > +
> >      ret = kvm_getput_regs(x86_cpu, 1);
> >      if (ret < 0) {
> >          return ret;
> > -- 
> > 2.4.8
> > 
> 
> -- 
> Eduardo