Re: [Xen-devel] [PATCH v17 13/23] x86/VPMU: Interface for setting PMU mode and flags

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.01.15 at 22:44, <boris.ostrovsky@xxxxxxxxxx> wrote:
> +static long vpmu_unload_next(void *arg)
> +{
> +    struct vcpu *last;
> +    int ret;
> +    unsigned int thiscpu = smp_processor_id();
> +
> +    if ( thiscpu != vpmu_next_unload_cpu )
> +    {
> +        /* Continuation thread may have been moved due to CPU hot-unplug */
> +        vpmu_mode = (unsigned long)arg;
> +        vpmu_first_unload_cpu = VPMU_INVALID_CPU;
> +        return -EAGAIN;
> +    }
> +
> +    local_irq_disable(); /* so that last_vcpu doesn't change under us. */
> +
> +    last = this_cpu(last_vcpu);
> +    if ( last )
> +    {
> +        vpmu_unload(vcpu_vpmu(last));
> +        this_cpu(last_vcpu) = NULL;
> +    }

So you do this for last_vcpu here, but ...


> +static int vpmu_unload_all(unsigned long old_mode)
> +{
> +    int ret = 0;
> +
> +    vpmu_unload(vcpu_vpmu(current));

... for current here, also without clearing this_cpu(last_vcpu). Is
that really correct?

> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
> +{
> +    int ret;
> +    struct xen_pmu_params pmu_params;
> +
> +    ret = xsm_pmu_op(XSM_OTHER, current->domain, op);
> +    if ( ret )
> +        return ret;
> +
> +    switch ( op )
> +    {
> +    case XENPMU_mode_set:
> +    {
> +        unsigned int old_mode;
> +        static DEFINE_SPINLOCK(xenpmu_mode_lock);
> +
> +        if ( copy_from_guest(&pmu_params, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( pmu_params.val & ~(XENPMU_MODE_SELF | XENPMU_MODE_HV) )
> +            return -EINVAL;
> +
> +        /* 32-bit dom0 can only sample itself. */
> +        if ( is_pv_32bit_vcpu(current) && (pmu_params.val & XENPMU_MODE_HV) )
> +            return -EINVAL;
> +
> +        /*
> +         * Return error if someone else is in the middle of changing mode ---
> +         * this is most likely indication of two system administrators
> +         * working against each other.
> +         */
> +        if ( !spin_trylock(&xenpmu_mode_lock) )
> +            return -EAGAIN;
> +        if ( vpmu_first_unload_cpu != VPMU_INVALID_CPU )
> +        {
> +            spin_unlock(&xenpmu_mode_lock);
> +            return -EAGAIN;
> +        }
> +
> +        old_mode = vpmu_mode;
> +        vpmu_mode = pmu_params.val;
> +
> +        if ( vpmu_mode == XENPMU_MODE_OFF )
> +        {
> +            /* Make sure all (non-dom0) VCPUs have unloaded their VPMUs. */
> +            ret = vpmu_unload_all(old_mode);
> +            if ( ret )
> +                vpmu_mode = old_mode;
> +        }
> +
> +        spin_unlock(&xenpmu_mode_lock);
> +
> +        break;
> +    }
> +
> +    case XENPMU_mode_get:
> +        memset(&pmu_params, 0, sizeof(pmu_params));
> +        pmu_params.val = vpmu_mode;
> +
> +        pmu_params.version.maj = XENPMU_VER_MAJ;
> +        pmu_params.version.min = XENPMU_VER_MIN;
> +
> +        if ( copy_to_guest(arg, &pmu_params, 1) )
> +            return -EFAULT;
> +        break;
> +
> +    case XENPMU_feature_set:
> +        if ( copy_from_guest(&pmu_params, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( pmu_params.val & ~XENPMU_FEATURE_INTEL_BTS )
> +            return -EINVAL;
> +
> +        vpmu_features = pmu_params.val;
> +        break;
> +
> +    case XENPMU_feature_get:
> +        pmu_params.val = vpmu_features;
> +        if ( copy_field_to_guest(arg, &pmu_params, val) )
> +            return -EFAULT;
> +        break;
> +
> +    default:
> +        ret = -EINVAL;
> +    }
> +
> +    return ret;
> +}

Throughout this function, the "pad" field isn't being checked to be
zero (and XENPMU_feature_get also doesn't clear it, but that seems
secondary, at least as long as it's being declared "IN" only). As I'm
sure I said before - we ought to do this in order to be able to assign
meaning to the field later on. Perhaps it would even better be
renamed to e.g. "mbz".

> @@ -144,6 +145,9 @@ do_tmem_op(
>  extern long
>  do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
>  
> +extern long
> +do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg);

Despite there being numerous bad examples - can "op" please be
"unsigned int"?

> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -173,6 +173,7 @@ struct xsm_operations {
>      int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq 
> *bind);
>      int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, 
> uint8_t allow);
>      int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t 
> allow);
> +    int (*pmu_op) (struct domain *d, int op);

And then here too?

Jan

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