From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v12 for-xen-4.5 09/20] x86/VPMU: Add public xenpmu.h Date: Mon, 29 Sep 2014 11:56:21 -0400 Message-ID: <54298125.5090405@oracle.com> References: <1411673336-32736-1-git-send-email-boris.ostrovsky@oracle.com> <1411673336-32736-10-git-send-email-boris.ostrovsky@oracle.com> <54298632020000780003A972@mail.emea.novell.com> <5429735D.9040008@oracle.com> <54299981020000780003AA93@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: <54299981020000780003AA93@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, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, jun.nakajima@intel.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 09/29/2014 11:40 AM, Jan Beulich wrote: >>>> On 29.09.14 at 16:57, wrote: >> On 09/29/2014 10:17 AM, Jan Beulich wrote: >>>>>> On 25.09.14 at 21:28, wrote: >>>> @@ -228,6 +229,11 @@ 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); >>>> + BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ); >>>> + BUILD_BUG_ON(sizeof(struct compat_pmu_regs) > XENPMU_REGS_PAD_SZ); >>> I'm having trouble finding where struct compat_pmu_regs gets defined >>> (largely since you're not adding anything to xen/include/xlat.h). >> >> It is generated into include/compat/arch-x86/xen-x86_32.h which is >> included via xen.h. And so is xlat.h -- but I didn't think I needed to >> add anything there (via xlat.lst) since I didn't see any reason for >> checking or translating it. > Right. In which case the check above is a bit pointless, and > possibly confusing (just like it happened to me now). If you > need the struct in a subsequent patch, move the check there. > If you don't ever need it till the end of your series, please > annotate the above stating that this is just for completeness > despite the hypervisor itself never using the structure. Hypervisor does use but in a later patch (#16, interrupt handling). I'll move the check there. >>>> #define vpmu_vcpu(vpmu) (container_of((vpmu), struct vcpu, \ >>>> arch.hvm_vcpu.vpmu)) >>>> -#define vpmu_domain(vpmu) (vpmu_vcpu(vpmu)->domain) >>> Is this really useful to delete, i.e. are absolutely sure that no future >>> use will ever arise? >> We never use it (and never had, apparently) so I didn't see any reason >> to carry it forward. > Oh, if it was never used, then the change is unrelated here - I was > taking it being here as a sign that the patch replaced the last > existing user. I'll add a comment about removal of this macro into commit message. -boris