Re: [PATCH v9 02/20] x86/VPMU: Manage VPMU_CONTEXT_SAVE flag in vpmu_save_force()

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

On 08/11/2014 09:28 AM, Jan Beulich wrote:
>>>> On 08.08.14 at 18:55, <boris.ostrovsky@oracle.com> wrote:
>> vpmu_load() may leave VPMU_CONTEXT_SAVE set after calling vpmu_save_force() if
>> the context is not loaded.
> To me this is ambiguous (and looking at the patch I can't really
> resolve it): Do you mean to say this can validly happen, or that it
> could have happened in error prior to the patch? In any event
> please alter the description to either clearly state what the wrong
> old behavior was, or what the correct new behavior is to be.

What I was trying to say (not particularly successfully, apparently) is 
that currently (i.e. prior to the patch) we may end up not clearing 
VPMU_CONTEXT_SAVE flag after calling vpmu_save_forced(). And that would 
be a bug, which this patch is fixing.

(It doesn't help that the diff doesn't show that the flag is cleared 
right after ->arch_vpmu_save() below)

-boris

>
> Jan
>
>> 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 63765fa..26e971b 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
>
>