Re: [PATCH] Intel/VPMU: Add support for full-width PMC writes

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

Am Montag 22 Juli 2013, 14:54:31 schrieb Boris Ostrovsky:
> A recent Linux commit (069e0c3c405814778c7475d95b9fff5318f39834) added
> support for full-width PMC writes to performance counter registers,
> making these registers default for perf. Since current Xen VPMU does
> not support these new MSRs perf will fail to initialise in guests.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Question to Intel folks: Do we need to update MSR bitmap for both original
> (0xc1) and alias (0x4c1) registers or will either one suffice?
> 
> ---
>  xen/arch/x86/hvm/vmx/vpmu_core2.c | 44 +++++++++++++++++++++++++++++++++++----
>  xen/include/asm-x86/msr-index.h   |  1 +
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> index 15b2036..823d364 100644
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -64,6 +64,10 @@
>  #define PMU_FIXED_WIDTH_BITS     8  /* 8 bits 5..12 */
>  #define PMU_FIXED_WIDTH_MASK     (((1 << PMU_FIXED_WIDTH_BITS) -1) << PMU_FIXED_WIDTH_SHIFT)
>  
> +/* Alias registers (0x4c1) for full-width writes to PMCs */
> +#define MSR_PMC_ALIAS_MASK       (~0x400)
> +bool_t __read_mostly fw_write;

Why not use "full_width_write" (or similar) like in the original linux kernel
patch for better readability?

> +
>  /*
>   * QUIRK to workaround an issue on various family 6 cpus.
>   * The issue leads to endless PMC interrupt loops on the processor.
> @@ -195,6 +199,7 @@ static int core2_get_bitwidth_fix_count(void)
>  static int is_core2_vpmu_msr(u32 msr_index, int *type, int *index)
>  {
>      int i;
> +    u32 msr_index_pmc;
>  
>      for ( i = 0; i < core2_fix_counters.num; i++ )
>      {
> @@ -224,11 +229,12 @@ static int is_core2_vpmu_msr(u32 msr_index, int *type, int *index)
>          return 1;
>      }
>  
> -    if ( (msr_index >= MSR_IA32_PERFCTR0) &&
> -         (msr_index < (MSR_IA32_PERFCTR0 + core2_get_pmc_count())) )
> +    msr_index_pmc =  msr_index & MSR_PMC_ALIAS_MASK;
> +    if ( (msr_index_pmc >= MSR_IA32_PERFCTR0) &&
> +         (msr_index_pmc < (MSR_IA32_PERFCTR0 + core2_get_pmc_count())) )
>      {
>          *type = MSR_TYPE_ARCH_COUNTER;
> -        *index = msr_index - MSR_IA32_PERFCTR0;
> +        *index = msr_index_pmc - MSR_IA32_PERFCTR0;
>          return 1;
>      }
>  
> @@ -260,6 +266,15 @@ static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap)
>          clear_bit(msraddr_to_bitpos(MSR_IA32_PERFCTR0+i),
>                    msr_bitmap + 0x800/BYTES_PER_LONG);
>      }
> +    if ( fw_write )
> +    {
> +        for ( i = 0; i < core2_get_pmc_count(); i++ )
> +        {
> +            clear_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i), msr_bitmap);
> +            clear_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i),
> +                      msr_bitmap + 0x800/BYTES_PER_LONG);
> +        }
> +    }
>  
>      /* Allow Read PMU Non-global Controls Directly. */
>      for ( i = 0; i < core2_ctrls.num; i++ )
> @@ -284,6 +299,16 @@ static void core2_vpmu_unset_msr_bitmap(unsigned long *msr_bitmap)
>          set_bit(msraddr_to_bitpos(MSR_IA32_PERFCTR0+i),
>                  msr_bitmap + 0x800/BYTES_PER_LONG);
>      }
> +    if ( fw_write )
> +    {
> +        for ( i = 0; i < core2_get_pmc_count(); i++ )
> +        {
> +            set_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i), msr_bitmap);
> +            set_bit(msraddr_to_bitpos(MSR_IA32_A_PERFCTR0+i),
> +                      msr_bitmap + 0x800/BYTES_PER_LONG);
> +        }
> +    }
> +
>      for ( i = 0; i < core2_ctrls.num; i++ )
>          set_bit(msraddr_to_bitpos(core2_ctrls.msr[i]), msr_bitmap);
>      for ( i = 0; i < core2_get_pmc_count(); i++ )
> @@ -324,11 +349,17 @@ static inline void __core2_vpmu_load(struct vcpu *v)
>  {
>      int i;
>      struct core2_vpmu_context *core2_vpmu_cxt = vcpu_vpmu(v)->context;
> +    int pmc_start;
>  
>      for ( i = 0; i < core2_fix_counters.num; i++ )
>          wrmsrl(core2_fix_counters.msr[i], core2_vpmu_cxt->fix_counters[i]);
> +    

Leading spaces.

Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>


> +    if ( fw_write )
> +        pmc_start = MSR_IA32_A_PERFCTR0;
> +    else
> +        pmc_start = MSR_IA32_PERFCTR0;
>      for ( i = 0; i < core2_get_pmc_count(); i++ )
> -        wrmsrl(MSR_IA32_PERFCTR0+i, core2_vpmu_cxt->arch_msr_pair[i].counter);
> +        wrmsrl(pmc_start+i, core2_vpmu_cxt->arch_msr_pair[i].counter);
>  
>      for ( i = 0; i < core2_ctrls.num; i++ )
>          wrmsrl(core2_ctrls.msr[i], core2_vpmu_cxt->ctrls[i]);
> @@ -855,6 +886,11 @@ int vmx_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
>  
>      if ( family == 6 )
>      {
> +        u64 caps;
> +
> +        rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);
> +        fw_write = (caps >> 13) & 1;
> +
>          switch ( cpu_model )
>          {
>          /* Core2: */
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index f500efd..5acaee8 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -34,6 +34,7 @@
>  /* Intel MSRs. Some also available on other CPUs */
>  #define MSR_IA32_PERFCTR0		0x000000c1
>  #define MSR_IA32_PERFCTR1		0x000000c2
> +#define MSR_IA32_A_PERFCTR0		0x000004c1
>  #define MSR_FSB_FREQ			0x000000cd
>  
>  #define MSR_NHM_SNB_PKG_CST_CFG_CTL	0x000000e2
> 
-- 
Company details: http://ts.fujitsu.com/imprint.html