Re: [PATCH v7 11/19] x86/VPMU: Interface for setting PMU mode and flags

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

On 06/06/2014 03:58 PM, Andrew Cooper wrote:
> On 06/06/14 18:40, Boris Ostrovsky wrote:
>> Add runtime interface for setting PMU mode and flags. Three main modes are
>> provided:
>> * PMU off
>> * PMU on: Guests can access PMU MSRs and receive PMU interrupts. dom0
>>    profiles itself and the hypervisor.
>> * dom0-only PMU: dom0 collects samples for both itself and guests.
>>
>> For feature flags only Intel's BTS is currently supported.
>>
>> Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
>> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
>> ---
>>
>> @@ -275,3 +283,100 @@ void vpmu_dump(struct vcpu *v)
>>           vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
>>   }
>>   
>> +/* Unload VPMU contexts */
>> +static void vpmu_unload_all(void)
>> +{
>> +    struct domain *d;
>> +    struct vcpu *v;
>> +    struct vpmu_struct *vpmu;
>> +
>> +    for_each_domain( d )
>> +    {
>> +        for_each_vcpu ( d, v )
>> +        {
>> +            if ( v != current )
>> +                vcpu_pause(v);
>> +            vpmu = vcpu_vpmu(v);
>> +
>> +            if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>> +            {
>> +                if ( v != current )
>> +                    vcpu_unpause(v);
>> +                continue;
>> +            }
>> +
>> +            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
>> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
>> +                             vpmu_save_force, (void *)v, 1);
>> +            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>> +
>> +            if ( v != current )
>> +                vcpu_unpause(v);
>> +        }
>> +    }
>> +}
> This is a very long function without any preemption.
>
> To reduce its impact greatly, would it be better to have a global mode
> controlling vcpu context switching?  You could enable
> "VCPU_GLOBAL_DISABLING", then IPI all cpus at the same time to save
> their vpmu context.  Once the IPI is complete, all vpmu context is clean.
>
> This avoids a double loop and all the vcpu_pause/unpause()s.

I can see the problem but I am not sure what you are suggesting will 
work (or perhaps I don't quite understand what you are suggesting). If I 
IPI a processor may be in the middle of working on the VPMU context.

Let me think about this.


>
>> +
>> +
>> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    int ret = -EINVAL;
>> +    xen_pmu_params_t pmu_params;
> This is probably going to want to be XSM'd up for permissions?

I haven't looked at this since earlier versions of the patch but when I 
did it turned out we were out of bits in the access vector (or whatever 
that structure was). I in fact mentioned this problem in the cover 
letter for the series.

-boris