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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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



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