[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
On Wed, Dec 17, 2014 at 11:53:41AM -0500, Boris Ostrovsky wrote: > On 12/17/2014 11:21 AM, Konrad Rzeszutek Wilk wrote: > >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 <boris.ostrovsky@xxxxxxxxxx> > >>--- > >> 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? > > > Ugh. This is actually what I meant to send but I forgot to refresh the > patch. > > Do you want me to re-send it? Never hurts. I would just respond to this email with the patch as attachment and inline for easier commit. > > -boris > > > > > >Either way, > > > >Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> > > > >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 > >> > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |