[PATCH 1/2] KVM: x86/pmu: Extract check_pmu_event_filter() from the same semantics

[PATCH 1/2] KVM: x86/pmu: Extract check_pmu_event_filter() from the same semantics

[Like Xu]

From: Like Xu <likexu@tencent.com>

Checking the kvm->arch.pmu_event_filter policy in both gp and fixed
code paths was somewhat redundant, so common parts can be extracted,
which reduces code footprint, improves readability and facilitates
the maintenance of SRCU logic for pmu_event_filter in a one place.

Signed-off-by: Like Xu <likexu@tencent.com>
Reviewed-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/pmu.c | 61 +++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index f614f95acc6b..af2a3dd22dd9 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -179,12 +179,42 @@ static int cmp_u64(const void *a, const void *b)
 	return *(__u64 *)a - *(__u64 *)b;
 }
 
+static bool check_pmu_event_filter(struct kvm_pmc *pmc)
+{
+	struct kvm_pmu_event_filter *filter;
+	struct kvm *kvm = pmc->vcpu->kvm;
+	bool allow_event = true;
+	__u64 key;
+	int idx;
+
+	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
+	if (!filter)
+		goto out;
+
+	if (pmc_is_gp(pmc)) {
+		key = pmc->eventsel & AMD64_RAW_EVENT_MASK_NB;
+		if (bsearch(&key, filter->events, filter->nevents,
+			    sizeof(__u64), cmp_u64))
+			allow_event = filter->action == KVM_PMU_EVENT_ALLOW;
+		else
+			allow_event = filter->action == KVM_PMU_EVENT_DENY;
+	} else {
+		idx = pmc->idx - INTEL_PMC_IDX_FIXED;
+		if (filter->action == KVM_PMU_EVENT_DENY &&
+		    test_bit(idx, (ulong *)&filter->fixed_counter_bitmap))
+			allow_event = false;
+		if (filter->action == KVM_PMU_EVENT_ALLOW &&
+		    !test_bit(idx, (ulong *)&filter->fixed_counter_bitmap))
+			allow_event = false;
+	}
+
+out:
+	return allow_event;
+}
+
 void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel)
 {
 	unsigned config, type = PERF_TYPE_RAW;
-	struct kvm *kvm = pmc->vcpu->kvm;
-	struct kvm_pmu_event_filter *filter;
-	bool allow_event = true;
 
 	if (eventsel & ARCH_PERFMON_EVENTSEL_PIN_CONTROL)
 		printk_once("kvm pmu: pin control bit is ignored\n");
@@ -196,17 +226,7 @@ void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel)
 	if (!(eventsel & ARCH_PERFMON_EVENTSEL_ENABLE) || !pmc_is_enabled(pmc))
 		return;
 
-	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
-	if (filter) {
-		__u64 key = eventsel & AMD64_RAW_EVENT_MASK_NB;
-
-		if (bsearch(&key, filter->events, filter->nevents,
-			    sizeof(__u64), cmp_u64))
-			allow_event = filter->action == KVM_PMU_EVENT_ALLOW;
-		else
-			allow_event = filter->action == KVM_PMU_EVENT_DENY;
-	}
-	if (!allow_event)
+	if (!check_pmu_event_filter(pmc))
 		return;
 
 	if (!(eventsel & (ARCH_PERFMON_EVENTSEL_EDGE |
@@ -241,23 +261,14 @@ void reprogram_fixed_counter(struct kvm_pmc *pmc, u8 ctrl, int idx)
 {
 	unsigned en_field = ctrl & 0x3;
 	bool pmi = ctrl & 0x8;
-	struct kvm_pmu_event_filter *filter;
-	struct kvm *kvm = pmc->vcpu->kvm;
 
 	pmc_pause_counter(pmc);
 
 	if (!en_field || !pmc_is_enabled(pmc))
 		return;
 
-	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
-	if (filter) {
-		if (filter->action == KVM_PMU_EVENT_DENY &&
-		    test_bit(idx, (ulong *)&filter->fixed_counter_bitmap))
-			return;
-		if (filter->action == KVM_PMU_EVENT_ALLOW &&
-		    !test_bit(idx, (ulong *)&filter->fixed_counter_bitmap))
-			return;
-	}
+	if (!check_pmu_event_filter(pmc))
+		return;
 
 	if (pmc->current_config == (u64)ctrl && pmc_resume_counter(pmc))
 		return;
-- 
2.35.0

[PATCH 2/2] KVM: x86/pmu: Protect kvm->arch.pmu_event_filter with SRCU

[Like Xu]

From: Like Xu <likexu@tencent.com>

Fix the following positive warning:

 =============================
 WARNING: suspicious RCU usage
 arch/x86/kvm/pmu.c:190 suspicious rcu_dereference_check() usage!
 other info that might help us debug this:
 rcu_scheduler_active = 2, debug_locks = 1
 1 lock held by CPU 28/KVM/370841:
 #0: ff11004089f280b8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x87/0x730 [kvm]
 Call Trace:
  <TASK>
  dump_stack_lvl+0x59/0x73
  reprogram_fixed_counter+0x15d/0x1a0 [kvm]
  kvm_pmu_trigger_event+0x1a3/0x260 [kvm]
  ? free_moved_vector+0x1b4/0x1e0
  complete_fast_pio_in+0x8a/0xd0 [kvm]
  [...]

It's possible to call KVM_SET_PMU_EVENT_FILTER ioctl with the vCPU running.
Similar to "kvm->arch.msr_filter", KVM should guarantee that vCPUs will
see either the previous filter or the new filter so that guest pmu events
with identical settings in both the old and new filter have deterministic
behavior.

Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter")
Signed-off-by: Like Xu <likexu@tencent.com>
Reviewed-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/pmu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index af2a3dd22dd9..94319f627f64 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -185,8 +185,9 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
 	struct kvm *kvm = pmc->vcpu->kvm;
 	bool allow_event = true;
 	__u64 key;
-	int idx;
+	int idx, srcu_idx;
 
+	srcu_idx = srcu_read_lock(&kvm->srcu);
 	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
 	if (!filter)
 		goto out;
@@ -209,6 +210,7 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
 	}
 
 out:
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
 	return allow_event;
 }
 
-- 
2.35.0

Re: [PATCH 2/2] KVM: x86/pmu: Protect kvm->arch.pmu_event_filter with SRCU

[Paolo Bonzini]

On 2/17/22 09:36, Like Xu wrote:
> From: Like Xu<likexu@tencent.com>
> 
> Fix the following positive warning:
> 
>   =============================
>   WARNING: suspicious RCU usage
>   arch/x86/kvm/pmu.c:190 suspicious rcu_dereference_check() usage!
>   other info that might help us debug this:
>   rcu_scheduler_active = 2, debug_locks = 1
>   1 lock held by CPU 28/KVM/370841:
>   #0: ff11004089f280b8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x87/0x730 [kvm]
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x59/0x73
>    reprogram_fixed_counter+0x15d/0x1a0 [kvm]
>    kvm_pmu_trigger_event+0x1a3/0x260 [kvm]
>    ? free_moved_vector+0x1b4/0x1e0
>    complete_fast_pio_in+0x8a/0xd0 [kvm]
>    [...]

I think the right fix is to add SRCU protection to complete_userspace_io 
in kvm_arch_vcpu_ioctl_run.  Most calls of complete_userspace_io can 
execute similar code to vmexits.

> Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter")

It fixes 9cd803d496e7 ("KVM: x86: Update vPMCs when retiring 
instructions", 2022-01-07), actually.  That is when the PMU filter was 
added to kvm_skip_emulated_instruction (called by kvm_fast_pio_in).

Thanks,

Paolo

> It's possible to call KVM_SET_PMU_EVENT_FILTER ioctl with the vCPU running.
> Similar to "kvm->arch.msr_filter", KVM should guarantee that vCPUs will
> see either the previous filter or the new filter so that guest pmu events
> with identical settings in both the old and new filter have deterministic
> behavior.

Re: [PATCH 2/2] KVM: x86/pmu: Protect kvm->arch.pmu_event_filter with SRCU

[Sean Christopherson]

On Fri, Feb 18, 2022, Paolo Bonzini wrote:
> On 2/17/22 09:36, Like Xu wrote:
> > From: Like Xu<likexu@tencent.com>
> > 
> > Fix the following positive warning:
> > 
> >   =============================
> >   WARNING: suspicious RCU usage
> >   arch/x86/kvm/pmu.c:190 suspicious rcu_dereference_check() usage!
> >   other info that might help us debug this:
> >   rcu_scheduler_active = 2, debug_locks = 1
> >   1 lock held by CPU 28/KVM/370841:
> >   #0: ff11004089f280b8 (&vcpu->mutex){+.+.}-{3:3}, at: kvm_vcpu_ioctl+0x87/0x730 [kvm]
> >   Call Trace:
> >    <TASK>
> >    dump_stack_lvl+0x59/0x73
> >    reprogram_fixed_counter+0x15d/0x1a0 [kvm]
> >    kvm_pmu_trigger_event+0x1a3/0x260 [kvm]
> >    ? free_moved_vector+0x1b4/0x1e0
> >    complete_fast_pio_in+0x8a/0xd0 [kvm]
> >    [...]
> 
> I think the right fix is to add SRCU protection to complete_userspace_io in
> kvm_arch_vcpu_ioctl_run.  Most calls of complete_userspace_io can execute
> similar code to vmexits.

Agreed, I bet similar warnings can be triggered on SVM with nrips=false due to
svm_skip_emulated_instruction() dropping into the emulator, e.g. for HyperV and
Xen usage where next_rip doesn't appear to be filled in all paths.