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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 01/30/2015 08:31 AM, Jan Beulich wrote:
>>>> On 05.01.15 at 22:44, <boris.ostrovsky@oracle.com> 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?

No, I should clear it here as well.

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

If we decide to start using this filed then presumably we will have to 
bump interface version. If pad field becomes something else it will only 
be considered when minor version is > 1. (And I should probably add 
major version check.)

-boris