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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.14 at 18:13, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 01/24/2014 10:10 AM, Jan Beulich wrote:
>>>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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.

>>> +/* 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...)

At which time you'd need to add something like a set-version sub-op,
which would then also be the time to make this as IN/OUT. Right now
it is just IN and hence should be marked as such.

Jan


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