From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v9 09/20] x86/VPMU: Add public xenpmu.h Date: Mon, 18 Aug 2014 12:02:40 -0400 Message-ID: <53F223A0.7000906@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> <53E8EC0D.8080800@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E8EC0D.8080800@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich 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 12:15 PM, Boris Ostrovsky wrote: > On 08/11/2014 10:08 AM, Jan Beulich wrote: >>>>> On 08.08.14 at 18:55, 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 >