From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v4 07/17] x86/VPMU: Add public xenpmu.h Date: Fri, 24 Jan 2014 11:49:12 -0500 Message-ID: <52E29988.2040503@oracle.com> References: <1390331342-3967-1-git-send-email-boris.ostrovsky@oracle.com> <1390331342-3967-8-git-send-email-boris.ostrovsky@oracle.com> <52E28CCC0200007800116B7D@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52E28CCC0200007800116B7D@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, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 01/24/2014 09:54 AM, Jan Beulich wrote: >>>> On 21.01.14 at 20:08, Boris Ostrovsky wrote: >> Add xenpmu.h header file, > To me, naming a public Xen header (other than the core one) xen*.h > is redundant. There's no information lost if you just called it pmu.h. I was trying to keep filename and top-level data structures the same (although now that I changed xenpmu_ prefix to xen_pmu_ they no longer are). > > Also I think you ought to use plural here. I'd prefer to keep the arch-independent and -dependent file names the same. ... > >> +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; >> + uint64_t fixed_counters; /* Offset to fixed counter MSRs */ >> + uint64_t arch_counters; /* Offset to architectural counter MSRs */ > I think these last two could easily be uint32_t. > >> +/* Shared between hypervisor and PV domain */ >> +struct xen_pmu_data { >> + uint32_t domain_id; >> + uint32_t vcpu_id; >> + uint32_t pcpu_id; >> + uint32_t pmu_flags; >> + >> + xen_arch_pmu_t pmu; >> +}; > So if this got included by an architecture independent source file > on ARM, how would this build? You at least need a stub definition > there for xen_arch_pmu_t afaict (if already give the impression - > further up - that you're supporting ARM compilation of this header). I was supposed to have an entry in arch-arm.h but dropped it somewhere along the way. I'll put it back. -boris