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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.

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

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