* [PATCH] KVM: vPMU: Use atomic bit operations for global_status
@ 2023-09-11 6:11 Mingwei Zhang
2023-09-11 15:01 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Mingwei Zhang @ 2023-09-11 6:11 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: H. Peter Anvin, kvm, linux-kernel, Mingwei Zhang, Dapeng Mi,
Jim Mattson, Like Xu
Use atomic bit operations for pmu->global_status because it may suffer from
race conditions between emulated overflow in KVM vPMU and PEBS overflow in
host PMI handler.
Fixes: f331601c65ad ("KVM: x86/pmu: Don't generate PEBS records for emulated instructions")
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
arch/x86/kvm/pmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index edb89b51b383..00b48f25afdb 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -117,11 +117,11 @@ static inline void __kvm_perf_overflow(struct kvm_pmc *pmc, bool in_pmi)
skip_pmi = true;
} else {
/* Indicate PEBS overflow PMI to guest. */
- skip_pmi = __test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
- (unsigned long *)&pmu->global_status);
+ skip_pmi = test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
+ (unsigned long *)&pmu->global_status);
}
} else {
- __set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
+ set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
}
if (!pmc->intr || skip_pmi)
base-commit: e2013f46ee2e721567783557c301e5c91d0b74ff
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: vPMU: Use atomic bit operations for global_status
2023-09-11 6:11 [PATCH] KVM: vPMU: Use atomic bit operations for global_status Mingwei Zhang
@ 2023-09-11 15:01 ` Sean Christopherson
2023-09-11 18:00 ` Mingwei Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2023-09-11 15:01 UTC (permalink / raw)
To: Mingwei Zhang
Cc: Paolo Bonzini, H. Peter Anvin, kvm, linux-kernel, Dapeng Mi,
Jim Mattson, Like Xu
On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> Use atomic bit operations for pmu->global_status because it may suffer from
> race conditions between emulated overflow in KVM vPMU and PEBS overflow in
> host PMI handler.
Only if the host PMI occurs on a different pCPU, and if that can happen don't we
have a much larger problem?
> Fixes: f331601c65ad ("KVM: x86/pmu: Don't generate PEBS records for emulated instructions")
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
> arch/x86/kvm/pmu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index edb89b51b383..00b48f25afdb 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -117,11 +117,11 @@ static inline void __kvm_perf_overflow(struct kvm_pmc *pmc, bool in_pmi)
> skip_pmi = true;
> } else {
> /* Indicate PEBS overflow PMI to guest. */
> - skip_pmi = __test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
> - (unsigned long *)&pmu->global_status);
> + skip_pmi = test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
> + (unsigned long *)&pmu->global_status);
> }
> } else {
> - __set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
> + set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
> }
>
> if (!pmc->intr || skip_pmi)
>
> base-commit: e2013f46ee2e721567783557c301e5c91d0b74ff
> --
> 2.42.0.283.g2d96d420d3-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: vPMU: Use atomic bit operations for global_status
2023-09-11 15:01 ` Sean Christopherson
@ 2023-09-11 18:00 ` Mingwei Zhang
2023-09-11 18:09 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Mingwei Zhang @ 2023-09-11 18:00 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, H. Peter Anvin, kvm, linux-kernel, Dapeng Mi,
Jim Mattson, Like Xu
On Mon, Sep 11, 2023 at 8:01 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> > Use atomic bit operations for pmu->global_status because it may suffer from
> > race conditions between emulated overflow in KVM vPMU and PEBS overflow in
> > host PMI handler.
>
> Only if the host PMI occurs on a different pCPU, and if that can happen don't we
> have a much larger problem?
Why on different pCPU? For vPMU, I think there is always contention
between the vCPU thread and the host PMI handler running on the same
pCPU, no?
So, in that case, anything that __kvm_perf_overflow(..., in_pmi=true)
touches on struct kvm_pmu will potentially race with the functions
like reprogram_counter() -> __kvm_perf_overflow(..., in_pmi=false).
-Mingwei
>
> > Fixes: f331601c65ad ("KVM: x86/pmu: Don't generate PEBS records for emulated instructions")
> > Signed-off-by: Mingwei Zhang <mizhang@google.com>
> > ---
> > arch/x86/kvm/pmu.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index edb89b51b383..00b48f25afdb 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -117,11 +117,11 @@ static inline void __kvm_perf_overflow(struct kvm_pmc *pmc, bool in_pmi)
> > skip_pmi = true;
> > } else {
> > /* Indicate PEBS overflow PMI to guest. */
> > - skip_pmi = __test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
> > - (unsigned long *)&pmu->global_status);
> > + skip_pmi = test_and_set_bit(GLOBAL_STATUS_BUFFER_OVF_BIT,
> > + (unsigned long *)&pmu->global_status);
> > }
> > } else {
> > - __set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
> > + set_bit(pmc->idx, (unsigned long *)&pmu->global_status);
> > }
> >
> > if (!pmc->intr || skip_pmi)
> >
> > base-commit: e2013f46ee2e721567783557c301e5c91d0b74ff
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: vPMU: Use atomic bit operations for global_status
2023-09-11 18:00 ` Mingwei Zhang
@ 2023-09-11 18:09 ` Sean Christopherson
2023-09-11 23:42 ` Mingwei Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2023-09-11 18:09 UTC (permalink / raw)
To: Mingwei Zhang
Cc: Paolo Bonzini, H. Peter Anvin, kvm, linux-kernel, Dapeng Mi,
Jim Mattson, Like Xu
On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> On Mon, Sep 11, 2023 at 8:01 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> > > Use atomic bit operations for pmu->global_status because it may suffer from
> > > race conditions between emulated overflow in KVM vPMU and PEBS overflow in
> > > host PMI handler.
> >
> > Only if the host PMI occurs on a different pCPU, and if that can happen don't we
> > have a much larger problem?
>
> Why on different pCPU? For vPMU, I think there is always contention
> between the vCPU thread and the host PMI handler running on the same
> pCPU, no?
A non-atomic instruction can't be interrupted by an NMI, or any other event, so
I don't see how switching to atomic operations fixes anything unless the NMI comes
in on a different pCPU.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: vPMU: Use atomic bit operations for global_status
2023-09-11 18:09 ` Sean Christopherson
@ 2023-09-11 23:42 ` Mingwei Zhang
0 siblings, 0 replies; 5+ messages in thread
From: Mingwei Zhang @ 2023-09-11 23:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, H. Peter Anvin, kvm, linux-kernel, Dapeng Mi,
Jim Mattson, Like Xu
On Mon, Sep 11, 2023 at 11:09 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> > On Mon, Sep 11, 2023 at 8:01 AM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Mon, Sep 11, 2023, Mingwei Zhang wrote:
> > > > Use atomic bit operations for pmu->global_status because it may suffer from
> > > > race conditions between emulated overflow in KVM vPMU and PEBS overflow in
> > > > host PMI handler.
> > >
> > > Only if the host PMI occurs on a different pCPU, and if that can happen don't we
> > > have a much larger problem?
> >
> > Why on different pCPU? For vPMU, I think there is always contention
> > between the vCPU thread and the host PMI handler running on the same
> > pCPU, no?
>
> A non-atomic instruction can't be interrupted by an NMI, or any other event, so
> I don't see how switching to atomic operations fixes anything unless the NMI comes
> in on a different pCPU.
You are right. I realize that. The race condition has to happen
concurrently from two different pCPUs. This happens to
pmu->reprogram_nmi but not pmu->global_status. The critical stuff we
care about should be re-entrancy issues for __kvm_perf_overflow() and
some state maintenance issues like avoiding duplicate NMI injection.
That concludes that we don't need this change.
Thanks
-Mingwei
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-12 3:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-11 6:11 [PATCH] KVM: vPMU: Use atomic bit operations for global_status Mingwei Zhang
2023-09-11 15:01 ` Sean Christopherson
2023-09-11 18:00 ` Mingwei Zhang
2023-09-11 18:09 ` Sean Christopherson
2023-09-11 23:42 ` Mingwei Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox