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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/11/2014 06:03 AM, Jan Beulich wrote:
>>>> On 06.06.14 at 19:40, <boris.ostrovsky@oracle.com> 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?

Relative to context union of xen_arch_pmu (i.e. xen_pmu_amd_ctxt). I'll 
add a comment.

>
>> +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)?

128 is for fixed part only (which is currently 64 bytes Intel, which is 
larger). Will add a comment.

-boris