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

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

On 10/27/2014 12:24 PM, Jan Beulich wrote:
>>>> On 17.10.14 at 23:17, <boris.ostrovsky@oracle.com> wrote:
>>
>> +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.

If the process is rescheduled then we will time out operation.

I suppose I can set a bit in the argument's val to mark that particular 
argument as pending a continuation completion (I don't think we need to 
worry about malicious domain here since this is a privileged operation).


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

Where would we crash? I have no interest in that.


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

I did test it (and retested it now) and it works. I guess it may be 
picking the same value from the stack during continuation which is why 
it does not fail.

-boris