[PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

[PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

[Sandipan Das]

Branch Sampling (BRS) and Last Branch Record (LBR) are mutually
exclusive hardware features, and users of both are tracked via
cpuc->lbr_users.

When SVM is toggled on a CPU, the host perf events are reprogrammed to
update the HostOnly filter bit (set when virtualization is enabled,
cleared when it is disabled). On PerfMonV2-capable processors, this
reprogramming is performed by calling amd_pmu_enable_all() to rewrite
the event selectors. However, amd_pmu_enable_all() also calls
amd_brs_enable_all(), which enables BRS whenever cpuc->lbr_users > 0.
Having active LBR events satisfies this gating on processors that have
LBR but not BRS. The kernel then tries to set the BRS enable bit in
DebugExtnCfg (MSR 0xc000010f). Since that bit is deprecated on such
hardware, the write results in a #GP, as seen below.

  [ 6869.372584] unchecked MSR access error: WRMSR to 0xc000010f (tried to write 0x000000000000001c) at rIP: 0xffffffff810100f3 (amd_brs_enable_all+0x43/0x50)
  [ 6869.372606] Call Trace:
  [ 6869.372611]  <IRQ>
  [ 6869.372613]  amd_pmu_enable_all+0x1d/0x90
  [ 6869.372620]  amd_pmu_disable_virt+0x62/0xb0
  [ 6869.372627]  kvm_arch_disable_virtualization_cpu+0xa/0x40 [kvm]
  [ 6869.372690]  hardware_disable_nolock+0x1a/0x30 [kvm]
  [ 6869.372729]  __flush_smp_call_function_queue+0x9b/0x410
  [ 6869.372735]  __sysvec_call_function+0x18/0xc0
  [ 6869.372740]  sysvec_call_function+0x69/0x90
  [ 6869.372746]  </IRQ>
  [ 6869.372747]  <TASK>
  [ 6869.372748]  asm_sysvec_call_function+0x16/0x20
  [ 6869.372754] RIP: 0010:cpuidle_enter_state+0xc4/0x450
  [ 6869.372759] Code: 00 e8 10 ab 33 ff e8 db f3 ff ff 8b 53 04 49 89 c5 0f 1f 44 00 00 31 ff e8 39 00 32 ff 45 84 ff 0f 85 76 02 00 00 fb 45 85 f6 <0f> 88 9d 01 00 00 49 63 d6 4c 89 e9 48 89 d0 48 c1 e0 04 48 01 d0
  [ 6869.372761] RSP: 0018:ffa0000008bafe88 EFLAGS: 00000202
  [ 6869.372764] RAX: ff11001fd09c7a80 RBX: ff11000115682800 RCX: 000000000000001f
  [ 6869.372766] RDX: 00000000000000b3 RSI: 0000000040189ace RDI: 0000000000000000
  [ 6869.372768] RBP: 0000000000000002 R08: 0000000000000002 R09: 00000000001d317e
  [ 6869.372768] R10: ff11001fd09c32ec R11: 071c71c71c71c71c R12: ffffffff8381aa20
  [ 6869.372770] R13: 0000063f6683245b R14: 0000000000000002 R15: 0000000000000000
  [ 6869.372773]  ? cpuidle_enter_state+0xb7/0x450
  [ 6869.372776]  cpuidle_enter+0x29/0x40
  [ 6869.372782]  cpuidle_idle_call+0xf5/0x160
  [ 6869.372787]  do_idle+0x7b/0xe0
  [ 6869.372789]  cpu_startup_entry+0x26/0x30
  [ 6869.372792]  start_secondary+0x115/0x140
  [ 6869.372795]  secondary_startup_64_no_verify+0x194/0x19b
  [ 6869.372802]  </TASK>

Fix this by ensuring that BRS is not enabled from the event selector
reprogramming path even when cpuc->lbr_users > 0.

Fixes: bae19fdd7e9e ("perf/x86/amd/core: Fix reloading events for SVM")
Signed-off-by: Sandipan Das <sandipan.das@amd.com>
---
 arch/x86/events/amd/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 6569048a8c1c..a787409f5a62 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -754,13 +754,11 @@ static void amd_pmu_enable_event(struct perf_event *event)
 	x86_pmu_enable_event(event);
 }
 
-static void amd_pmu_enable_all(int added)
+static void __amd_pmu_enable_all(void)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	int idx;
 
-	amd_brs_enable_all();
-
 	for_each_set_bit(idx, x86_pmu.cntr_mask, X86_PMC_IDX_MAX) {
 		/* only activate events which are marked as active */
 		if (!test_bit(idx, cpuc->active_mask))
@@ -775,6 +773,12 @@ static void amd_pmu_enable_all(int added)
 	}
 }
 
+static void amd_pmu_enable_all(int added)
+{
+	amd_brs_enable_all();
+	__amd_pmu_enable_all();
+}
+
 static void amd_pmu_v2_enable_event(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -1561,7 +1565,7 @@ static inline void amd_pmu_reload_virt(void)
 		 * set global enable bits once again
 		 */
 		amd_pmu_v2_disable_all();
-		amd_pmu_enable_all(0);
+		__amd_pmu_enable_all();
 		amd_pmu_v2_enable_all(0);
 		return;
 	}
-- 
2.53.0

Re: [PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] An NMI race condition during PMU reprogramming can lead to an NMI storm (Denial of Service) by erroneously re-enabling a throttled PMU event.
--

--- Patch [1]: [PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path ---
commit d7d24046acc25b0323b31722e9413ffccd9d4f43
Author: Sandipan Das <sandipan.das@amd.com>

perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

This commit fixes a general protection fault (#GP) during an SVM toggle on
processors that support LBR but not BRS. It separates the reprogramming of
event selectors from the BRS enablement to avoid writing to the deprecated
DebugExtnCfg MSR.

> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
[ ... ]
> @@ -752,13 +752,11 @@ static void amd_pmu_enable_event(struct perf_event *event)
>  	x86_pmu_enable_event(event);
>  }
>  
> -static void amd_pmu_enable_all(int added)
> +static void __amd_pmu_enable_all(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	int idx;
>  
> -	amd_brs_enable_all();
> -
>  	for_each_set_bit(idx, x86_pmu.cntr_mask, X86_PMC_IDX_MAX) {
>  		/* only activate events which are marked as active */
>  		if (!test_bit(idx, cpuc->active_mask))
> @@ -773,6 +771,12 @@ static void amd_pmu_enable_all(int added)
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about a
pre-existing issue in the __amd_pmu_enable_all() event iteration loop in
arch/x86/events/amd/core.c.

The loop checks test_bit(idx, cpuc->active_mask), and then subsequently calls
amd_pmu_enable_event(cpuc->events[idx]).

Could an NMI strike between evaluating the active_mask and actually enabling
the event?

CPU0 in __amd_pmu_enable_all():
   if (!test_bit(idx, cpuc->active_mask)) 
       // evaluates to false, event is active, execution is interrupted

NMI fires:
x86_pmu_handle_irq()
   // event overflows and is throttled
   x86_pmu_stop()
       clear_bit(idx, cpuc->active_mask)
       hwc->state |= PERF_HES_STOPPED
   // cpuc->events[idx] is NOT set to NULL here

CPU0 resumes:
   if (cpuc->events[idx])
       amd_pmu_enable_event(cpuc->events[idx]);

Would this result in the hardware re-arming a throttled event? On the next
overflow, an NMI would be generated. However, because the event is no longer
in active_mask, would the NMI handler ignore it, failing to clear the
overflow status?

Could this uncleared overflow trigger an unrecoverable NMI storm and lock up
the CPU, especially since an unprivileged user might be able to reach this
path by profiling a KVM guest during an SVM reload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/702fa204d574b03d14e3664c7d4b201db048bbfd.1783506528.git.sandipan.das@amd.com?part=1