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

["Jan Beulich" <jbeulich@suse.com>]

>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 08/18/14 6:02 PM >>>
>On 08/11/2014 12:15 PM, Boris Ostrovsky wrote:
>> On 08/11/2014 10:08 AM, Jan Beulich wrote:
>>>> +    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[];
>>> };
>>
>> OK, this should work.
>
>Actually, no, it won't: we decided early on that register banks should 
>be outside the fixed portion of the structure. Keeping values[] inside 
>xen_pmu_amd_ctxt doesn't necessarily preclude this *if* it is never 
>referred to by code and is only used for determining the type. But that 
>would be an unreasonable requirement IMO.
>
>I understand the dislike to sizeof(uint64_t). OTOH, your original 
>concern was that we may forget to update size calculation if value 
>changes type sometime later. But the type cannot be anything but 
>uint64_t since it is used in rd/wrmsr().

I see; stay with the explicit sizeof(uint64_t) here then, but please try
to use sizeof(variable-or-field) elsewhere when possible.

Jan