* [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
@ 2014-12-17 15:35 Boris Ostrovsky
2014-12-17 16:21 ` Konrad Rzeszutek Wilk
2014-12-17 17:28 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2014-12-17 15:35 UTC (permalink / raw)
To: jbeulich, keir, konrad.wilk; +Cc: boris.ostrovsky, xen-devel
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@oracle.com>
---
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;
+
+ /* 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
2014-12-17 15:35 [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU Boris Ostrovsky
@ 2014-12-17 16:21 ` Konrad Rzeszutek Wilk
2014-12-17 16:53 ` Boris Ostrovsky
2014-12-17 17:28 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-12-17 16:21 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: keir, jbeulich, xen-devel
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@oracle.com>
> ---
> 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 <konrad.wilk@oracle.com>
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
2014-12-17 16:21 ` Konrad Rzeszutek Wilk
@ 2014-12-17 16:53 ` Boris Ostrovsky
2014-12-17 17:16 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2014-12-17 16:53 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: keir, jbeulich, xen-devel
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@oracle.com>
>> ---
>> 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?
-boris
>
> Either way,
>
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
2014-12-17 16:53 ` Boris Ostrovsky
@ 2014-12-17 17:16 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-12-17 17:16 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: keir, jbeulich, xen-devel
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@oracle.com>
> >>---
> >> 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@oracle.com>
> >
> >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
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
2014-12-17 15:35 [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU Boris Ostrovsky
2014-12-17 16:21 ` Konrad Rzeszutek Wilk
@ 2014-12-17 17:28 ` Jan Beulich
2014-12-17 17:57 ` Boris Ostrovsky
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2014-12-17 17:28 UTC (permalink / raw)
To: boris.ostrovsky; +Cc: keir, xen-devel
>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 12/17/14 4:10 PM >>>
>+ /* Need to clear last_vcpu in case it points to v */
>+ (void)cmpxchg(last, v, NULL);
In a (later) reply to v2 I had indicated that it doesn't seem safe to so here but
rely on an IPI in the other path altering that value. Can you explain why you
think this is safe without changing the others to CPU-atomic accesses too?
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU
2014-12-17 17:28 ` Jan Beulich
@ 2014-12-17 17:57 ` Boris Ostrovsky
0 siblings, 0 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2014-12-17 17:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: keir, xen-devel
On 12/17/2014 12:28 PM, Jan Beulich wrote:
>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 12/17/14 4:10 PM >>>
>> + /* Need to clear last_vcpu in case it points to v */
>> + (void)cmpxchg(last, v, NULL);
>
> In a (later) reply to v2 I had indicated that it doesn't seem safe to so here but
> rely on an IPI in the other path altering that value. Can you explain why you
> think this is safe without changing the others to CPU-atomic accesses too?
So the local access to last_vcpu in vpmu_load() may be still raced on (I
was thinking that access in vpmu_load() would on the same processor as
vpmu_destroy(), which is obviously not the case).
So I will have to go back to IPI (I don't want to use cmpxchg in
vpmu_load()).
-boris
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-17 17:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-17 15:35 [PATCH v3] x86/VPMU: Clear last_vcpu when destroying VPMU Boris Ostrovsky
2014-12-17 16:21 ` Konrad Rzeszutek Wilk
2014-12-17 16:53 ` Boris Ostrovsky
2014-12-17 17:16 ` Konrad Rzeszutek Wilk
2014-12-17 17:28 ` Jan Beulich
2014-12-17 17:57 ` Boris Ostrovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.