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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.01.14 at 16:20, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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.

I don't really care what you do here, so long as you don't ignore
data passed into the hypercall.

Jan


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