[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v6 08/19] x86/VPMU: Add public xenpmu.h



Am Dienstag 13 Mai 2014, 11:53:22 schrieb Boris Ostrovsky:
> Add pmu.h header files, move various macros and structures that will be
> shared between hypervisor and PV guests to it.
> 
> Move MSR banks out of architectural PMU structures to allow for larger sizes
> in the future. The banks are allocated immediately after the context and
> PMU structures store offsets to them.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> Tested-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>

It seems I didn't test right because calling
# xl debug-keys q
when running a HVM linux guest crashs the hypervisor.
Please see below.


> ---
>  xen/arch/x86/hvm/svm/vpmu.c              |  81 ++++++++++++-----------
>  xen/arch/x86/hvm/vmx/vpmu_core2.c        | 110 
> +++++++++++++++++--------------
>  xen/arch/x86/hvm/vpmu.c                  |   1 +
>  xen/arch/x86/oprofile/op_model_ppro.c    |   6 +-
>  xen/include/asm-x86/hvm/vmx/vpmu_core2.h |  32 ---------
>  xen/include/asm-x86/hvm/vpmu.h           |  16 ++---
>  xen/include/public/arch-arm.h            |   3 +
>  xen/include/public/arch-x86/pmu.h        |  62 +++++++++++++++++
>  xen/include/public/pmu.h                 |  38 +++++++++++
>  9 files changed, 220 insertions(+), 129 deletions(-)
>  delete mode 100644 xen/include/asm-x86/hvm/vmx/vpmu_core2.h
>  create mode 100644 xen/include/public/arch-x86/pmu.h
>  create mode 100644 xen/include/public/pmu.h
> 
> diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
> index 2fbe2c1..ebdba8e 100644
> --- a/xen/arch/x86/hvm/svm/vpmu.c
> +++ b/xen/arch/x86/hvm/svm/vpmu.c
> @@ -30,10 +30,7 @@
>  #include <asm/apic.h>
>  #include <asm/hvm/vlapic.h>
>  #include <asm/hvm/vpmu.h>
> -
> -#define F10H_NUM_COUNTERS 4
> -#define F15H_NUM_COUNTERS 6
> -#define MAX_NUM_COUNTERS F15H_NUM_COUNTERS
> +#include <public/pmu.h>
>  
>  #define MSR_F10H_EVNTSEL_GO_SHIFT   40
>  #define MSR_F10H_EVNTSEL_EN_SHIFT   22
> @@ -49,6 +46,10 @@ static const u32 __read_mostly *counters;
>  static const u32 __read_mostly *ctrls;
>  static bool_t __read_mostly k7_counters_mirrored;
>  
> +#define F10H_NUM_COUNTERS   4
> +#define F15H_NUM_COUNTERS   6
> +#define AMD_MAX_COUNTERS    6
> +
>  /* PMU Counter MSRs. */
>  static const u32 AMD_F10H_COUNTERS[] = {
>      MSR_K7_PERFCTR0,
> @@ -83,12 +84,10 @@ static const u32 AMD_F15H_CTRLS[] = {
>      MSR_AMD_FAM15H_EVNTSEL5
>  };
>  
> -/* storage for context switching */
> -struct amd_vpmu_context {
> -    u64 counters[MAX_NUM_COUNTERS];
> -    u64 ctrls[MAX_NUM_COUNTERS];
> -    bool_t msr_bitmap_set;
> -};
> +/* Use private context as a flag for MSR bitmap */
> +#define msr_bitmap_on(vpmu)    {vpmu->priv_context = (void *)-1;}
> +#define msr_bitmap_off(vpmu)   {vpmu->priv_context = NULL;}
> +#define is_msr_bitmap_on(vpmu) (vpmu->priv_context != NULL)
>  
>  static inline int get_pmu_reg_type(u32 addr)
>  {
> @@ -142,7 +141,6 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
>  {
>      unsigned int i;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
>  
>      for ( i = 0; i < num_counters; i++ )
>      {
> @@ -150,14 +148,13 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
>          svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_WRITE);
>      }
>  
> -    ctxt->msr_bitmap_set = 1;
> +    msr_bitmap_on(vpmu);
>  }
>  
>  static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
>  {
>      unsigned int i;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
>  
>      for ( i = 0; i < num_counters; i++ )
>      {
> @@ -165,7 +162,7 @@ static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
>          svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_RW);
>      }
>  
> -    ctxt->msr_bitmap_set = 0;
> +    msr_bitmap_off(vpmu);
>  }
>  
>  static int amd_vpmu_do_interrupt(struct cpu_user_regs *regs)
> @@ -177,19 +174,22 @@ static inline void context_load(struct vcpu *v)
>  {
>      unsigned int i;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
> +    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
> +    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>  
>      for ( i = 0; i < num_counters; i++ )
>      {
> -        wrmsrl(counters[i], ctxt->counters[i]);
> -        wrmsrl(ctrls[i], ctxt->ctrls[i]);
> +        wrmsrl(counters[i], counter_regs[i]);
> +        wrmsrl(ctrls[i], ctrl_regs[i]);
>      }
>  }
>  
>  static void amd_vpmu_load(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
> +    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>  
>      vpmu_reset(vpmu, VPMU_FROZEN);
>  
> @@ -198,7 +198,7 @@ static void amd_vpmu_load(struct vcpu *v)
>          unsigned int i;
>  
>          for ( i = 0; i < num_counters; i++ )
> -            wrmsrl(ctrls[i], ctxt->ctrls[i]);
> +            wrmsrl(ctrls[i], ctrl_regs[i]);
>  
>          return;
>      }
> @@ -212,17 +212,17 @@ static inline void context_save(struct vcpu *v)
>  {
>      unsigned int i;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
> +    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
>  
>      /* No need to save controls -- they are saved in amd_vpmu_do_wrmsr */
>      for ( i = 0; i < num_counters; i++ )
> -        rdmsrl(counters[i], ctxt->counters[i]);
> +        rdmsrl(counters[i], counter_regs[i]);
>  }
>  
>  static int amd_vpmu_save(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctx = vpmu->context;
>      unsigned int i;
>  
>      /*
> @@ -245,7 +245,7 @@ static int amd_vpmu_save(struct vcpu *v)
>      context_save(v);
>  
>      if ( is_hvm_domain(v->domain) &&
> -        !vpmu_is_set(vpmu, VPMU_RUNNING) && ctx->msr_bitmap_set )
> +        !vpmu_is_set(vpmu, VPMU_RUNNING) && is_msr_bitmap_on(vpmu) )
>          amd_vpmu_unset_msr_bitmap(v);
>  
>      return 1;
> @@ -256,7 +256,9 @@ static void context_update(unsigned int msr, u64 
> msr_content)
>      unsigned int i;
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct amd_vpmu_context *ctxt = vpmu->context;
> +    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
> +    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>  
>      if ( k7_counters_mirrored &&
>          ((msr >= MSR_K7_EVNTSEL0) && (msr <= MSR_K7_PERFCTR3)) )
> @@ -268,12 +270,12 @@ static void context_update(unsigned int msr, u64 
> msr_content)
>      {
>         if ( msr == ctrls[i] )
>         {
> -           ctxt->ctrls[i] = msr_content;
> +           ctrl_regs[i] = msr_content;
>             return;
>         }
>          else if (msr == counters[i] )
>          {
> -            ctxt->counters[i] = msr_content;
> +            counter_regs[i] = msr_content;
>              return;
>          }
>      }
> @@ -299,8 +301,7 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> msr_content)
>              return 1;
>          vpmu_set(vpmu, VPMU_RUNNING);
>  
> -        if ( is_hvm_domain(v->domain) &&
> -             !((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
> +        if ( is_hvm_domain(v->domain) && is_msr_bitmap_on(vpmu) )
>              amd_vpmu_set_msr_bitmap(v);
>      }
>  
> @@ -309,8 +310,7 @@ static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> msr_content)
>          (is_pmu_enabled(msr_content) == 0) && vpmu_is_set(vpmu, 
> VPMU_RUNNING) )
>      {
>          vpmu_reset(vpmu, VPMU_RUNNING);
> -        if ( is_hvm_domain(v->domain) &&
> -             ((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
> +        if ( is_hvm_domain(v->domain) && is_msr_bitmap_on(vpmu) )
>              amd_vpmu_unset_msr_bitmap(v);
>          release_pmu_ownship(PMU_OWNER_HVM);
>      }
> @@ -351,7 +351,7 @@ static int amd_vpmu_do_rdmsr(unsigned int msr, uint64_t 
> *msr_content)
>  
>  static int amd_vpmu_initialise(struct vcpu *v)
>  {
> -    struct amd_vpmu_context *ctxt;
> +    struct xen_pmu_amd_ctxt *ctxt;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      uint8_t family = current_cpu_data.x86;
>  
> @@ -381,7 +381,9 @@ static int amd_vpmu_initialise(struct vcpu *v)
>        }
>      }
>  
> -    ctxt = xzalloc(struct amd_vpmu_context);
> +    ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) + 
> +                      sizeof(uint64_t) * AMD_MAX_COUNTERS + 
> +                      sizeof(uint64_t) * AMD_MAX_COUNTERS);
>      if ( !ctxt )
>      {
>          gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> @@ -390,7 +392,11 @@ static int amd_vpmu_initialise(struct vcpu *v)
>          return -ENOMEM;
>      }
>  
> +    ctxt->counters = sizeof(struct xen_pmu_amd_ctxt);
> +    ctxt->ctrls = ctxt->counters + sizeof(uint64_t) * AMD_MAX_COUNTERS;
> +
>      vpmu->context = ctxt;
> +    vpmu->priv_context = NULL;

msr_bitmap_off(vpmu) ?

>      vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
>      return 0;
>  }
> @@ -402,8 +408,7 @@ static void amd_vpmu_destroy(struct vcpu *v)
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>          return;
>  
> -    if ( is_hvm_domain(v->domain) &&
> -         ((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
> +    if ( is_hvm_domain(v->domain) && is_msr_bitmap_on(vpmu) )
>          amd_vpmu_unset_msr_bitmap(v);
>  
>      xfree(vpmu->context);
> @@ -420,7 +425,9 @@ static void amd_vpmu_destroy(struct vcpu *v)
>  static void amd_vpmu_dump(const struct vcpu *v)
>  {
>      const struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    const struct amd_vpmu_context *ctxt = vpmu->context;
> +    const struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
> +    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>      unsigned int i;
>  
>      printk("    VPMU state: 0x%x ", vpmu->flags);
> @@ -450,8 +457,8 @@ static void amd_vpmu_dump(const struct vcpu *v)
>          rdmsrl(ctrls[i], ctrl);
>          rdmsrl(counters[i], cntr);
>          printk("      %#x: %#lx (%#lx in HW)    %#x: %#lx (%#lx in HW)\n",
> -               ctrls[i], ctxt->ctrls[i], ctrl,
> -               counters[i], ctxt->counters[i], cntr);
> +               ctrls[i], ctrl_regs[i], ctrl,
> +               counters[i], counter_regs[i], cntr);
>      }
>  }
>  
> diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c 
> b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> index dffdd80..1fe583f 100644
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -35,8 +35,8 @@
>  #include <asm/hvm/vmx/vmcs.h>
>  #include <public/sched.h>
>  #include <public/hvm/save.h>
> +#include <public/pmu.h>
>  #include <asm/hvm/vpmu.h>
> -#include <asm/hvm/vmx/vpmu_core2.h>
>  
>  /*
>   * See Intel SDM Vol 2a Instruction Set Reference chapter 3 for CPUID
> @@ -68,6 +68,10 @@
>  #define MSR_PMC_ALIAS_MASK       (~(MSR_IA32_PERFCTR0 ^ MSR_IA32_A_PERFCTR0))
>  static bool_t __read_mostly full_width_write;
>  
> +/* Intel-specific VPMU features */
> +#define VPMU_CPU_HAS_DS                     0x100 /* Has Debug Store */
> +#define VPMU_CPU_HAS_BTS                    0x200 /* Has Branch Trace Store 
> */
> +
>  /*
>   * MSR_CORE_PERF_FIXED_CTR_CTRL contains the configuration of all fixed
>   * counters. 4 bits for every counter.
> @@ -75,17 +79,6 @@ static bool_t __read_mostly full_width_write;
>  #define FIXED_CTR_CTRL_BITS 4
>  #define FIXED_CTR_CTRL_MASK ((1 << FIXED_CTR_CTRL_BITS) - 1)
>  
> -#define VPMU_CORE2_MAX_FIXED_PMCS     4
> -struct core2_vpmu_context {
> -    u64 fixed_ctrl;
> -    u64 ds_area;
> -    u64 pebs_enable;
> -    u64 global_ovf_status;
> -    u64 enabled_cntrs;  /* Follows PERF_GLOBAL_CTRL MSR format */
> -    u64 fix_counters[VPMU_CORE2_MAX_FIXED_PMCS];
> -    struct arch_msr_pair arch_msr_pair[1];
> -};
> -
>  /* Number of general-purpose and fixed performance counters */
>  static unsigned int __read_mostly arch_pmc_cnt, fixed_pmc_cnt;
>  
> @@ -225,6 +218,7 @@ static int is_core2_vpmu_msr(u32 msr_index, int *type, 
> int *index)
>      return 0;
>  }
>  
> +#define msraddr_to_bitpos(x) (((x)&0xffff) + ((x)>>31)*0x2000)
>  static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap)
>  {
>      int i;
> @@ -294,12 +288,15 @@ static void core2_vpmu_unset_msr_bitmap(unsigned long 
> *msr_bitmap)
>  static inline void __core2_vpmu_save(struct vcpu *v)
>  {
>      int i;
> -    struct core2_vpmu_context *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    uint64_t *fixed_counters = vpmu_reg_pointer(core2_vpmu_cxt, 
> fixed_counters);
> +    struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
> +        vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);
>  
>      for ( i = 0; i < fixed_pmc_cnt; i++ )
> -        rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 
> core2_vpmu_cxt->fix_counters[i]);
> +        rdmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, fixed_counters[i]);
>      for ( i = 0; i < arch_pmc_cnt; i++ )
> -        rdmsrl(MSR_IA32_PERFCTR0 + i, 
> core2_vpmu_cxt->arch_msr_pair[i].counter);
> +        rdmsrl(MSR_IA32_PERFCTR0 + i, xen_pmu_cntr_pair[i].counter);
>  }
>  
>  static int core2_vpmu_save(struct vcpu *v)
> @@ -322,10 +319,13 @@ static int core2_vpmu_save(struct vcpu *v)
>  static inline void __core2_vpmu_load(struct vcpu *v)
>  {
>      unsigned int i, pmc_start;
> -    struct core2_vpmu_context *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    uint64_t *fixed_counters = vpmu_reg_pointer(core2_vpmu_cxt, 
> fixed_counters);
> +    struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
> +        vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);
>  
>      for ( i = 0; i < fixed_pmc_cnt; i++ )
> -        wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, 
> core2_vpmu_cxt->fix_counters[i]);
> +        wrmsrl(MSR_CORE_PERF_FIXED_CTR0 + i, fixed_counters[i]);
>  
>      if ( full_width_write )
>          pmc_start = MSR_IA32_A_PERFCTR0;
> @@ -333,8 +333,8 @@ static inline void __core2_vpmu_load(struct vcpu *v)
>          pmc_start = MSR_IA32_PERFCTR0;
>      for ( i = 0; i < arch_pmc_cnt; i++ )
>      {
> -        wrmsrl(pmc_start + i, core2_vpmu_cxt->arch_msr_pair[i].counter);
> -        wrmsrl(MSR_P6_EVNTSEL0 + i, 
> core2_vpmu_cxt->arch_msr_pair[i].control);
> +        wrmsrl(pmc_start + i, xen_pmu_cntr_pair[i].counter);
> +        wrmsrl(MSR_P6_EVNTSEL0 + i, xen_pmu_cntr_pair[i].control);
>      }
>  
>      wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
> @@ -357,7 +357,8 @@ static void core2_vpmu_load(struct vcpu *v)
>  static int core2_vpmu_alloc_resource(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;
> +    uint64_t *p = NULL;
>  
>      if ( !acquire_pmu_ownership(PMU_OWNER_HVM) )
>          return 0;
> @@ -370,12 +371,20 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>          goto out_err;
>      vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>  
> -    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct core2_vpmu_context) +
> -                    (arch_pmc_cnt-1)*sizeof(struct arch_msr_pair));
> -    if ( !core2_vpmu_cxt )
> +    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct xen_pmu_intel_ctxt) +
> +                                   sizeof(uint64_t) * fixed_pmc_cnt +
> +                                   sizeof(struct xen_pmu_cntr_pair) *
> +                                   arch_pmc_cnt);
> +    p = xzalloc_bytes(sizeof(uint64_t));
> +    if ( !core2_vpmu_cxt || !p )
>          goto out_err;
>  
> +    core2_vpmu_cxt->fixed_counters = sizeof(struct xen_pmu_intel_ctxt);
> +    core2_vpmu_cxt->arch_counters = core2_vpmu_cxt->fixed_counters +
> +                                    sizeof(uint64_t) * fixed_pmc_cnt;
> +
>      vpmu->context = (void *)core2_vpmu_cxt;
> +    vpmu->priv_context = (void *)p;
>  
>      vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
>  
> @@ -386,6 +395,9 @@ out_err:
>      vmx_rm_msr(MSR_CORE_PERF_GLOBAL_CTRL, VMX_GUEST_MSR);
>      release_pmu_ownship(PMU_OWNER_HVM);
>  
> +    xfree(core2_vpmu_cxt);
> +    xfree(p);
> +
>      printk("Failed to allocate VPMU resources for domain %u vcpu %u\n",
>             v->vcpu_id, v->domain->domain_id);
>  
> @@ -421,7 +433,8 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> msr_content)
>      int type = -1, index = -1;
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = NULL;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;
> +    uint64_t *enabled_cntrs;
>  
>      if ( !core2_vpmu_msr_common_check(msr, &type, &index) )
>      {
> @@ -447,10 +460,11 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>      }
>  
>      core2_vpmu_cxt = vpmu->context;
> +    enabled_cntrs = (uint64_t *)vpmu->priv_context;
>      switch ( msr )
>      {
>      case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
> -        core2_vpmu_cxt->global_ovf_status &= ~msr_content;
> +        core2_vpmu_cxt->global_status &= ~msr_content;
>          return 1;
>      case MSR_CORE_PERF_GLOBAL_STATUS:
>          gdprintk(XENLOG_INFO, "Can not write readonly MSR: "
> @@ -484,15 +498,14 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>          break;
>      case MSR_CORE_PERF_FIXED_CTR_CTRL:
>          vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl);
> -        core2_vpmu_cxt->enabled_cntrs &=
> -                ~(((1ULL << VPMU_CORE2_MAX_FIXED_PMCS) - 1) << 32);
> +        *enabled_cntrs &= ~(((1ULL << fixed_pmc_cnt) - 1) << 32);
>          if ( msr_content != 0 )
>          {
>              u64 val = msr_content;
>              for ( i = 0; i < fixed_pmc_cnt; i++ )
>              {
>                  if ( val & 3 )
> -                    core2_vpmu_cxt->enabled_cntrs |= (1ULL << 32) << i;
> +                    *enabled_cntrs |= (1ULL << 32) << i;
>                  val >>= FIXED_CTR_CTRL_BITS;
>              }
>          }
> @@ -503,19 +516,21 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>          tmp = msr - MSR_P6_EVNTSEL0;
>          if ( tmp >= 0 && tmp < arch_pmc_cnt )
>          {
> +            struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
> +                vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);
> +
>              vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl);
>  
>              if ( msr_content & (1ULL << 22) )
> -                core2_vpmu_cxt->enabled_cntrs |= 1ULL << tmp;
> +                *enabled_cntrs |= 1ULL << tmp;
>              else
> -                core2_vpmu_cxt->enabled_cntrs &= ~(1ULL << tmp);
> +                *enabled_cntrs &= ~(1ULL << tmp);
>  
> -            core2_vpmu_cxt->arch_msr_pair[tmp].control = msr_content;
> +            xen_pmu_cntr_pair[tmp].control = msr_content;
>          }
>      }
>  
> -    if ((global_ctrl & core2_vpmu_cxt->enabled_cntrs) ||
> -        (core2_vpmu_cxt->ds_area != 0)  )
> +    if ((global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0)  )
>          vpmu_set(vpmu, VPMU_RUNNING);
>      else
>          vpmu_reset(vpmu, VPMU_RUNNING);
> @@ -561,7 +576,7 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t 
> *msr_content)
>      int type = -1, index = -1;
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = NULL;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;
>  
>      if ( core2_vpmu_msr_common_check(msr, &type, &index) )
>      {
> @@ -572,7 +587,7 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t 
> *msr_content)
>              *msr_content = 0;
>              break;
>          case MSR_CORE_PERF_GLOBAL_STATUS:
> -            *msr_content = core2_vpmu_cxt->global_ovf_status;
> +            *msr_content = core2_vpmu_cxt->global_status;
>              break;
>          case MSR_CORE_PERF_GLOBAL_CTRL:
>              vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, msr_content);
> @@ -621,8 +636,11 @@ static void core2_vpmu_dump(const struct vcpu *v)
>  {
>      const struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      int i;
> -    const struct core2_vpmu_context *core2_vpmu_cxt = NULL;
> +    const struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;
>      u64 val;
> +    uint64_t *fixed_counters = vpmu_reg_pointer(core2_vpmu_cxt, 
> fixed_counters);
> +    struct xen_pmu_cntr_pair *xen_pmu_cntr_pair =
> +        vpmu_reg_pointer(core2_vpmu_cxt, arch_counters);

This crashs the hypervisor because of derefencing core2_vpmu_cxt (NULL) in
vpmu_req_pointer().

  --> +    const struct xen_pmu_intel_ctxt *core2_vpmu_cxt = vpmu->context;

Dietmar.

>  
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>           return;
> @@ -641,12 +659,9 @@ static void core2_vpmu_dump(const struct vcpu *v)
>  
>      /* Print the contents of the counter and its configuration msr. */
>      for ( i = 0; i < arch_pmc_cnt; i++ )
> -    {
> -        const struct arch_msr_pair *msr_pair = core2_vpmu_cxt->arch_msr_pair;
> -
>          printk("      general_%d: 0x%016lx ctrl: 0x%016lx\n",
> -               i, msr_pair[i].counter, msr_pair[i].control);
> -    }
> +            i, xen_pmu_cntr_pair[i].counter, xen_pmu_cntr_pair[i].control);
> +
>      /*
>       * The configuration of the fixed counter is 4 bits each in the
>       * MSR_CORE_PERF_FIXED_CTR_CTRL.
> @@ -655,7 +670,7 @@ static void core2_vpmu_dump(const struct vcpu *v)
>      for ( i = 0; i < fixed_pmc_cnt; i++ )
>      {
>          printk("      fixed_%d:   0x%016lx ctrl: %#lx\n",
> -               i, core2_vpmu_cxt->fix_counters[i],
> +               i, fixed_counters[i],
>                 val & FIXED_CTR_CTRL_MASK);
>          val >>= FIXED_CTR_CTRL_BITS;
>      }
> @@ -666,14 +681,14 @@ static int core2_vpmu_do_interrupt(struct cpu_user_regs 
> *regs)
>      struct vcpu *v = current;
>      u64 msr_content;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = vpmu->context;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = vpmu->context;
>  
>      rdmsrl(MSR_CORE_PERF_GLOBAL_STATUS, msr_content);
>      if ( msr_content )
>      {
>          if ( is_pmc_quirk )
>              handle_pmc_quirk(msr_content);
> -        core2_vpmu_cxt->global_ovf_status |= msr_content;
> +        core2_vpmu_cxt->global_status |= msr_content;
>          msr_content = 0xC000000700000000 | ((1 << arch_pmc_cnt) - 1);
>          wrmsrl(MSR_CORE_PERF_GLOBAL_OVF_CTRL, msr_content);
>      }
> @@ -736,12 +751,6 @@ func_out:
>  
>      arch_pmc_cnt = core2_get_arch_pmc_count();
>      fixed_pmc_cnt = core2_get_fixed_pmc_count();
> -    if ( fixed_pmc_cnt > VPMU_CORE2_MAX_FIXED_PMCS )
> -    {
> -        fixed_pmc_cnt = VPMU_CORE2_MAX_FIXED_PMCS;
> -        printk(XENLOG_G_WARNING "Limiting number of fixed counters to %d\n",
> -               fixed_pmc_cnt);
> -    }
>      check_pmc_quirk();
>  
>      return 0;
> @@ -755,6 +764,7 @@ static void core2_vpmu_destroy(struct vcpu *v)
>          return;
>  
>      xfree(vpmu->context);
> +    xfree(vpmu->priv_context);
>      if ( cpu_has_vmx_msr_bitmap && is_hvm_domain(v->domain) )
>          core2_vpmu_unset_msr_bitmap(v->arch.hvm_vmx.msr_bitmap);
>      release_pmu_ownship(PMU_OWNER_HVM);
> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> index 0340e5b..0acc486 100644
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -31,6 +31,7 @@
>  #include <asm/hvm/svm/svm.h>
>  #include <asm/hvm/svm/vmcb.h>
>  #include <asm/apic.h>
> +#include <public/pmu.h>
>  
>  /*
>   * "vpmu" :     vpmu generally enabled
> diff --git a/xen/arch/x86/oprofile/op_model_ppro.c 
> b/xen/arch/x86/oprofile/op_model_ppro.c
> index 3225937..5aae2e7 100644
> --- a/xen/arch/x86/oprofile/op_model_ppro.c
> +++ b/xen/arch/x86/oprofile/op_model_ppro.c
> @@ -20,11 +20,15 @@
>  #include <asm/regs.h>
>  #include <asm/current.h>
>  #include <asm/hvm/vpmu.h>
> -#include <asm/hvm/vmx/vpmu_core2.h>
>  
>  #include "op_x86_model.h"
>  #include "op_counter.h"
>  
> +struct arch_msr_pair {
> +    u64 counter;
> +    u64 control;
> +};
> +
>  /*
>   * Intel "Architectural Performance Monitoring" CPUID
>   * detection/enumeration details:
> diff --git a/xen/include/asm-x86/hvm/vmx/vpmu_core2.h 
> b/xen/include/asm-x86/hvm/vmx/vpmu_core2.h
> deleted file mode 100644
> index 410372d..0000000
> --- a/xen/include/asm-x86/hvm/vmx/vpmu_core2.h
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -
> -/*
> - * vpmu_core2.h: CORE 2 specific PMU virtualization for HVM domain.
> - *
> - * Copyright (c) 2007, Intel Corporation.
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms and conditions of the GNU General Public License,
> - * version 2, as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope it will be useful, but WITHOUT
> - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> - * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> - * more details.
> - *
> - * You should have received a copy of the GNU General Public License along 
> with
> - * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
> - * Place - Suite 330, Boston, MA 02111-1307 USA.
> - *
> - * Author: Haitao Shan <haitao.shan@xxxxxxxxx>
> - */
> -
> -#ifndef __ASM_X86_HVM_VPMU_CORE_H_
> -#define __ASM_X86_HVM_VPMU_CORE_H_
> -
> -struct arch_msr_pair {
> -    u64 counter;
> -    u64 control;
> -};
> -
> -#endif /* __ASM_X86_HVM_VPMU_CORE_H_ */
> -
> diff --git a/xen/include/asm-x86/hvm/vpmu.h b/xen/include/asm-x86/hvm/vpmu.h
> index 7ee0f01..3e5d9de 100644
> --- a/xen/include/asm-x86/hvm/vpmu.h
> +++ b/xen/include/asm-x86/hvm/vpmu.h
> @@ -22,6 +22,8 @@
>  #ifndef __ASM_X86_HVM_VPMU_H_
>  #define __ASM_X86_HVM_VPMU_H_
>  
> +#include <public/pmu.h>
> +
>  /*
>   * Flag bits given as a string on the hypervisor boot parameter 'vpmu'.
>   * See arch/x86/hvm/vpmu.c.
> @@ -29,12 +31,9 @@
>  #define VPMU_BOOT_ENABLED 0x1    /* vpmu generally enabled. */
>  #define VPMU_BOOT_BTS     0x2    /* Intel BTS feature wanted. */
>  
> -
> -#define msraddr_to_bitpos(x) (((x)&0xffff) + ((x)>>31)*0x2000)
>  #define vcpu_vpmu(vcpu)   (&((vcpu)->arch.hvm_vcpu.vpmu))
>  #define vpmu_vcpu(vpmu)   (container_of((vpmu), struct vcpu, \
>                                            arch.hvm_vcpu.vpmu))
> -#define vpmu_domain(vpmu) (vpmu_vcpu(vpmu)->domain)
>  
>  #define MSR_TYPE_COUNTER            0
>  #define MSR_TYPE_CTRL               1
> @@ -42,6 +41,9 @@
>  #define MSR_TYPE_ARCH_COUNTER       3
>  #define MSR_TYPE_ARCH_CTRL          4
>  
> +/* Start of PMU register bank */
> +#define vpmu_reg_pointer(ctxt, offset) ((void *)((uintptr_t)ctxt + \
> +                                                 (uintptr_t)ctxt->offset))
>  
>  /* Arch specific operations shared by all vpmus */
>  struct arch_vpmu_ops {
> @@ -64,7 +66,8 @@ struct vpmu_struct {
>      u32 flags;
>      u32 last_pcpu;
>      u32 hw_lapic_lvtpc;
> -    void *context;
> +    void *context;      /* May be shared with PV guest */
> +    void *priv_context; /* hypervisor-only */
>      struct arch_vpmu_ops *arch_vpmu_ops;
>  };
>  
> @@ -76,11 +79,6 @@ struct vpmu_struct {
>  #define VPMU_FROZEN                         0x10  /* Stop counters while 
> VCPU is not running */
>  #define VPMU_PASSIVE_DOMAIN_ALLOCATED       0x20
>  
> -/* VPMU features */
> -#define VPMU_CPU_HAS_DS                     0x100 /* Has Debug Store */
> -#define VPMU_CPU_HAS_BTS                    0x200 /* Has Branch Trace Store 
> */
> -
> -
>  #define vpmu_set(_vpmu, _x)         ((_vpmu)->flags |= (_x))
>  #define vpmu_reset(_vpmu, _x)       ((_vpmu)->flags &= ~(_x))
>  #define vpmu_is_set(_vpmu, _x)      ((_vpmu)->flags & (_x))
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 7496556..e982b53 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -388,6 +388,9 @@ typedef uint64_t xen_callback_t;
>  
>  #endif
>  
> +/* Stub definition of PMU structure */
> +typedef struct xen_arch_pmu {} xen_arch_pmu_t;
> +
>  #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
>  
>  /*
> diff --git a/xen/include/public/arch-x86/pmu.h 
> b/xen/include/public/arch-x86/pmu.h
> new file mode 100644
> index 0000000..b4eda67
> --- /dev/null
> +++ b/xen/include/public/arch-x86/pmu.h
> @@ -0,0 +1,62 @@
> +#ifndef __XEN_PUBLIC_ARCH_X86_PMU_H__
> +#define __XEN_PUBLIC_ARCH_X86_PMU_H__
> +
> +/* x86-specific PMU definitions */
> +
> +/* AMD PMU registers and structures */
> +struct xen_pmu_amd_ctxt {
> +    uint32_t counters;       /* Offset to counter MSRs */
> +    uint32_t ctrls;          /* Offset to control MSRs */
> +};
> +
> +/* Intel PMU registers and structures */
> +struct xen_pmu_cntr_pair {
> +    uint64_t counter;
> +    uint64_t control;
> +};
> +
> +struct xen_pmu_intel_ctxt {
> +    uint64_t global_ctrl;
> +    uint64_t global_ovf_ctrl;
> +    uint64_t global_status;
> +    uint64_t fixed_ctrl;
> +    uint64_t ds_area;
> +    uint64_t pebs_enable;
> +    uint64_t debugctl;
> +    uint32_t fixed_counters;  /* Offset to fixed counter MSRs */
> +    uint32_t arch_counters;   /* Offset to architectural counter MSRs */
> +};
> +
> +#define XENPMU_MAX_CTXT_SZ        (sizeof(struct xen_pmu_amd_ctxt) > \
> +                                    sizeof(struct xen_pmu_intel_ctxt) ? \
> +                                     sizeof(struct xen_pmu_amd_ctxt) : \
> +                                     sizeof(struct xen_pmu_intel_ctxt))
> +#define XENPMU_CTXT_PAD_SZ        (((XENPMU_MAX_CTXT_SZ + 64) & ~63) + 128)
> +struct xen_arch_pmu {
> +    union {
> +        struct cpu_user_regs regs;
> +        uint8_t pad1[256];
> +    } r;
> +    union {
> +        uint32_t lapic_lvtpc;
> +        uint64_t pad2;
> +    } l;
> +    union {
> +        struct xen_pmu_amd_ctxt amd;
> +        struct xen_pmu_intel_ctxt intel;
> +        uint8_t pad3[XENPMU_CTXT_PAD_SZ];
> +    } c;
> +};
> +typedef struct xen_arch_pmu xen_arch_pmu_t;
> +
> +#endif /* __XEN_PUBLIC_ARCH_X86_PMU_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> +
> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> new file mode 100644
> index 0000000..3ffd2cf
> --- /dev/null
> +++ b/xen/include/public/pmu.h
> @@ -0,0 +1,38 @@
> +#ifndef __XEN_PUBLIC_PMU_H__
> +#define __XEN_PUBLIC_PMU_H__
> +
> +#include "xen.h"
> +#if defined(__i386__) || defined(__x86_64__)
> +#include "arch-x86/pmu.h"
> +#elif defined (__arm__) || defined (__aarch64__)
> +#include "arch-arm.h"
> +#else
> +#error "Unsupported architecture"
> +#endif
> +
> +#define XENPMU_VER_MAJ    0
> +#define XENPMU_VER_MIN    0
> +
> +
> +/* Shared between hypervisor and PV domain */
> +struct xen_pmu_data {
> +    uint32_t domain_id;
> +    uint32_t vcpu_id;
> +    uint32_t pcpu_id;
> +    uint32_t pmu_flags;
> +
> +    xen_arch_pmu_t pmu;
> +};
> +typedef struct xen_pmu_data xen_pmu_data_t;
> +
> +#endif /* __XEN_PUBLIC_PMU_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.