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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> +/* Use private context as a flag for MSR bitmap */
> +#define msr_bitmap_on(vpmu)    { (vpmu)->priv_context = (void *)-1L; }
> +#define msr_bitmap_off(vpmu)   { (vpmu)->priv_context = NULL; }

To avoid surprises, please use either an expression without braces
and without semicolon, ({ ... }), or traditional do/while(0) for macros
like this.

> @@ -382,7 +382,8 @@ static int amd_vpmu_initialise(struct vcpu *v)
>        }
>      }
>  
> -    ctxt = xzalloc(struct amd_vpmu_context);
> +    ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) +
> +                         2 * sizeof(uint64_t) * AMD_MAX_COUNTERS);

Both here, ...

> @@ -391,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;

... here, and elsewhere I'd really like to ask you to avoid using
sizeof on a type when you have a suitable variable in place. I
suppose here you really mean sizeof(*ctxt->counters). Doing it
this way avoids needing (and perhaps forgetting) to touch this
code if the array element type changes (since you have no way
to reasonably grep for this especially when the type is a rather
generic one).

> @@ -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));

Why not xzalloc(uint64_t)?

Jan


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