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 Tue, Sep 23, 2014 at 11:06:42AM -0400, Boris Ostrovsky wrote:
> 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().

Duh! Right.
> 
> 
> >
> >>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).

Could you kindly include this verbose description in the patch?

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