Re: [PATCH v12 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 Thu, Sep 25, 2014 at 03:28:38PM -0400, Boris Ostrovsky wrote:
> There is a possibility that we set VPMU_CONTEXT_SAVE on VPMU context in
> vpmu_load() and never clear it (because vpmu_save_force() will see
> VPMU_CONTEXT_LOADED bit clear, which is possible on AMD processors)
> 
> The problem is that 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. The latter may cause all
> sorts of problem (like counters still running in a wrong guest and hypervisor
> sending to that guest unexpected PMU interrupts).
> 
> Since setting this flag is currently always done prior to calling
> vpmu_save_force() let's both set and clear it there.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@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