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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +
> +        vpmu_mode &= ~XENPMU_MODE_MASK;
> +        vpmu_mode |= mode;
> +
> +        ret = 0;
> +        break;
> +
> +    case XENPMU_mode_get:
> +        pmu_params.d.val = vpmu_mode & XENPMU_MODE_MASK;
> +        pmu_params.v.version.maj = XENPMU_VER_MAJ;
> +        pmu_params.v.version.min = XENPMU_VER_MIN;
> +        if ( copy_to_guest(arg, &pmu_params, 1) )

__copy_to_guest().

> +            return -EFAULT;
> +        ret = 0;
> +        break;
> +
> +    case XENPMU_feature_set:
> +        if ( !is_control_domain(current->domain) )
> +            return -EPERM;
> +
> +        if ( copy_from_guest(&pmu_params, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( (uint32_t)pmu_params.d.val & ~XENPMU_FEATURE_INTEL_BTS )
> +            return -EINVAL;

See above.

> +
> +        vpmu_mode &= ~XENPMU_FEATURE_MASK;
> +        vpmu_mode |= (uint32_t)pmu_params.d.val << XENPMU_FEATURE_SHIFT;
> +
> +        ret = 0;
> +        break;
> +
> +    case XENPMU_feature_get:
> +        pmu_params.d.val = vpmu_mode & XENPMU_FEATURE_MASK;
> +        if ( copy_to_guest(arg, &pmu_params, 1) )

See above.

> --- 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.

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

Jan


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