From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v3 07/16] x86/VPMU: Add public xenpmu.h Date: Mon, 13 Jan 2014 10:23:35 -0500 Message-ID: <52D404F7.9090906@oracle.com> References: <1389036295-3877-1-git-send-email-boris.ostrovsky@oracle.com> <1389036295-3877-8-git-send-email-boris.ostrovsky@oracle.com> <52D3FAB20200007800113162@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W2jMB-0008Uv-KN for xen-devel@lists.xenproject.org; Mon, 13 Jan 2014 15:23:15 +0000 In-Reply-To: <52D3FAB20200007800113162@nat28.tlf.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: keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, dietmar.hahn@ts.fujitsu.com, jun.nakajima@intel.com, xen-devel List-Id: xen-devel@lists.xenproject.org On 01/13/2014 08:39 AM, Jan Beulich wrote: >> --- /dev/null >> +++ b/xen/include/public/arch-x86/xenpmu.h >> @@ -0,0 +1,74 @@ .... >> + >> +/* AMD PMU registers and structures */ >> +struct amd_vpmu_context { >> + uint64_t counters; /* Offset to counter MSRs */ >> + uint64_t ctrls; /* Offset to control MSRs */ >> + uint8_t msr_bitmap_set; /* Used by HVM only */ >> +}; > sizeof() of this structure will differ between 32- and 64-bit guests - > are you intending to do the necessary translation even though it > seems rather easy to avoid having to do so? 'msr_bitmap_set' field is actually never used by PV and it's the last one in the structure which is why I didn't bother to make it bigger. But you are right, I should fix this to avoid problems in the future. > >> + >> +/* Intel PMU registers and structures */ >> +struct arch_cntr_pair { >> + uint64_t counter; >> + uint64_t control; >> +}; >> +struct core2_vpmu_context { > Blank line missing between the two structures. > >> + 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; >> + uint64_t fixed_counters; /* Offset to fixed counter MSRs */ >> + uint64_t arch_counters; /* Offset to architectural counter MSRs */ >> +}; >> + >> +/* ANSI-C does not support anonymous unions */ >> +#if !defined(__GNUC__) || defined(__STRICT_ANSI__) >> +#define __ANON_UNION_NAME(x) x >> +#else >> +#define __ANON_UNION_NAME(x) >> +#endif > Why? And if really needed, why here? I'll drop this. I thought anonymous unions looked better but now that I look at it again I think the ifdefs are rather ugly too. > >> + >> +#define XENPMU_MAX_CTXT_SZ (sizeof(struct amd_vpmu_context) > \ >> + sizeof(struct core2_vpmu_context) ? \ >> + sizeof(struct amd_vpmu_context) : \ >> + sizeof(struct core2_vpmu_context)) >> +#define XENPMU_CTXT_PAD_SZ (((XENPMU_MAX_CTXT_SZ + 64) & ~63) + 128) >> +struct arch_xenpmu { >> + union { >> + struct cpu_user_regs regs; >> + uint8_t pad1[256]; >> + } __ANON_UNION_NAME(r); >> + union { >> + uint32_t lapic_lvtpc; >> + uint64_t pad2; >> + } __ANON_UNION_NAME(l); >> + union { >> + struct amd_vpmu_context amd; >> + struct core2_vpmu_context intel; >> + uint8_t pad3[XENPMU_CTXT_PAD_SZ]; >> + } __ANON_UNION_NAME(c); > I don't think there's be a severe problem if you simply always > had names on these unions. > >> +}; >> +typedef struct arch_xenpmu arch_xenpmu_t; > Overall you should also prefix all types added to global scope with > "xen". I know this wasn't done consistently for older headers, but > we shouldn't be extending this name space cluttering. You mean something like arch_xenpmu ==> xen_arch_pmu ? -boris