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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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, \
> >                                             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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel