Re: [PATCH] vpmu intel: Add cpuid handling when vpmu disabled

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Mar 20, 2013 at 07:40:58AM +0100, Dietmar Hahn wrote:
> Am Mittwoch 13 März 2013, 10:55:29 schrieb Dietmar Hahn:
> > Even though vpmu is disabled in the hypervisor in the HVM guest the call of
> > cpuid(0xa) returns informations about usable performance counters.
> > This may confuse guest software when trying to use the counters and nothing
> > happens.
> > This patch clears most bits in registers eax and edx of cpuid(0xa) instruction
> > 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 <dietmar.hahn@ts.fujitsu.com>
> > 
> > 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 = current_cpu_data.x86;
> >      int ret = 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 = {
> >      .arch_vpmu_load = 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 registers */
> > +#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 == 0xa)
> > +    {
> > +        *eax &= ~(((1 << FEATURE_PMU_VER_BITS) -1) << X86_FEATURE_PMU_VER_OFF);
> > +        *eax &= ~(((1 << FEATURE_NUM_GEN_BITS) -1) << X86_FEATURE_NUM_GEN_OFF);
> > +        *eax &= ~(((1 << FEATURE_GEN_WIDTH_BITS) -1) << X86_FEATURE_GEN_WIDTH_OFF);
> > +
> > +        *edx &= ~(((1 << FEATURE_NUM_FIX_BITS) -1) << X86_FEATURE_NUM_FIX_OFF);
> > +        *edx &= ~(((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_content)
> > +{
> > +    int type = -1, index = -1;
> > +    if ( !is_core2_vpmu_msr(msr, &type, &index) )
> > +        return 0;
> > +    *msr_content = 0;
> > +    return 1;
> > +}
> > +
> > +/*
> > + * These functions are used in case vpmu is not enabled.
> > + */
> > +struct arch_vpmu_ops core2_no_vpmu_ops = {
> > +    .do_rdmsr = core2_no_vpmu_do_rdmsr,
> > +    .do_cpuid = core2_no_vpmu_do_cpuid,
> > +};
> > +
> >  int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
> >  {
> >      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> > @@ -734,6 +787,10 @@ int vmx_vpmu_initialise(struct vcpu *v, 
> >      uint8_t cpu_model = current_cpu_data.x86_model;
> >      int ret = 0;
> >  
> > +    vpmu->arch_vpmu_ops = &core2_no_vpmu_ops;
> > +    if ( !vpmu_flags )
> > +        return 0;
> > +
> >      if ( family == 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 = 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 = 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 = 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 = 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 = 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 = vcpu_vpmu(v);
> >      uint8_t vendor = 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 = 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
>