Re: [Xen-devel] [PATCH v14 for-xen-4.5 11/21] x86/VPMU: Interface for setting PMU mode and flags

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.10.14 at 23:17, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -21,6 +21,8 @@
>  #include <xen/config.h>
>  #include <xen/sched.h>
>  #include <xen/xenoprof.h>
> +#include <xen/event.h>
> +#include <xen/guest_access.h>
>  #include <asm/regs.h>
>  #include <asm/types.h>
>  #include <asm/msr.h>
> @@ -32,8 +34,11 @@
>  #include <asm/hvm/svm/vmcb.h>
>  #include <asm/apic.h>
>  #include <public/pmu.h>
> +#include <xen/tasklet.h>
> +#include <xsm/xsm.h>

Please put the xen/ one alongside the other xen/ ones.

> +static int vpmu_force_context_switch(void)
> +{
> +    unsigned i, numcpus, mycpu;
> +    static s_time_t start;
> +    struct vcpu *curr_vcpu = current;
> +    static DEFINE_PER_CPU(struct tasklet *, sync_task);
> +    int ret = 0;
> +
> +    numcpus = num_online_cpus();

I think you'd be better off counting these as you schedule the tasklets.

> +    mycpu = smp_processor_id();
> +
> +    if ( sync_vcpu != NULL ) /* if set, we may be in hypercall continuation 
> */
> +    {
> +        if (sync_vcpu != curr_vcpu )

Coding style.

> +            /* We are not the original caller */
> +            return -EAGAIN;

That would seem to be the wrong return value then. Also, the HAP
side fix for XSA-97 taught us that identifying a caller by vCPU is
problematic - in the course of the retries the kernel's scheduler
may move the calling process to a different vCPU, yet it's still the
legitimate original caller.

> +        goto cont_wait;
> +    }
> +
> +    for_each_online_cpu ( i )
> +    {
> +        if ( i == mycpu )
> +            continue;
> +
> +        per_cpu(sync_task, i) = xmalloc(struct tasklet);
> +        if ( per_cpu(sync_task, i) == NULL )
> +        {
> +            printk(XENLOG_WARNING "vpmu_force_context_switch: out of 
> memory\n");
> +            ret = -ENOMEM;
> +            goto out;
> +        }
> +        tasklet_init(per_cpu(sync_task, i), vpmu_sched_checkin, 0);
> +    }
> +
> +    /* First count is for self */
> +    atomic_set(&vpmu_sched_counter, 1);
> +
> +    for_each_online_cpu ( i )
> +    {
> +        if ( i != mycpu )
> +            tasklet_schedule_on_cpu(per_cpu(sync_task, i), i);
> +    }
> +
> +    vpmu_save(current);
> +
> +    sync_vcpu = curr_vcpu;
> +    start = NOW();
> +
> + cont_wait:
> +    /*
> +     * Note that we may fail here if a CPU is hot-plugged while we are
> +     * waiting. We will then time out.
> +     */

And I continue to miss the handling of the hot-unplug case (or at the
very least a note on this being unimplemented [and going to crash],
to at least clarify matters to the curious reader).

> +    while ( atomic_read(&vpmu_sched_counter) != numcpus )
> +    {
> +        s_time_t now;
> +
> +        cpu_relax();
> +
> +        now = NOW();
> +
> +        /* Give up after (arbitrarily chosen) 5 seconds */
> +        if ( now > start + SECONDS(5) )
> +        {
> +            printk(XENLOG_WARNING
> +                   "vpmu_force_context_switch: failed to sync\n");
> +            ret = -EBUSY;
> +            break;
> +        }
> +
> +        if ( hypercall_preempt_check() )
> +            return hypercall_create_continuation(
> +                __HYPERVISOR_xenpmu_op, "i", XENPMU_mode_set);

Did you test this code path? I don't see how with the missing second
hypercall argument the continuation could reliably succeed.

> +    case XENPMU_mode_set:
> +    {
> +        uint32_t current_mode;
> +
> +        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 is someone else is in the middle of changing mode ---
> +         * this is most likely indication of two system administrators
> +         * working against each other.
> +         * If continuation from vpmu_force_context_switch() is still pending
> +         * we can proceed here without getting the lock:
> +         * vpmu_force_context_switch() will check whether we are the vcpu 
> that
> +         * initialted it.
> +         */
> +        if ( (sync_vcpu == NULL) && !spin_trylock(&xenpmu_mode_lock) )
> +            return -EAGAIN;

Continuing without holding the lock when sync_vcpu != NULL is
bogus: What if sync_vcpu got cleared by the time
vpmu_force_context_switch() gets around to look at it?

> +
> +        current_mode = vpmu_mode;
> +        vpmu_mode = pmu_params.val;
> +
> +        if ( vpmu_mode == XENPMU_MODE_OFF )
> +        {
> +            /*
> +             * Make sure all (non-dom0) VCPUs have unloaded their VPMUs. This
> +             * can be achieved by having all physical processors go through
> +             * context_switch().
> +             */
> +            ret = vpmu_force_context_switch();
> +            if ( ret )
> +                vpmu_mode = current_mode;
> +        }
> +
> +        if ( spin_is_locked(&xenpmu_mode_lock) )
> +            spin_unlock(&xenpmu_mode_lock);

spin_is_locked() only tells you that _someone_ holds the lock - that
isn't necessarily the current vCPU.

> +        break;
> +    }
> +
> +    case XENPMU_mode_get:
> +        /* See whether we are in the middle of mode change */

Please make your comments conform to ./CODING_STYLE.

> +        if ( (sync_vcpu != NULL) || !spin_trylock(&xenpmu_mode_lock) )
> +            return -EAGAIN;
> +
> +        memset(&pmu_params, 0, sizeof(pmu_params));
> +        pmu_params.val = vpmu_mode;
> +        spin_unlock(&xenpmu_mode_lock);

Is it really meaningful to do this under lock? By the time the caller
gets to see it it's stale anyway.

> @@ -116,5 +109,21 @@ void vpmu_dump(struct vcpu *v);
>  extern int acquire_pmu_ownership(int pmu_ownership);
>  extern void release_pmu_ownership(int pmu_ownership);
>  
> +extern uint64_t vpmu_mode;
> +extern uint64_t vpmu_features;

Any particular reason for these to not be unsigned int?

Jan

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