From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v9 02/20] x86/VPMU: Manage VPMU_CONTEXT_SAVE flag in vpmu_save_force() Date: Mon, 11 Aug 2014 11:35:47 -0400 Message-ID: <53E8E2D3.6050008@oracle.com> References: <1407516946-17833-1-git-send-email-boris.ostrovsky@oracle.com> <1407516946-17833-3-git-send-email-boris.ostrovsky@oracle.com> <53E8E102020000780002B2E9@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E8E102020000780002B2E9@mail.emea.novell.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: Jan Beulich Cc: kevin.tian@intel.com, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 08/11/2014 09:28 AM, Jan Beulich wrote: >>>> On 08.08.14 at 18:55, wrote: >> vpmu_load() may leave VPMU_CONTEXT_SAVE set after calling vpmu_save_force() if >> the context is not loaded. > To me this is ambiguous (and looking at the patch I can't really > resolve it): Do you mean to say this can validly happen, or that it > could have happened in error prior to the patch? In any event > please alter the description to either clearly state what the wrong > old behavior was, or what the correct new behavior is to be. What I was trying to say (not particularly successfully, apparently) is that currently (i.e. prior to the patch) we may end up not clearing VPMU_CONTEXT_SAVE flag after calling vpmu_save_forced(). And that would be a bug, which this patch is fixing. (It doesn't help that the diff doesn't show that the flag is cleared right after ->arch_vpmu_save() below) -boris > > Jan > >> 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 63765fa..26e971b 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 > >