Re: [Xen-devel] [PATCH v14 for-xen-4.5 09/21] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.10.14 at 23:17, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -222,6 +215,12 @@ static int is_core2_vpmu_msr(u32 msr_index, int *type, 
> int *index)
>      }
>  }
>  
> +static inline int msraddr_to_bitpos(int x)
> +{
> +    ASSERT(x == (x & 0x1fff));
> +    return x;
> +}

Now this is interesting: I indeed asked you to fix this while you move
it, but (a) should such a fix be mentioned in the commit message and
(b) even more so when you outright drop support for the other half
of the possible MSR ranges.

> @@ -367,12 +373,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(*core2_vpmu_cxt) +

Okay, you switched this one as I had asked for. But you don't
really expect that I repeat this comment in various places, do you?
I.e. I would have expected you to fix this uniformly, not just here.

> +                                   sizeof(uint64_t) * fixed_pmc_cnt +
> +                                   sizeof(struct xen_pmu_cntr_pair) *
> +                                   arch_pmc_cnt);
> +    p = xzalloc(uint64_t);
> +    if ( !core2_vpmu_cxt || !p )
>          goto out_err;
>  
> -    vpmu->context = (void *)core2_vpmu_cxt;
> +    core2_vpmu_cxt->fixed_counters = sizeof(struct xen_pmu_intel_ctxt);

I.e. here as well as in the AMD code as well as wherever else.

Jan


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