Re: [Xen-devel] [PATCH v8 04/19] intel/VPMU: Clean up Intel VPMU code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
> +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
> @@ -69,6 +69,27 @@
>  static bool_t __read_mostly full_width_write;
>  
>  /*
> + * MSR_CORE_PERF_FIXED_CTR_CTRL contains the configuration of all fixed
> + * counters. 4 bits for every counter.
> + */
> +#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];

Since you don't really mean [1] here, can you avoid writing it this way?
Variable length arrays are okay everywhere except in public headers.

> @@ -211,14 +197,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) ||
> +         (msr_index == MSR_IA32_DS_AREA) ||
> +         (msr_index == MSR_IA32_PEBS_ENABLE) )

Can I talk you into coding constructs like this with switch() rather than
a long (and potentially growing) if() condition?

> @@ -682,7 +643,7 @@ static void core2_vpmu_do_cpuid(unsigned int input,
>  static void core2_vpmu_dump(const struct vcpu *v)
>  {
>      const struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    int i, num;
> +    int i;

Please use the occasion and make such "unsigned int" at once.

Jan


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