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 10:57:33 -0400 Message-ID: <5429735D.9040008@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54298632020000780003A972@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 10:17 AM, Jan Beulich wrote: >>>> On 25.09.14 at 21:28, wrote: >> @@ -385,7 +389,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); >> if ( !ctxt ) >> { >> gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, " >> @@ -394,7 +399,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; > Is using the compile time count really necessary? I.e. is the runtime > limit (which hopefully is going to be lower) not possible here? If not, > why is doing so on the VMX side possible? I can use runtime value. > >> @@ -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. > >> #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. > >> --- a/xen/include/public/arch-x86/xen-x86_64.h >> +++ b/xen/include/public/arch-x86/xen-x86_64.h >> @@ -174,6 +174,14 @@ struct cpu_user_regs { >> typedef struct cpu_user_regs cpu_user_regs_t; >> DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t); >> >> +struct xen_pmu_regs { >> + __DECL_REG(ip); >> + __DECL_REG(sp); > Do you really need __DECL_REG() here? I.e. can't these two fields > be just xen_ulong_t e[is]p and the structure definition then be > shared with 32-bit code (and hence moved altogether into pmu.h)? I wasn't sure which way to go. The reason I did it this way was because I was essentially following cpu_user_regs() implementation. -boris