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

[Eduardo Habkost]

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.


> +        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.

> +        } 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.

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

-- 
Eduardo