Re: [PATCH v4 09/17] x86/VPMU: Interface for setting PMU mode and flags

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

On 01/24/2014 10:10 AM, Jan Beulich wrote:
>>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    int ret = -EINVAL;
>> +    xen_pmu_params_t pmu_params;
>> +    uint32_t mode;
>> +
>> +    switch ( op )
>> +    {
>> +    case XENPMU_mode_set:
>> +        if ( !is_control_domain(current->domain) )
>> +            return -EPERM;
>> +
>> +        if ( copy_from_guest(&pmu_params, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        mode = (uint32_t)pmu_params.d.val & XENPMU_MODE_MASK;
>> +        if ( mode & ~XENPMU_MODE_ON )
>> +            return -EINVAL;
> Please, if you add a new interface, think carefully about future
> extension room: Here you ignore the upper 32 bits of .val instead
> of making sure they're zero, thus making it impossible to assign
> them some meaning later on.

I think I can leave this as is for now --- I am storing VPMU mode and 
VPMU features in the Xen-private vpmu_mode, which is a 64-bit value.

What I probably should do is remove XENPMU_MODE_MASK (and 
XENPMU_FEATURE_SHIFT  and XENPMU_FEATURE_MASK) from the public header 
since Linux passes down 64-bit pmu_params.d.val without any format 
assumptions anyway.

>
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -101,6 +101,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>>   #define __HYPERVISOR_kexec_op             37
>>   #define __HYPERVISOR_tmem_op              38
>>   #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
>> +#define __HYPERVISOR_xenpmu_op            40
>>   
>>   /* Architecture-specific hypercall definitions. */
>>   #define __HYPERVISOR_arch_0               48
> Are you certain this wouldn't better be an architecture-specific
> hypercall? Just like with Machine Check, I don't think all
> architectures are guaranteed to have (or ever get) performance
> monitoring capabilities.

An architecture doesn't necessarily need to have HW performance 
monitoring support. In principle this interface can be used for passing 
any performance-related data (e.g. collected by the hypervisor) to the 
guest.

>> +/* Parameters structure for HYPERVISOR_xenpmu_op call */
>> +struct xen_pmu_params {
>> +    /* IN/OUT parameters */
>> +    union {
>> +        struct version {
>> +            uint8_t maj;
>> +            uint8_t min;
>> +        } version;
>> +        uint64_t pad;
>> +    } v;
> Looking at the implementation above I don't see this ever being an
> IN parameter.

Currently Xen doesn't care about version but in the future a guest may 
specify what version of PMU it wants to use (I hope this day will never 
come though...)

-boris