From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Liu, Jinsong" Subject: RE: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest Date: Fri, 23 Mar 2012 03:49:27 +0000 Message-ID: References: <4F0A099C.5040805@web.de> <4F4BBAEC.2040603@siemens.com> <4F55E39F.6020608@siemens.com> <4F67B4C9.3010502@redhat.com> <20120320133314.GV9375@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: Rik van Riel , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , Jan Kiszka , Avi Kivity To: Eduardo Habkost Return-path: Received: from mga14.intel.com ([143.182.124.37]:10581 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752922Ab2CWDtb convert rfc822-to-8bit (ORCPT ); Thu, 22 Mar 2012 23:49:31 -0400 In-Reply-To: <20120320133314.GV9375@otherpad.lan.raisama.net> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: Eduardo Habkost wrote: > On Tue, Mar 20, 2012 at 12:53:57PM +0000, Liu, Jinsong wrote: >> Rik van Riel wrote: >>> On 03/09/2012 01:27 PM, Liu, Jinsong wrote: >>> >>>> As for 'tsc deadline' feature exposing, my patch (as attached) just >>>> obey qemu general cpuid exposing method, and also satisfied your >>>> target I think. >>> >>> One question. >>> >>> Why is TSC_DEADLINE not exposed in the cpuid allowed feature >>> bits in do_cpuid_ent() in arch/x86/kvm/x86.c ? >>> >>> /* cpuid 1.ecx */ >>> const u32 kvm_supported_word4_x86_features = >>> F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | >>> 0 /* DS-CPL, VMX, SMX, EST */ | >>> 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* >>> Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update, >>> PDCM */ | 0 /* Reserved, DCA */ | F(XMM4_1) | >>> F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) | >>> 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE >>> */ | F(AVX) | F(F16C) | F(RDRAND); >>> >>> Would it make sense to expose F(TSC_DEADLINE) above? >>> >>> Or is there something truly special about tsc deadline >>> that means it should be different from everything else? >> >> Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot >> guarantee will be called, we expose it via a >> KVM_CAP_TSC_DEADLINE_TIMER and not KVM_GET_SUPPORTED_CPUID. > > We have many other features that depend on proper support from > userspace otherwise they wouldn't work, but are listed on > GET_SUPPORTED_CPUID, don't we? Why is TSC-deadline special? > > GET_SUPPORTED_CPUID just means "KVM supports it as long as userspace > supports it too and enables it", it doesn't mean "CPUID bit that will > be enabled by default"[1]. > >> Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e. > > That changeset was necessary because the kernel was doing this on > update_cpu > > if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && > best->function == 0x1) { > best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER); > > And that was really wrong, because it enabled the bit unconditionally. > But I don't understand why KVM_CAP_TSC_DEADLINE_TIMER was created if > we already have KVM_GET_SUPPORTED_CPUID to tell userspace which bits > are supported by KVM. > Yes, exactly. That's why we need this patch. > > [1] From Documentation/virtual/kvm/api.txt: > > "KVM_GET_SUPPORTED_CPUID > [...] > This ioctl returns x86 cpuid features which are supported by both the > hardware and kvm. Userspace can use the information returned by this > ioctl to construct cpuid information (for KVM_SET_CPUID2) that is > consistent with hardware, kernel, and userspace capabilities, and with > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > user requirements (for example, the user may wish to constrain cpuid > to emulate older hardware, or for feature consistency across a > cluster)." The fixbug patch is implemented by Jan and Avi, I reply per my understanding. I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is slightly better than KVM_GET_SUPPORTED_CPUID. If use KVM_GET_SUPPORTED_CPUID, it means tsc deadline features bind to host cpuid, while it fact it could be pure software emulated by kvm (though currently it implemented as bound to hareware). For the sake of extension, it choose KVM_CAP_TSC_DEADLINE_TIMER. Thanks, Jinsong From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAvVS-0007YB-MG for qemu-devel@nongnu.org; Thu, 22 Mar 2012 23:49:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SAvVQ-0000xl-OH for qemu-devel@nongnu.org; Thu, 22 Mar 2012 23:49:38 -0400 Received: from mga14.intel.com ([143.182.124.37]:27854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAvVQ-0000xI-DE for qemu-devel@nongnu.org; Thu, 22 Mar 2012 23:49:36 -0400 From: "Liu, Jinsong" Date: Fri, 23 Mar 2012 03:49:27 +0000 Message-ID: References: <4F0A099C.5040805@web.de> <4F4BBAEC.2040603@siemens.com> <4F55E39F.6020608@siemens.com> <4F67B4C9.3010502@redhat.com> <20120320133314.GV9375@otherpad.lan.raisama.net> In-Reply-To: <20120320133314.GV9375@otherpad.lan.raisama.net> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Jan Kiszka , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , Avi Kivity Eduardo Habkost wrote: > On Tue, Mar 20, 2012 at 12:53:57PM +0000, Liu, Jinsong wrote: >> Rik van Riel wrote: >>> On 03/09/2012 01:27 PM, Liu, Jinsong wrote: >>>=20 >>>> As for 'tsc deadline' feature exposing, my patch (as attached) just >>>> obey qemu general cpuid exposing method, and also satisfied your >>>> target I think. >>>=20 >>> One question. >>>=20 >>> Why is TSC_DEADLINE not exposed in the cpuid allowed feature >>> bits in do_cpuid_ent() in arch/x86/kvm/x86.c ? >>>=20 >>> /* cpuid 1.ecx */ >>> const u32 kvm_supported_word4_x86_features =3D >>> F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | >>> 0 /* DS-CPL, VMX, SMX, EST */ | >>> 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* >>> Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update, >>> PDCM */ | 0 /* Reserved, DCA */ | F(XMM4_1) | >>> F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) | >>> 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE >>> */ | F(AVX) | F(F16C) | F(RDRAND); >>>=20 >>> Would it make sense to expose F(TSC_DEADLINE) above? >>>=20 >>> Or is there something truly special about tsc deadline >>> that means it should be different from everything else? >>=20 >> Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot >> guarantee will be called, we expose it via a >> KVM_CAP_TSC_DEADLINE_TIMER and not KVM_GET_SUPPORTED_CPUID. =20 >=20 > We have many other features that depend on proper support from > userspace otherwise they wouldn't work, but are listed on > GET_SUPPORTED_CPUID, don't we? Why is TSC-deadline special? >=20 > GET_SUPPORTED_CPUID just means "KVM supports it as long as userspace > supports it too and enables it", it doesn't mean "CPUID bit that will > be enabled by default"[1]. >=20 >> Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e. >=20 > That changeset was necessary because the kernel was doing this on > update_cpu >=20 > if (boot_cpu_data.x86_vendor =3D=3D X86_VENDOR_INTEL && > best->function =3D=3D 0x1) { > best->ecx |=3D bit(X86_FEATURE_TSC_DEADLINE_TIMER); >=20 > And that was really wrong, because it enabled the bit unconditionally. > But I don't understand why KVM_CAP_TSC_DEADLINE_TIMER was created if > we already have KVM_GET_SUPPORTED_CPUID to tell userspace which bits > are supported by KVM. >=20 Yes, exactly. That's why we need this patch. >=20 > [1] From Documentation/virtual/kvm/api.txt: >=20 > "KVM_GET_SUPPORTED_CPUID > [...] > This ioctl returns x86 cpuid features which are supported by both the > hardware and kvm. Userspace can use the information returned by this > ioctl to construct cpuid information (for KVM_SET_CPUID2) that is > consistent with hardware, kernel, and userspace capabilities, and with > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > user requirements (for example, the user may wish to constrain cpuid > to emulate older hardware, or for feature consistency across a > cluster)."=20 The fixbug patch is implemented by Jan and Avi, I reply per my understandin= g. I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is sligh= tly better than KVM_GET_SUPPORTED_CPUID. If use KVM_GET_SUPPORTED_CPUID, it= means tsc deadline features bind to host cpuid, while it fact it could be = pure software emulated by kvm (though currently it implemented as bound to = hareware). For the sake of extension, it choose KVM_CAP_TSC_DEADLINE_TIMER. Thanks, Jinsong