From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled Date: Mon, 25 Mar 2013 14:41:35 -0500 Message-ID: <5150A86F.2030808@amd.com> References: <10932493.oBcqbPszgs@amur> <1486142.9kgXaF8ZbC@amur> <20130325145922.GB15129@phenom.dumpdata.com> <20130325145954.GC15129@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20130325145954.GC15129@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: Konrad Rzeszutek Wilk Cc: Eddie Dong , jacob.shin@amd.com, Jun Nakajima , Dietmar Hahn , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 3/25/2013 9:59 AM, Konrad Rzeszutek Wilk wrote: > 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 ca= ll of >>>> cpuid(0xa) returns informations about usable performance counters. >>>> This may confuse guest software when trying to use the counters and no= thing >>>> happens. >>>> This patch clears most bits in registers eax and edx of cpuid(0xa) ins= truction >>>> 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 This looks fine. AMD code doesn't currently use the vpmu_flags in the = AMD-specific code. Suravee >>>> 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 registe= rs */ >>>> +#define FEATURE_NUM_GEN_BITS 8 /* 8 bits */ >>>> +#define X86_FEATURE_GEN_WIDTH_OFF 16 /* Width of general pmu register= s */ >>>> +#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_P= MU_VER_OFF); >>>> + *eax &=3D ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_N= UM_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_N= UM_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_con= tent) >>>> +{ >>>> + 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_destro= y ) >>>> 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 >>>