From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v9 09/20] x86/VPMU: Add public xenpmu.h
Date: Mon, 11 Aug 2014 12:15:09 -0400
Message-ID: <53E8EC0D.8080800@oracle.com>
References: <1407516946-17833-1-git-send-email-boris.ostrovsky@oracle.com>	<1407516946-17833-10-git-send-email-boris.ostrovsky@oracle.com>
	<53E8EA9A020000780002B35C@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <53E8EA9A020000780002B35C@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: kevin.tian@intel.com, keir@xen.org, jun.nakajima@intel.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, suravee.suthikulpanit@amd.com
List-Id: xen-devel@lists.xenproject.org

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.

>
> 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