[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v12 for-xen-4.5 09/20] x86/VPMU: Add public xenpmu.h

On 09/29/2014 10:17 AM, Jan Beulich wrote:
On 25.09.14 at 21:28, <boris.ostrovsky@xxxxxxxxxx> 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, \
-#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;
+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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.