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 10:17 AM, Jan Beulich wrote:
>>>> On 25.09.14 at 21:28, <boris.ostrovsky@oracle.com> 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