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

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

On 08/11/2014 12:15 PM, Boris Ostrovsky wrote:
> On 08/11/2014 10:08 AM, Jan Beulich wrote:
>>>>> On 08.08.14 at 18:55, <boris.ostrovsky@oracle.com> 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[];
>> };
>
> 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 also considered introducing pmu_reg_t (typedef'd to uint64_t) but we'd 
be back again to rd/wrmsr() -- even though compiler may not flag it this 
still would be rather unnatural.

-boris

>
>>
>> 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?
>
> Not in BUILD_BUG_ON but I should check whether I have enough space 
> when computing ctxt->{ctrls | counters}.
>
> -boris
>