Re: [PATCH 2 of 2] vpmu: Add a vpmu cpuid function

[Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>]

Am Freitag 20 Januar 2012, 10:29:41 schrieb Keir Fraser:
> On 19/01/2012 13:54, "Dietmar Hahn" <dietmar.hahn@ts.fujitsu.com> wrote:
> 
> > 
> >  tools/libxc/xc_cpuid_x86.c        |   1 +
> >  xen/arch/x86/hvm/vmx/vmx.c        |   2 ++
> >  xen/arch/x86/hvm/vmx/vpmu_core2.c |  38
> > ++++++++++++++++++++++++++++++++++++--
> >  xen/arch/x86/hvm/vpmu.c           |  25 ++++++++++++++++++-------
> >  xen/include/asm-x86/hvm/vpmu.h    |   6 ++++++
> >  5 files changed, 63 insertions(+), 9 deletions(-)
> > 
> > 
> > The  "Debug Store" cpuid flag in the Intel processors gets enabled in the
> > libxc.
> > A new function call arch_vpmu_cpuid is added to the struct arch_vpmu_ops and
> > for
> > Intel processors the function core2_vpmu_cpuid() is added.
> > The aim is that always a "struct arch_vpmu_ops" is accessible at least with
> > a cpuid function to switch off special PMU cpuid flags dynamically depending
> > on
> > the boot variable opt_vpmu_enabled.
> 
> Our CPUID configuration is done per-domain, and from
> tools/libxc/xc_cpuid_x86.c. CPUID adjustments implemented within the
> hypervisor are generally not acceptable without very good reason.

Then a way is needed to have access to the opt_vpmu_enabled variable within the
hypervisor from the tools to decide the enabling of the flag (is there such a
way?) or the mechanism with the boot variable must be changed.
The opt_vpmu_enabled boot variable was introduced because of a PMU problem in
the Nehalem cpus leading sometimes to hypervisor crashes. But with the done
quirk we never had a crash anymore.
So maybe we can always switch on the vpmu stuff in the hypervisor and add a
flag in the domain configuration when somebody wants to do some performance
tests?

Dietmar.

> 
>  -- Keir
> 
> > Signed-off-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> > 
> > diff -r 7a82c2e2eb33 tools/libxc/xc_cpuid_x86.c
> > --- a/tools/libxc/xc_cpuid_x86.c Thu Jan 19 13:14:02 2012 +0100
> > +++ b/tools/libxc/xc_cpuid_x86.c Thu Jan 19 14:37:17 2012 +0100
> > @@ -343,6 +343,7 @@ static void xc_cpuid_hvm_policy(
> >                      bitmaskof(X86_FEATURE_CMOV) |
> >                      bitmaskof(X86_FEATURE_PAT) |
> >                      bitmaskof(X86_FEATURE_CLFLSH) |
> > +                    bitmaskof(X86_FEATURE_DS) |
> >                      bitmaskof(X86_FEATURE_PSE36) |
> >                      bitmaskof(X86_FEATURE_MMX) |
> >                      bitmaskof(X86_FEATURE_FXSR) |
> > diff -r 7a82c2e2eb33 xen/arch/x86/hvm/vmx/vmx.c
> > --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Jan 19 13:14:02 2012 +0100
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c Thu Jan 19 14:37:17 2012 +0100
> > @@ -1603,6 +1603,8 @@ static void vmx_cpuid_intercept(
> >              break;
> >      }
> >  
> > +    vpmu_do_cpuid(input, eax, ebx, ecx, edx);
> > +
> >      HVMTRACE_5D (CPUID, input, *eax, *ebx, *ecx, *edx);
> >  }
> >  
> > diff -r 7a82c2e2eb33 xen/arch/x86/hvm/vmx/vpmu_core2.c
> > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c Thu Jan 19 13:14:02 2012 +0100
> > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c Thu Jan 19 14:37:17 2012 +0100
> > @@ -598,6 +598,29 @@ static void core2_vpmu_destroy(struct vc
> >      vpmu->flags &= ~VPMU_CONTEXT_ALLOCATED;
> >  }
> >  
> > +/**
> > + * core2_vpmu_cpuid - prepare special vpmu cpuid bits
> > + * If emulation of vpmu is switched off, some bits are swtiched off,
> > currently:
> > + * - EAX[0x1].EAX[Bits 0-7]: PMC revision id.
> > + * - EAX[0xa].EDX[Bit 21]:   Debug Store
> > + */
> > +#define bitmaskof(idx)  (1U << ((idx) & 31))
> > +static void core2_vpmu_cpuid(unsigned int input,
> > +                             unsigned int *eax, unsigned int *ebx,
> > +                             unsigned int *ecx, unsigned int *edx)
> > +{
> > +    switch ( input )
> > +    {
> > +    case 0x1:
> > +        *edx &= ~bitmaskof(X86_FEATURE_DS);    /* Debug Store not supported
> > */
> > +        break;
> > +    case 0xa:
> > +        if ( !opt_vpmu_enabled )
> > +            *eax &= ~(unsigned int)0xff;       /* Clear pmc version id. */
> > +        break;
> > +    }
> > +}
> > +
> >  struct arch_vpmu_ops core2_vpmu_ops = {
> >      .do_wrmsr = core2_vpmu_do_wrmsr,
> >      .do_rdmsr = core2_vpmu_do_rdmsr,
> > @@ -605,7 +628,13 @@ struct arch_vpmu_ops core2_vpmu_ops = {
> >      .arch_vpmu_initialise = core2_vpmu_initialise,
> >      .arch_vpmu_destroy = core2_vpmu_destroy,
> >      .arch_vpmu_save = core2_vpmu_save,
> > -    .arch_vpmu_load = core2_vpmu_load
> > +    .arch_vpmu_load = core2_vpmu_load,
> > +    .arch_vpmu_cpuid = core2_vpmu_cpuid
> > +};
> > +
> > +/* Used if vpmu is disabled. */
> > +struct arch_vpmu_ops core2_vpmu_dis_ops = {
> > +    .arch_vpmu_cpuid = core2_vpmu_cpuid
> >  };
> >  
> >  int vmx_vpmu_initialize(struct vcpu *v)
> > @@ -615,7 +644,7 @@ int vmx_vpmu_initialize(struct vcpu *v)
> >      __u8 cpu_model = current_cpu_data.x86_model;
> >  
> >      if ( !opt_vpmu_enabled )
> > -        return -EINVAL;
> > +        goto func_out;
> >  
> >      if ( family == 6 )
> >      {
> > @@ -635,6 +664,11 @@ int vmx_vpmu_initialize(struct vcpu *v)
> >      printk("VPMU: Initialization failed. "
> >             "Intel processor family %d model %d has not "
> >             "been supported\n", family, cpu_model);
> > +
> > +func_out:
> > +
> > +    vpmu->arch_vpmu_ops = &core2_vpmu_dis_ops;
> > +
> >      return -EINVAL;
> >  }
> >  
> > diff -r 7a82c2e2eb33 xen/arch/x86/hvm/vpmu.c
> > --- a/xen/arch/x86/hvm/vpmu.c Thu Jan 19 13:14:02 2012 +0100
> > +++ b/xen/arch/x86/hvm/vpmu.c Thu Jan 19 14:37:17 2012 +0100
> > @@ -39,7 +39,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;
> >  }
> > @@ -48,7 +48,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;
> >  }
> > @@ -57,7 +57,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;
> >  }
> > @@ -66,7 +66,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);
> >  }
> >  
> > @@ -74,7 +74,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);
> >  }
> >  
> > @@ -109,7 +109,8 @@ void vpmu_initialise(struct vcpu *v)
> >      {
> >          vpmu->flags = 0;
> >          vpmu->context = NULL;
> > -        vpmu->arch_vpmu_ops->arch_vpmu_initialise(v);
> > +        if ( vpmu->arch_vpmu_ops->arch_vpmu_initialise )
> > +            vpmu->arch_vpmu_ops->arch_vpmu_initialise(v);
> >      }
> >  }
> >  
> > @@ -117,7 +118,17 @@ 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);
> >  }
> >  
> > +void vpmu_do_cpuid(unsigned int input,
> > +                   unsigned int *eax, unsigned int *ebx,
> > +                   unsigned int *ecx, unsigned int *edx)
> > +{
> > +    struct vpmu_struct *vpmu = vcpu_vpmu(current);
> > +
> > +    if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_cpuid)
> > +        vpmu->arch_vpmu_ops->arch_vpmu_cpuid(input, eax, ebx, ecx, edx);
> > +}
> > +
> > diff -r 7a82c2e2eb33 xen/include/asm-x86/hvm/vpmu.h
> > --- a/xen/include/asm-x86/hvm/vpmu.h Thu Jan 19 13:14:02 2012 +0100
> > +++ b/xen/include/asm-x86/hvm/vpmu.h Thu Jan 19 14:37:17 2012 +0100
> > @@ -56,6 +56,9 @@ struct arch_vpmu_ops {
> >      void (*arch_vpmu_destroy)(struct vcpu *v);
> >      void (*arch_vpmu_save)(struct vcpu *v);
> >      void (*arch_vpmu_load)(struct vcpu *v);
> > +    void (*arch_vpmu_cpuid)(unsigned int input,
> > +                            unsigned int *eax, unsigned int *ebx,
> > +                            unsigned int *ecx, unsigned int *edx);
> >  };
> >  
> >  int vmx_vpmu_initialize(struct vcpu *v);
> > @@ -78,6 +81,9 @@ void vpmu_initialise(struct vcpu *v);
> >  void vpmu_destroy(struct vcpu *v);
> >  void vpmu_save(struct vcpu *v);
> >  void vpmu_load(struct vcpu *v);
> > +void vpmu_do_cpuid(unsigned int input,
> > +                   unsigned int *eax, unsigned int *ebx,
> > +                   unsigned int *ecx, unsigned int *edx);
> >  
> >  extern int acquire_pmu_ownership(int pmu_ownership);
> >  extern void release_pmu_ownership(int pmu_ownership);
> 
> 
> 
-- 
Company details: http://ts.fujitsu.com/imprint.html