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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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