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 11:40 AM, Jan Beulich wrote:
> > > > On 29.09.14 at 16:57, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 09/29/2014 10:17 AM, Jan Beulich wrote:
> > > > > > On 25.09.14 at 21:28, <boris.ostrovsky@xxxxxxxxxx> 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


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