From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [PATCH v4 1/2] target-i386: fallback vcpu's TSC rate to value returned by KVM Date: Mon, 16 Nov 2015 11:39:06 -0200 Message-ID: <20151116133906.GV4180@thinpad.lan.raisama.net> References: <1447661048-31048-1-git-send-email-haozhong.zhang@intel.com> <1447661048-31048-2-git-send-email-haozhong.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini , Richard Henderson , "Michael S. Tsirkin" , afaerber@suse.de, Marcelo Tosatti , kvm@vger.kernel.org To: Haozhong Zhang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37051 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752643AbbKPNjJ (ORCPT ); Mon, 16 Nov 2015 08:39:09 -0500 Content-Disposition: inline In-Reply-To: <1447661048-31048-2-git-send-email-haozhong.zhang@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyK02-00011I-PE for qemu-devel@nongnu.org; Mon, 16 Nov 2015 08:39:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyJzx-0003Yo-PV for qemu-devel@nongnu.org; Mon, 16 Nov 2015 08:39:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyJzx-0003Yk-K3 for qemu-devel@nongnu.org; Mon, 16 Nov 2015 08:39:09 -0500 Date: Mon, 16 Nov 2015 11:39:06 -0200 From: Eduardo Habkost Message-ID: <20151116133906.GV4180@thinpad.lan.raisama.net> References: <1447661048-31048-1-git-send-email-haozhong.zhang@intel.com> <1447661048-31048-2-git-send-email-haozhong.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447661048-31048-2-git-send-email-haozhong.zhang@intel.com> Subject: Re: [Qemu-devel] [PATCH v4 1/2] target-i386: fallback vcpu's TSC rate to value returned by KVM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Haozhong Zhang Cc: kvm@vger.kernel.org, "Michael S. Tsirkin" , Marcelo Tosatti , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini , afaerber@suse.de, Richard Henderson 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 > --- > 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