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:54 -0400 Message-ID: <20130325145954.GC15129@phenom.dumpdata.com> References: <10932493.oBcqbPszgs@amur> <1486142.9kgXaF8ZbC@amur> <20130325145922.GB15129@phenom.dumpdata.com> 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: <20130325145922.GB15129@phenom.dumpdata.com> 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 , suravee.suthikulpanit@amd.com, jacob.shin@amd.com Cc: Eddie Dong , Jun Nakajima , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Mon, Mar 25, 2013 at 10:59:22AM -0400, Konrad Rzeszutek Wilk wrote: > 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 c= all of > > > cpuid(0xa) returns informations about usable performance counters. > > > This may confuse guest software when trying to use the counters and n= othing > > > happens. > > > This patch clears most bits in registers eax and edx of cpuid(0xa) in= struction > > > 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. This time doing it > > = > > > = > > > 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 regist= ers */ > > > +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ > > > +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu registe= rs */ > > > +#define FEATURE_GEN_WIDTH_BITS 8 /* 8 bits */ > > > + > > > +/* cpuid 0xa - edx */ > > > +#define X86_FEATURE_NUM_FIX_OFF 0 /* Number of fixed pmu register= s */ > > > +#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_= PMU_VER_OFF); > > > + *eax &=3D ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_= NUM_GEN_OFF); > > > + *eax &=3D ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATUR= E_GEN_WIDTH_OFF); > > > + > > > + *edx &=3D ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_= NUM_FIX_OFF); > > > + *edx &=3D ~(((1 << FEATURE_FIX_WIDTH_BITS) -1) << X86_FEATUR= E_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_co= ntent) > > > +{ > > > + 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_destr= oy ) > > > 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 > > =