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

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

On 01/27/2014 03:34 AM, Jan Beulich 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.
> You should drop the cast to a 32-bit value at the very least -
> "leave this as is for now" reads like you don#t need to make
> any changes.

mode is stored in the lower 32 bits of vpmu_mode variable a few lines below

      vpmu_mode &= ~XENPMU_MODE_MASK; // XENPMU_MODE_MASK - 0xffffffff
      vpmu_mode |= mode;

so the cast needs to happen somewhere. I can move it to the line above 
although I
am not sure what difference that would make.

-boris