Re: [PATCH v7 05/19] intel/VPMU: Clean up Intel VPMU code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/06/14 18:40, Boris Ostrovsky wrote:
> Remove struct pmumsr and core2_pmu_enable. Replace static MSR structures with
> fields in core2_vpmu_context.
>
> Call core2_get_pmc_count() once, during initialization.
>
> Properly clean up when core2_vpmu_alloc_resource() fails and add routines
> to remove MSRs from VMCS.
>
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
>
> diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -125,75 +143,42 @@ static void handle_pmc_quirk(u64 msr_content)
>      }
>  }
>  
> -static const u32 core2_fix_counters_msr[] = {
> -    MSR_CORE_PERF_FIXED_CTR0,
> -    MSR_CORE_PERF_FIXED_CTR1,
> -    MSR_CORE_PERF_FIXED_CTR2
> -};
> -
>  /*
> - * MSR_CORE_PERF_FIXED_CTR_CTRL contains the configuration of all fixed
> - * counters. 4 bits for every counter.
> + * Read the number of general counters via CPUID.EAX[0xa].EAX[8..15]
>   */
> -#define FIXED_CTR_CTRL_BITS 4
> -#define FIXED_CTR_CTRL_MASK ((1 << FIXED_CTR_CTRL_BITS) - 1)
> -
> -/* The index into the core2_ctrls_msr[] of this MSR used in core2_vpmu_dump() */
> -#define MSR_CORE_PERF_FIXED_CTR_CTRL_IDX 0
> -
> -/* Core 2 Non-architectual Performance Control MSRs. */
> -static const u32 core2_ctrls_msr[] = {
> -    MSR_CORE_PERF_FIXED_CTR_CTRL,
> -    MSR_IA32_PEBS_ENABLE,
> -    MSR_IA32_DS_AREA
> -};
> -
> -struct pmumsr {
> -    unsigned int num;
> -    const u32 *msr;
> -};
> -
> -static const struct pmumsr core2_fix_counters = {
> -    VPMU_CORE2_NUM_FIXED,
> -    core2_fix_counters_msr
> -};
> +static int core2_get_arch_pmc_count(void)
> +{
> +    u32 eax;
>  
> -static const struct pmumsr core2_ctrls = {
> -    VPMU_CORE2_NUM_CTRLS,
> -    core2_ctrls_msr
> -};
> -static int arch_pmc_cnt;
> +    eax = cpuid_eax(0xa);
> +    return ( (eax & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT );
> +}

This (and later) can be made much simpler.  The style guidelines easily
permit:

static int core2_get_arch_pmc_count(void)
{
    return (cpuid_eax(0xa) & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT;
}

Unrelated to this code itself, I wonder whether Xen should gain some
mnemonics for cpuid leaves.

>  
>  /*
> - * Read the number of general counters via CPUID.EAX[0xa].EAX[8..15]
> + * Read the number of fixed counters via CPUID.EDX[0xa].EDX[0..4]
>   */
> -static int core2_get_pmc_count(void)
> +static int core2_get_fixed_pmc_count(void)
>  {
> -    u32 eax, ebx, ecx, edx;
> -
> -    if ( arch_pmc_cnt == 0 )
> -    {
> -        cpuid(0xa, &eax, &ebx, &ecx, &edx);
> -        arch_pmc_cnt = (eax & PMU_GENERAL_NR_MASK) >> PMU_GENERAL_NR_SHIFT;
> -    }
> +    u32 eax;
>  
> -    return arch_pmc_cnt;
> +    eax = cpuid_eax(0xa);
> +    return ( (eax & PMU_FIXED_NR_MASK) >> PMU_FIXED_NR_SHIFT );
>  }
>  
>  static u64 core2_calc_intial_glb_ctrl_msr(void)
>  {
> -    int arch_pmc_bits = (1 << core2_get_pmc_count()) - 1;
> -    u64 fix_pmc_bits  = (1 << 3) - 1;
> -    return ((fix_pmc_bits << 32) | arch_pmc_bits);
> +    int arch_pmc_bits = (1 << arch_pmc_cnt) - 1;
> +    u64 fix_pmc_bits  = (1 << fixed_pmc_cnt) - 1;

Newline after variable declarations.

> +    return ( (fix_pmc_bits << 32) | arch_pmc_bits );

Redundant brackets.

>  }
>  
>  /* edx bits 5-12: Bit width of fixed-function performance counters  */
>  static int core2_get_bitwidth_fix_count(void)
>  {
> -    u32 eax, ebx, ecx, edx;
> +    u32 edx;
>  
> -    cpuid(0xa, &eax, &ebx, &ecx, &edx);
> -    return ((edx & PMU_FIXED_WIDTH_MASK) >> PMU_FIXED_WIDTH_SHIFT);
> +    edx = cpuid_edx(0xa);
> +    return ( (edx & PMU_FIXED_WIDTH_MASK) >> PMU_FIXED_WIDTH_SHIFT );
>  }
>  
>  static int is_core2_vpmu_msr(u32 msr_index, int *type, int *index)
> @@ -201,9 +186,9 @@ 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++ )
> +    for ( i = 0; i < fixed_pmc_cnt; i++ )
>      {
> -        if ( core2_fix_counters.msr[i] == msr_index )
> +        if ( msr_index == MSR_CORE_PERF_FIXED_CTR0 + i )
>          {
>              *type = MSR_TYPE_COUNTER;
>              *index = i;
> @@ -211,14 +196,12 @@ static int is_core2_vpmu_msr(u32 msr_index, int *type, int *index)
>          }
>      }
>  
> -    for ( i = 0; i < core2_ctrls.num; i++ )
> +    if ( (msr_index == MSR_CORE_PERF_FIXED_CTR_CTRL ) ||

Stray space before the closing bracket

> +        (msr_index == MSR_IA32_DS_AREA) ||
> +        (msr_index == MSR_IA32_PEBS_ENABLE) )
>      {
> -        if ( core2_ctrls.msr[i] == msr_index )
> -        {
> -            *type = MSR_TYPE_CTRL;
> -            *index = i;
> -            return 1;
> -        }
> +        *type = MSR_TYPE_CTRL;
> +        return 1;
>      }
>  
>      if ( (msr_index == MSR_CORE_PERF_GLOBAL_CTRL) ||
> @@ -275,26 +258,28 @@ static void core2_vpmu_set_msr_bitmap(unsigned long *msr_bitmap)
>      }
>  
>      /* Allow Read PMU Non-global Controls Directly. */
> -    for ( i = 0; i < core2_ctrls.num; i++ )
> -        clear_bit(msraddr_to_bitpos(core2_ctrls.msr[i]), msr_bitmap);
> -    for ( i = 0; i < core2_get_pmc_count(); i++ )
> -        clear_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL0+i), msr_bitmap);
> +    for ( i = 0; i < arch_pmc_cnt; i++ )
> +         clear_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL0 + i), msr_bitmap);
> +
> +    clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR_CTRL), msr_bitmap);
> +    clear_bit(msraddr_to_bitpos(MSR_IA32_PEBS_ENABLE), msr_bitmap);
> +    clear_bit(msraddr_to_bitpos(MSR_IA32_DS_AREA), msr_bitmap);

This code looks as if "clear_msr_in_map(MSR_$FOO, msr_bitmap)" would be
a useful helper.

> @@ -801,18 +759,27 @@ static int core2_vpmu_initialise(struct vcpu *v, unsigned int vpmu_flags)
>          }
>      }
>  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;
>  }
>  
>  static void core2_vpmu_destroy(struct vcpu *v)
>  {
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = vpmu->context;
>  
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>          return;
> -    xfree(core2_vpmu_cxt->pmu_enable);
> +
>      xfree(vpmu->context);

Is it really right to bail if !VPMU_CONTEXT_ALLOCATED ?  A stray
vpmu_clear() would result in a memory leak.

i.e. can VPMU_CONTEXT_ALLOCATED be deemed from "vpmu->context != NULL"
instead?

~Andrew