[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

133     if ( vpmu->arch_vpmu_ops )
134         (void)vpmu->arch_vpmu_ops->arch_vpmu_save(v);
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) )

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


Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
  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) )
+ vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
      if ( vpmu->arch_vpmu_ops )
@@ -178,7 +180,6 @@ void vpmu_load(struct vcpu *v)
          if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
-            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
                               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_reset(vpmu, VPMU_CONTEXT_LOADED);

Xen-devel mailing list

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.