[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 06.06.14 at 19:40, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -228,6 +229,10 @@ 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) ||
> +                 (sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ));

This should be two BUILD_BUG_ON()s, so that if one triggers there's
no ambiguity in which of the structures grew too large.

> +struct xen_pmu_amd_ctxt {
> +    uint32_t counters;       /* Offset to counter MSRs */
> +    uint32_t ctrls;          /* Offset to control MSRs */

Offsets relative to what?

> +struct xen_pmu_intel_ctxt {
> +    uint64_t global_ctrl;
> +    uint64_t global_ovf_ctrl;
> +    uint64_t global_status;
> +    uint64_t fixed_ctrl;
> +    uint64_t ds_area;
> +    uint64_t pebs_enable;
> +    uint64_t debugctl;
> +    uint32_t fixed_counters;  /* Offset to fixed counter MSRs */
> +    uint32_t arch_counters;   /* Offset to architectural counter MSRs */

Same question here.

> +struct xen_arch_pmu {
> +    union {
> +        struct cpu_user_regs regs;
> +        uint8_t pad[256];
> +    } r;
> +    union {
> +        uint32_t lapic_lvtpc;
> +        uint64_t pad;
> +    } l;
> +    union {
> +        struct xen_pmu_amd_ctxt amd;
> +        struct xen_pmu_intel_ctxt intel;
> +#define XENPMU_CTXT_PAD_SZ  128
> +        uint8_t pad[XENPMU_CTXT_PAD_SZ];

So assuming the variable size MSR arrays follow here, is 128 enough
for forward compatibility? Or is 128 indeed only expected to cover the
fixed size base structures (whichever way it is should perhaps be
made explicit by way of adding a comment)?

Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.