From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v7 09/19] x86/VPMU: Add public xenpmu.h Date: Wed, 11 Jun 2014 08:32:30 -0400 Message-ID: <53984C5E.30406@oracle.com> References: <1402076415-26475-1-git-send-email-boris.ostrovsky@oracle.com> <1402076415-26475-10-git-send-email-boris.ostrovsky@oracle.com> <539845A40200007800019D2B@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <539845A40200007800019D2B@mail.emea.novell.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, suravee.suthikulpanit@amd.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 06/11/2014 06:03 AM, Jan Beulich wrote: >>>> On 06.06.14 at 19:40, 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