From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Subject: Re: [PATCH V2] Resend - Enabling IA32_TSC_ADJUST for Qemu KVM guest VMs Date: Mon, 26 Nov 2012 20:05:26 +0100 Message-ID: <50B3BD76.8030909@suse.de> References: <1353955371.5921.0.camel@WillAuldHomeLinux> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: "jinsong.liu@intel.com" , "kvm@vger.kernel.org" , Gleb , Marcelo Tosatti , Will Auld , qemu-devel , "donald.d.dugger@intel.com" , "avi@redhat.com" To: will.auld@intel.com Return-path: In-Reply-To: <1353955371.5921.0.camel@WillAuldHomeLinux> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org Hello, Am 26.11.2012 19:42, schrieb Will Auld: > CPUID.7.0.EBX[1]=3D1 indicates IA32_TSC_ADJUST MSR 0x3b is supported >=20 > Basic design is to emulate the MSR by allowing reads and writes to the > hypervisor vcpu specific locations to store the value of the emulated M= SRs. > In this way the IA32_TSC_ADJUST value will be included in all reads to > the TSC MSR whether through rdmsr or rdtsc. >=20 > As this is a new MSR that the guest may access and modify its value nee= ds > to be migrated along with the other MRSs. The changes here are specific= ally > for recognizing when IA32_TSC_ADJUST is enabled in CPUID and code added > for migrating its value. >=20 > Signed-off-by: Will Auld $subject should get a prefix of "target-i386: " and "resend" is better used inside a tag so that it doesn't end up in the commit. And it's "QEMU". ;) Some more stylistic issues inline: > --- > target-i386/cpu.h | 2 ++ > target-i386/kvm.c | 15 +++++++++++++++ > target-i386/machine.c | 21 +++++++++++++++++++++ > 3 files changed, 38 insertions(+) >=20 > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index aabf993..13d4152 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -284,6 +284,7 @@ > #define MSR_IA32_APICBASE_BSP (1<<8) > #define MSR_IA32_APICBASE_ENABLE (1<<11) > #define MSR_IA32_APICBASE_BASE (0xfffff<<12) > +#define MSR_TSC_ADJUST 0x0000003b Tabs. You can use scripts/checkpatch.pl to verify. > #define MSR_IA32_TSCDEADLINE 0x6e0 > =20 > #define MSR_MTRRcap 0xfe > @@ -701,6 +702,7 @@ typedef struct CPUX86State { > uint64_t async_pf_en_msr; > =20 > uint64_t tsc; > + uint64_t tsc_adjust; > uint64_t tsc_deadline; > =20 > uint64_t mcg_status; > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 696b14a..e974c42 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -61,6 +61,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilitie= s[] =3D { > =20 > static bool has_msr_star; > static bool has_msr_hsave_pa; > +static bool has_msr_tsc_adjust; > static bool has_msr_tsc_deadline; > static bool has_msr_async_pf_en; > static bool has_msr_misc_enable; > @@ -641,6 +642,10 @@ static int kvm_get_supported_msrs(KVMState *s) > has_msr_hsave_pa =3D true; > continue; > } > + if (kvm_msr_list->indices[i] =3D=3D MSR_TSC_ADJUST) { > + has_msr_tsc_adjust =3D true; > + continue; > + } > if (kvm_msr_list->indices[i] =3D=3D MSR_IA32_TSCDEADLI= NE) { > has_msr_tsc_deadline =3D true; > continue; > @@ -978,6 +983,10 @@ static int kvm_put_msrs(CPUX86State *env, int leve= l) > if (has_msr_hsave_pa) { > kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave); > } > + if (has_msr_tsc_adjust) { > + kvm_msr_entry_set(&msrs[n++],=20 > + MSR_TSC_ADJUST, env->tsc_adjust); Tabs. > + } > if (has_msr_tsc_deadline) { > kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSCDEADLINE, env->tsc_d= eadline); > } > @@ -1234,6 +1243,9 @@ static int kvm_get_msrs(CPUX86State *env) > if (has_msr_hsave_pa) { > msrs[n++].index =3D MSR_VM_HSAVE_PA; > } > + if (has_msr_tsc_adjust) { > + msrs[n++].index =3D MSR_TSC_ADJUST; > + } > if (has_msr_tsc_deadline) { > msrs[n++].index =3D MSR_IA32_TSCDEADLINE; > } > @@ -1308,6 +1320,9 @@ static int kvm_get_msrs(CPUX86State *env) > case MSR_IA32_TSC: > env->tsc =3D msrs[i].data; > break; > + case MSR_TSC_ADJUST: > + env->tsc_adjust =3D msrs[i].data; > + break; > case MSR_IA32_TSCDEADLINE: > env->tsc_deadline =3D msrs[i].data; > break; > diff --git a/target-i386/machine.c b/target-i386/machine.c > index a8be058..95bda9b 100644 > --- a/target-i386/machine.c > +++ b/target-i386/machine.c > @@ -310,6 +310,24 @@ static const VMStateDescription vmstate_fpop_ip_dp= =3D { > } > }; > =20 > +static bool tsc_adjust_needed(void *opaque) > +{ > + CPUX86State *cpu =3D opaque; Please name this "env" to differentiate from CPUState / X86CPU. Since there are other tsc_* fields already I won't request that you move your new field to the containing X86CPU struct but at some point we will need to convert the VMSDs to X86CPU. > + > + return cpu->tsc_adjust !=3D 0; > +} > + > +static const VMStateDescription vmstate_msr_tsc_adjust =3D { > + .name =3D "cpu/msr_tsc_adjust", > + .version_id =3D 1, > + .minimum_version_id =3D 1, > + .minimum_version_id_old =3D 1, > + .fields =3D (VMStateField []) { > + VMSTATE_UINT64(tsc_adjust, CPUX86State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static bool tscdeadline_needed(void *opaque) > { > CPUX86State *env =3D opaque; > @@ -457,6 +475,9 @@ static const VMStateDescription vmstate_cpu =3D { > .vmsd =3D &vmstate_fpop_ip_dp, > .needed =3D fpop_ip_dp_needed, > }, { > + .vmsd =3D &vmstate_msr_tsc_adjust, > + .needed =3D tsc_adjust_needed, > + }, { > .vmsd =3D &vmstate_msr_tscdeadline, > .needed =3D tscdeadline_needed, > }, { Otherwise looks okay to me. I'm expecting this to go through Marcello's queue unless I'm told otherwise. Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Td3zp-0000pl-Vq for qemu-devel@nongnu.org; Mon, 26 Nov 2012 14:05:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Td3zo-0006Q3-KH for qemu-devel@nongnu.org; Mon, 26 Nov 2012 14:05:33 -0500 Received: from cantor2.suse.de ([195.135.220.15]:36117 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Td3zo-0006Pv-AS for qemu-devel@nongnu.org; Mon, 26 Nov 2012 14:05:32 -0500 Message-ID: <50B3BD76.8030909@suse.de> Date: Mon, 26 Nov 2012 20:05:26 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1353955371.5921.0.camel@WillAuldHomeLinux> In-Reply-To: <1353955371.5921.0.camel@WillAuldHomeLinux> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V2] Resend - Enabling IA32_TSC_ADJUST for Qemu KVM guest VMs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: will.auld@intel.com Cc: "jinsong.liu@intel.com" , "kvm@vger.kernel.org" , Gleb , Marcelo Tosatti , Will Auld , qemu-devel , "donald.d.dugger@intel.com" , "avi@redhat.com" Hello, Am 26.11.2012 19:42, schrieb Will Auld: > CPUID.7.0.EBX[1]=3D1 indicates IA32_TSC_ADJUST MSR 0x3b is supported >=20 > Basic design is to emulate the MSR by allowing reads and writes to the > hypervisor vcpu specific locations to store the value of the emulated M= SRs. > In this way the IA32_TSC_ADJUST value will be included in all reads to > the TSC MSR whether through rdmsr or rdtsc. >=20 > As this is a new MSR that the guest may access and modify its value nee= ds > to be migrated along with the other MRSs. The changes here are specific= ally > for recognizing when IA32_TSC_ADJUST is enabled in CPUID and code added > for migrating its value. >=20 > Signed-off-by: Will Auld $subject should get a prefix of "target-i386: " and "resend" is better used inside a tag so that it doesn't end up in the commit. And it's "QEMU". ;) Some more stylistic issues inline: > --- > target-i386/cpu.h | 2 ++ > target-i386/kvm.c | 15 +++++++++++++++ > target-i386/machine.c | 21 +++++++++++++++++++++ > 3 files changed, 38 insertions(+) >=20 > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index aabf993..13d4152 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -284,6 +284,7 @@ > #define MSR_IA32_APICBASE_BSP (1<<8) > #define MSR_IA32_APICBASE_ENABLE (1<<11) > #define MSR_IA32_APICBASE_BASE (0xfffff<<12) > +#define MSR_TSC_ADJUST 0x0000003b Tabs. You can use scripts/checkpatch.pl to verify. > #define MSR_IA32_TSCDEADLINE 0x6e0 > =20 > #define MSR_MTRRcap 0xfe > @@ -701,6 +702,7 @@ typedef struct CPUX86State { > uint64_t async_pf_en_msr; > =20 > uint64_t tsc; > + uint64_t tsc_adjust; > uint64_t tsc_deadline; > =20 > uint64_t mcg_status; > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 696b14a..e974c42 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -61,6 +61,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilitie= s[] =3D { > =20 > static bool has_msr_star; > static bool has_msr_hsave_pa; > +static bool has_msr_tsc_adjust; > static bool has_msr_tsc_deadline; > static bool has_msr_async_pf_en; > static bool has_msr_misc_enable; > @@ -641,6 +642,10 @@ static int kvm_get_supported_msrs(KVMState *s) > has_msr_hsave_pa =3D true; > continue; > } > + if (kvm_msr_list->indices[i] =3D=3D MSR_TSC_ADJUST) { > + has_msr_tsc_adjust =3D true; > + continue; > + } > if (kvm_msr_list->indices[i] =3D=3D MSR_IA32_TSCDEADLI= NE) { > has_msr_tsc_deadline =3D true; > continue; > @@ -978,6 +983,10 @@ static int kvm_put_msrs(CPUX86State *env, int leve= l) > if (has_msr_hsave_pa) { > kvm_msr_entry_set(&msrs[n++], MSR_VM_HSAVE_PA, env->vm_hsave); > } > + if (has_msr_tsc_adjust) { > + kvm_msr_entry_set(&msrs[n++],=20 > + MSR_TSC_ADJUST, env->tsc_adjust); Tabs. > + } > if (has_msr_tsc_deadline) { > kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSCDEADLINE, env->tsc_d= eadline); > } > @@ -1234,6 +1243,9 @@ static int kvm_get_msrs(CPUX86State *env) > if (has_msr_hsave_pa) { > msrs[n++].index =3D MSR_VM_HSAVE_PA; > } > + if (has_msr_tsc_adjust) { > + msrs[n++].index =3D MSR_TSC_ADJUST; > + } > if (has_msr_tsc_deadline) { > msrs[n++].index =3D MSR_IA32_TSCDEADLINE; > } > @@ -1308,6 +1320,9 @@ static int kvm_get_msrs(CPUX86State *env) > case MSR_IA32_TSC: > env->tsc =3D msrs[i].data; > break; > + case MSR_TSC_ADJUST: > + env->tsc_adjust =3D msrs[i].data; > + break; > case MSR_IA32_TSCDEADLINE: > env->tsc_deadline =3D msrs[i].data; > break; > diff --git a/target-i386/machine.c b/target-i386/machine.c > index a8be058..95bda9b 100644 > --- a/target-i386/machine.c > +++ b/target-i386/machine.c > @@ -310,6 +310,24 @@ static const VMStateDescription vmstate_fpop_ip_dp= =3D { > } > }; > =20 > +static bool tsc_adjust_needed(void *opaque) > +{ > + CPUX86State *cpu =3D opaque; Please name this "env" to differentiate from CPUState / X86CPU. Since there are other tsc_* fields already I won't request that you move your new field to the containing X86CPU struct but at some point we will need to convert the VMSDs to X86CPU. > + > + return cpu->tsc_adjust !=3D 0; > +} > + > +static const VMStateDescription vmstate_msr_tsc_adjust =3D { > + .name =3D "cpu/msr_tsc_adjust", > + .version_id =3D 1, > + .minimum_version_id =3D 1, > + .minimum_version_id_old =3D 1, > + .fields =3D (VMStateField []) { > + VMSTATE_UINT64(tsc_adjust, CPUX86State), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static bool tscdeadline_needed(void *opaque) > { > CPUX86State *env =3D opaque; > @@ -457,6 +475,9 @@ static const VMStateDescription vmstate_cpu =3D { > .vmsd =3D &vmstate_fpop_ip_dp, > .needed =3D fpop_ip_dp_needed, > }, { > + .vmsd =3D &vmstate_msr_tsc_adjust, > + .needed =3D tsc_adjust_needed, > + }, { > .vmsd =3D &vmstate_msr_tscdeadline, > .needed =3D tscdeadline_needed, > }, { Otherwise looks okay to me. I'm expecting this to go through Marcello's queue unless I'm told otherwise. Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg