From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled Date: Mon, 25 Mar 2013 10:59:22 -0400 Message-ID: <20130325145922.GB15129@phenom.dumpdata.com> References: <10932493.oBcqbPszgs@amur> <1486142.9kgXaF8ZbC@amur> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1486142.9kgXaF8ZbC@amur> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dietmar Hahn Cc: Eddie Dong , Jun Nakajima , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, Mar 20, 2013 at 07:40:58AM +0100, Dietmar Hahn wrote: > Am Mittwoch 13 M=E4rz 2013, 10:55:29 schrieb Dietmar Hahn: > > Even though vpmu is disabled in the hypervisor in the HVM guest the cal= l of > > cpuid(0xa) returns informations about usable performance counters. > > This may confuse guest software when trying to use the counters and not= hing > > happens. > > This patch clears most bits in registers eax and edx of cpuid(0xa) inst= ruction > > for the guest when vpmu is disabled: > > - version ID of architectural performance counting > > - number of general pmu registers > > - width of general pmu registers > > - number of fixed pmu registers > > - width of ixed pmu registers > > = > > Thanks. > > = > > Dietmar. > = > Ping? You need to CC the AMD maintainer as well. CC-ing him here. > = > > = > > Signed-off-by: Dietmar Hahn > > = > > diff -r a6b81234b189 xen/arch/x86/hvm/svm/vpmu.c > > --- a/xen/arch/x86/hvm/svm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/svm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -370,6 +370,10 @@ int svm_vpmu_initialise(struct vcpu *v, = > > uint8_t family =3D current_cpu_data.x86; > > int ret =3D 0; > > = > > + /* vpmu enabled? */ > > + if (!vpmu_flags) > > + return 0; > > + > > switch ( family ) > > { > > case 0x10: > > diff -r a6b81234b189 xen/arch/x86/hvm/vmx/vpmu_core2.c > > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -727,6 +727,59 @@ struct arch_vpmu_ops core2_vpmu_ops =3D { > > .arch_vpmu_load =3D core2_vpmu_load > > }; > > = > > +/* cpuid 0xa - eax */ Do you think it might make sense to point out where these are defined in the Intel SDM? > > +#define X86_FEATURE_PMU_VER_OFF 0 /* Version */ > > +#define FEATURE_PMU_VER_BITS 8 /* 8 bits */ > > +#define X86_FEATURE_NUM_GEN_OFF 8 /* Number of general pmu register= s */ > > +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ > > +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registers= */ > > +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ > > + > > +/* cpuid 0xa - edx */ > > +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu registers = */ > > +#define FEATURE_NUM_FIX_BITS 5 /* 5 bits */ > > +#define X86_FEATURE_FIX_WIDTH_OFF 5 /* Width of fixed pmu registers */ > > +#define FEATURE_FIX_WIDTH_BITS 8 /* 8 bits */ > > + > > +static void core2_no_vpmu_do_cpuid(unsigned int input, > > + unsigned int *eax, unsigned int *ebx, > > + unsigned int *ecx, unsigned int *edx) > > +{ > > + /* > > + * As in this case the vpmu is not enabled reset some bits in the > > + * pmu part of the cpuid. > > + */ > > + if (input =3D=3D 0xa) > > + { > > + *eax &=3D ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PM= U_VER_OFF); > > + *eax &=3D ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NU= M_GEN_OFF); > > + *eax &=3D ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_= GEN_WIDTH_OFF); > > + > > + *edx &=3D ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NU= M_FIX_OFF); > > + *edx &=3D ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATURE_= FIX_WIDTH_OFF); > > + } > > +} > > + > > +/* > > + * If it's a vpmu msr set it to 0. .. s/it/its value/ > > + */ > > +static int core2_no_vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_cont= ent) > > +{ > > + int type =3D -1, index =3D -1; > > + if ( !is_core2_vpmu_msr(msr, &type, &index) ) > > + return 0; > > + *msr_content =3D 0; > > + return 1; > > +} > > + > > +/* > > + * These functions are used in case vpmu is not enabled. > > + */ > > +struct arch_vpmu_ops core2_no_vpmu_ops =3D { > > + .do_rdmsr =3D core2_no_vpmu_do_rdmsr, > > + .do_cpuid =3D core2_no_vpmu_do_cpuid, > > +}; > > + > > int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags) > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, = > > uint8_t cpu_model =3D current_cpu_data.x86_model; > > int ret =3D 0; > > = > > + vpmu->arch_vpmu_ops =3D &core2_no_vpmu_ops; > > + if ( !vpmu_flags ) > > + return 0; > > + > > if ( family =3D=3D 6 ) > > { > > switch ( cpu_model ) > > diff -r a6b81234b189 xen/arch/x86/hvm/vpmu.c > > --- a/xen/arch/x86/hvm/vpmu.c Mon Mar 11 16:13:42 2013 +0000 > > +++ b/xen/arch/x86/hvm/vpmu.c Wed Mar 13 09:54:00 2013 +0100 > > @@ -67,7 +67,7 @@ int vpmu_do_wrmsr(unsigned int msr, uint > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(current); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) > > return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); > > return 0; > > } > > @@ -76,7 +76,7 @@ int vpmu_do_rdmsr(unsigned int msr, uint > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(current); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_rdmsr ) > > return vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content); > > return 0; > > } > > @@ -85,7 +85,7 @@ int vpmu_do_interrupt(struct cpu_user_re > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(current); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_interrupt ) > > return vpmu->arch_vpmu_ops->do_interrupt(regs); > > return 0; > > } > > @@ -104,7 +104,7 @@ void vpmu_save(struct vcpu *v) > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_save ) > > vpmu->arch_vpmu_ops->arch_vpmu_save(v); > > } > > = > > @@ -112,7 +112,7 @@ void vpmu_load(struct vcpu *v) > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load ) > > vpmu->arch_vpmu_ops->arch_vpmu_load(v); > > } > > = > > @@ -121,9 +121,6 @@ void vpmu_initialise(struct vcpu *v) > > struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > uint8_t vendor =3D current_cpu_data.x86_vendor; > > = > > - if ( !opt_vpmu_enabled ) > > - return; > > - > > if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > > vpmu_destroy(v); > > vpmu_clear(vpmu); > > @@ -153,7 +150,7 @@ void vpmu_destroy(struct vcpu *v) > > { > > struct vpmu_struct *vpmu =3D vcpu_vpmu(v); > > = > > - if ( vpmu->arch_vpmu_ops ) > > + if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy= ) > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > > } > > = > > = > > = > -- = > Company details: http://ts.fujitsu.com/imprint.html > = > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > =