From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v11 for-xen-4.5 02/20] x86/VPMU: Manage VPMU_CONTEXT_SAVE flag in vpmu_save_force() Date: Tue, 23 Sep 2014 11:06:42 -0400 Message-ID: <54218C82.1030407@oracle.com> References: <1411430281-6132-1-git-send-email-boris.ostrovsky@oracle.com> <1411430281-6132-3-git-send-email-boris.ostrovsky@oracle.com> <20140923144438.GE3007@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140923144438.GE3007@laptop.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: kevin.tian@intel.com, keir@xen.org, jbeulich@suse.com, jun.nakajima@intel.com, andrew.cooper3@citrix.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org 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(). > >> 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). -boris > >> Signed-off-by: Boris Ostrovsky >> --- >> 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