Re: [Xen-devel] [PATCH v9 09/20] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
>  static int amd_vpmu_initialise(struct vcpu *v)
>  {
> -    struct amd_vpmu_context *ctxt;
> +    struct xen_pmu_amd_ctxt *ctxt;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      uint8_t family = current_cpu_data.x86;
>  
> @@ -382,7 +386,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);

So I guess this is one of said sizeof()-s, and I continue to think this
would benefit from using a proper field. It is my understanding that
the real (non-C89 compliant) declaration of struct xen_pmu_amd_ctxt
would be

struct xen_pmu_amd_ctxt {
    /* Offsets to counter and control MSRs (relative to xen_arch_pmu.c.amd) */
    uint32_t counters;
    uint32_t ctrls;
    uint64_t values[];
};

Just like already done elsewhere you _can_ add variable-sized
arrays to the public interface as long as you guard them suitably.
And then your statement would become

    ctxt = xzalloc_bytes(sizeof(*ctxt) +
                         2 * sizeof(*ctxt->values) * AMD_MAX_COUNTERS);

making clear what the allocation is doing.

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

    ctxt->counters = sizeof(*ctxt);
    ctxt->ctrls = ctxt->counters + sizeof(*ctxt->values) * AMD_MAX_COUNTERS;

or

    ctxt->counters = offsetof(typeof(ctxt), values[0]);
    ctxt->ctrls = offsetof(typeof(ctxt), values[AMD_MAX_COUNTERS]);

> @@ -228,6 +229,9 @@ void vpmu_initialise(struct vcpu *v)
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      uint8_t vendor = current_cpu_data.x86_vendor;
>  
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);

Don't these need to include the variable size array portions too?

Jan


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