From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU Date: Wed, 17 Dec 2014 11:21:15 -0500 Message-ID: <20141217162115.GD6414@laptop.dumpdata.com> References: <1418830547-2218-1-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1418830547-2218-1-git-send-email-boris.ostrovsky@oracle.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: Boris Ostrovsky Cc: keir@xen.org, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, Dec 17, 2014 at 10:35:47AM -0500, Boris Ostrovsky wrote: > We need to make sure that last_vcpu is not pointing to VCPU whose > VPMU is being destroyed. Otherwise we may try to dereference it in > the future, when VCPU is gone. > > We have to do this atomically since otherwise there is a (somewhat > theoretical) chance that between test and subsequent clearing > of last_vcpu the remote processor (i.e. vpmu->last_pcpu) might do > both vpmu_load() and then vpmu_save() for another VCPU. The former > will clear last_vcpu and the latter will set it to something else. > > We should also check for VPMU_CONTEXT_ALLOCATED in vpmu_destroy() to > avoid unnecessary cmpxchg() and arch-specific destroy ops. Thus > checks in AMD and Intel routines are no longer needed. > > Signed-off-by: Boris Ostrovsky > --- > xen/arch/x86/hvm/svm/vpmu.c | 3 --- > xen/arch/x86/hvm/vmx/vpmu_core2.c | 2 -- > xen/arch/x86/hvm/vpmu.c | 7 +++++++ > 3 files changed, 7 insertions(+), 5 deletions(-) > > Changes in v3: > * Use cmpxchg instead of IPI > * Use correct routine nemas in commit message > * Remove duplicate test for VPMU_CONTEXT_ALLOCATED in arch-specific > destroy ops > > Changes in v2: > * Test last_vcpu locally before IPI > * Don't handle local pcpu as a special case --- on_selected_cpus > will take care of that > * Dont't cast variables unnecessarily > > diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c > index 8e07a98..4c448bb 100644 > --- a/xen/arch/x86/hvm/svm/vpmu.c > +++ b/xen/arch/x86/hvm/svm/vpmu.c > @@ -403,9 +403,6 @@ static void amd_vpmu_destroy(struct vcpu *v) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > - if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > - return; > - > if ( ((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set ) > amd_vpmu_unset_msr_bitmap(v); > > diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c > index 68b6272..590c2a9 100644 > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c > @@ -818,8 +818,6 @@ static void core2_vpmu_destroy(struct vcpu *v) > struct vpmu_struct *vpmu = vcpu_vpmu(v); > struct core2_vpmu_context *core2_vpmu_cxt = vpmu->context; > > - if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > - return; > xfree(core2_vpmu_cxt->pmu_enable); > xfree(vpmu->context); > if ( cpu_has_vmx_msr_bitmap ) > diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c > index 1df74c2..7cc95ae 100644 > --- a/xen/arch/x86/hvm/vpmu.c > +++ b/xen/arch/x86/hvm/vpmu.c > @@ -250,6 +250,13 @@ void vpmu_initialise(struct vcpu *v) > void vpmu_destroy(struct vcpu *v) > { > struct vpmu_struct *vpmu = vcpu_vpmu(v); > + struct vcpu **last = &per_cpu(last_vcpu, vpmu->last_pcpu); > + > + if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) ) > + return; > + Could this just be: (void)cmpxchg(&per_cpu(last_vcpu, vpmu->last_pcpu), v, NULL); so that we don't do the 'per_cpu' access in case we return because VPMU_CONTEXT_ALLOCATED is not set? Either way, Release-Acked-by: Konrad Rzeszutek Wilk Thank you for spotting this. > + /* Need to clear last_vcpu in case it points to v */ > + (void)cmpxchg(last, v, NULL); > > if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy ) > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > -- > 1.7.1 >