Re: [PATCH v11 for-xen-4.5 02/20] x86/VPMU: Manage VPMU_CONTEXT_SAVE flag in vpmu_save_force()

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

On 09/23/2014 10:44 AM, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 22, 2014 at 07:57:43PM -0400, Boris Ostrovsky wrote:
>> vpmu_load() leaves VPMU_CONTEXT_SAVE set after calling vpmu_save_force() in
> The vpmu_save_force clears the VPMU_CONTEXT_SAVE after running the
> arch_vpmu_save:
>
> 133     if ( vpmu->arch_vpmu_ops )
> 134         (void)vpmu->arch_vpmu_ops->arch_vpmu_save(v);
> 135
> 136     vpmu_reset(vpmu, VPMU_CONTEXT_SAVE);
>
> So the VPMU_CONTEXT_SAVE does get cleared.

Yes, but right above this code fragment is

     if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
             return;

so you may not reach vpmu_reset().


>
>> vpmu_load(). This will break amd_vpmu_save() which expects this flag to be set
>> only when counters are already stopped.
>>
>> Since setting this flag is currently always done prior to calling
>> vpmu_save_force() let's both set and clear it there.
> I can see the merit of moving the dual vpmu_set to the vpmu_save_force
> but I must say I am not understanding the 'break ..' explanation above.

There is a possibility that we set VPMU_CONTEXT_SAVE on VPMU context and 
never clear it (because of what I mentioned above).

amd_vpmu_save() assumes that if VPMU_CONTEXT_SAVE is set then (1) we 
need to save counters and (2) we don't need to "stop" control registers 
since they must have been stopped earlier. It's (2) that causes all 
sorts of problem (like counters still running in wrong guest and 
hypervisor sending to that guest unexpected PMU interrupts).

(That, of course, is beside the fact that logically VPMU_CONTEXT_SAVE 
should be manipulated in vpmu_save_force(). As you pointed out too).

-boris



>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   xen/arch/x86/hvm/vpmu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
>> index 15d5b6f..451b346 100644
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -130,6 +130,8 @@ static void vpmu_save_force(void *arg)
>>       if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>>           return;
>>   
>> +    vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
>> +
>>       if ( vpmu->arch_vpmu_ops )
>>           (void)vpmu->arch_vpmu_ops->arch_vpmu_save(v);
>>   
>> @@ -178,7 +180,6 @@ void vpmu_load(struct vcpu *v)
>>            */
>>           if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>>           {
>> -            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);
>> @@ -195,7 +196,6 @@ void vpmu_load(struct vcpu *v)
>>           vpmu = vcpu_vpmu(prev);
>>   
>>           /* Someone ran here before us */
>> -        vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
>>           vpmu_save_force(prev);
>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>   
>> -- 
>> 1.8.1.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel