Re: [PATCH] perf/amd/ibs: Clear stale IBS_{FETCH|OP}_CTL in perf_ibs_start() with CTL2[Dis]

[Ravi Bangoria]

Hi Jim,

> Commit 1b044ff3c17e ("perf/amd/ibs: Avoid race between event add and
> NMI") makes perf_ibs_start() reset IBS_{FETCH|OP}_CTL before it sets
> IBS_STARTED. Thus, an NMI from another source that arrives before the
> event is enabled finds Val=0 and does not process a stale sample or
> enable the event too early.
> 
> The reset is a call to perf_ibs_disable_event() with config=0. A later
> commit changed perf_ibs_disable_event():
> 
>   commit efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race")
> 
> When IBS_CAPS_DIS is set, the function now writes only
> IBS_{FETCH|OP}_CTL2 and returns. It does not write IBS_{FETCH|OP}_CTL.
> On hardware with IBS_CAPS_DIS, the reset does nothing, and the race is
> possible again: CTL still holds the previous sample, with Val=1, when
> IBS_STARTED is set.
> 
> Clear IBS_{FETCH|OP}_CTL explicitly when IBS_CAPS_DIS is set, as
> perf_ibs_disable_event() did before CTL2 support. Everything in CTL is
> stale at this point, so it does not matter if this write discards a
> Val that the hardware sets late.

Since the additional disable bit in the independent control register
eliminates the RMW write race, we can simplify the logic by removing
the IBS_STARTED and IBS_STOPPED software states. However, we may still
need to retain IBS_STOPPING, since ->stop() can race with NMIs, and
NMIs also arrive with a delay.

Can you please review this. I'll send a formal patch after testing
it thoroughly.

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 3531f9c23b8c..c9b96c3fa7ac 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -28,6 +28,7 @@ static u32 ibs_caps;
 #include <linux/hardirq.h>
 
 #include <asm/nmi.h>
+#include <asm/delay.h>
 #include <asm/amd/ibs.h>
 
 /* attr.config2 */
@@ -73,7 +74,12 @@ static u32 ibs_caps;
  *
  * XXX: we could probably be using !atomic bitops for all this.
  */
-
+/*
+ * NOTE: IBS_CAPS_DIS eliminates the RMW race on the IBS_{FETCH|OP}_CTL, making
+ * IBS_STARTED and IBS_STOPPED states unnecessary. We use only IBS_ENABLED and
+ * IBS_STOPPING states when IBS_CAPS_DIS is present. Semantics of IBS_STOPPING
+ * changes a bit as well.
+ */
 enum ibs_states {
 	IBS_ENABLED	= 0,
 	IBS_STARTED	= 1,
@@ -575,23 +581,82 @@ static void perf_ibs_start(struct perf_event *event, int flags)
 	}
 	config |= period >> 4;
 
-	/*
-	 * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state.
-	 * Doing so prevents a race condition in which an NMI due to other
-	 * source might accidentally activate the event before we enable
-	 * it ourselves.
-	 */
-	perf_ibs_disable_event(perf_ibs, hwc, 0);
+	if (!(ibs_caps & IBS_CAPS_DIS)) {
+		/*
+		 * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state.
+		 * Doing so prevents a race condition in which an NMI due to
+		 * other source might accidentally activate the event before we
+		 * enable it ourselves.
+		 */
+		perf_ibs_disable_event(perf_ibs, hwc, 0);
+		/*
+		 * Set STARTED before enabling the hardware, such that a
+		 * subsequent NMI must observe it.
+		 */
+		set_bit(IBS_STARTED,    pcpu->state);
+		clear_bit(IBS_STOPPING, pcpu->state);
+	}
+	perf_ibs_enable_event(perf_ibs, hwc, config);
+
+	perf_event_update_userpage(event);
+}
+
+/* ->stop() for platforms with additional CTL2[Dis] bit. */
+static void perf_ibs_stop_dis(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct perf_ibs *perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
+	struct cpu_perf_ibs *pcpu = this_cpu_ptr(perf_ibs->pcpu);
+	unsigned int delay = 50;
+	u64 ctl2, ctl;
+	u64 config;
+
+	WARN_ON_ONCE(test_bit(IBS_STOPPING, pcpu->state));
+
+	set_bit(IBS_STOPPING, pcpu->state);
+
+	rdmsrq(hwc->extra_reg.reg, ctl2);
+	if (ctl2 & perf_ibs->disable_mask)
+		goto reset_state;
+
+	wrmsrq(hwc->extra_reg.reg, perf_ibs->disable_mask);
+
+	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
+	hwc->state |= PERF_HES_STOPPED;
+
+	if (hwc->state & PERF_HES_UPTODATE)
+		goto reset_state;
+
+	rdmsrq(hwc->config_base, config);
 
 	/*
-	 * Set STARTED before enabling the hardware, such that a subsequent NMI
-	 * must observe it.
+	 *      IBS HW                      SW
+	 *
+	 *  o Capture sample        o ->stop()
+	 *  o Set VAL bit             CTL2[Dis] = 1
+	 *  o Raise NMI             o ...
+	 *                          o ->start()
+	 *                            wrmsr(IBS_{FETCH|OP}_CTL, new value);
+	 *                            (wrmsr clears the VAL bit)
+	 *                          o Receives delayed NMI
+	 *                            (Unknown NMI since VAL bit is cleared)
+	 *
+	 * Handle this with induced delay.
 	 */
-	set_bit(IBS_STARTED,    pcpu->state);
-	clear_bit(IBS_STOPPING, pcpu->state);
-	perf_ibs_enable_event(perf_ibs, hwc, config);
+	if (config & perf_ibs->valid_mask) {
+		ctl = config;
+		while (delay-- && ctl & perf_ibs->valid_mask) {
+			udelay(1);
+			rdmsrq(hwc->config_base, ctl);
+		}
+	}
 
-	perf_event_update_userpage(event);
+	config &= ~perf_ibs->valid_mask;
+	perf_ibs_event_update(perf_ibs, event, &config);
+	hwc->state |= PERF_HES_UPTODATE;
+
+reset_state:
+	clear_bit(IBS_STOPPING, pcpu->state);
 }
 
 static void perf_ibs_stop(struct perf_event *event, int flags)
@@ -602,6 +667,9 @@ static void perf_ibs_stop(struct perf_event *event, int flags)
 	u64 config;
 	int stopping;
 
+	if (ibs_caps & IBS_CAPS_DIS)
+		return perf_ibs_stop_dis(event, flags);
+
 	if (test_and_set_bit(IBS_STOPPING, pcpu->state))
 		return;
 
@@ -1416,19 +1484,38 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 	unsigned int msr;
 	u64 *buf, *config, period, new_config = 0;
 	int br_target_idx = -1;
+	u64 ctl2, ctl;
 
-	if (!test_bit(IBS_STARTED, pcpu->state)) {
-fail:
-		/*
-		 * Catch spurious interrupts after stopping IBS: After
-		 * disabling IBS there could be still incoming NMIs
-		 * with samples that even have the valid bit cleared.
-		 * Mark all this NMIs as handled.
-		 */
-		if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
+	/* Catch delayed NMIs which arrives after disabling IBS PMUs. */
+	if (ibs_caps & IBS_CAPS_DIS) {
+		rdmsrq(perf_ibs->msr2, ctl2);
+		if (ctl2 & perf_ibs->disable_mask) {
+			rdmsrq(perf_ibs->msr, ctl);
+			if (ctl & perf_ibs->valid_mask) {
+				ctl &= ~(perf_ibs->enable_mask &
+					 perf_ibs->valid_mask);
+				wrmsrq(perf_ibs->msr, ctl);
+				return 1;
+			}
+			return 0;
+		}
+
+		if (test_bit(IBS_STOPPING, pcpu->state))
 			return 1;
+	} else {
+		if (!test_bit(IBS_STARTED, pcpu->state)) {
+fail:
+			/*
+			 * Catch spurious interrupts after stopping IBS: After
+			 * disabling IBS there could be still incoming NMIs
+			 * with samples that even have the valid bit cleared.
+			 * Mark all this NMIs as handled.
+			 */
+			if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
+				return 1;
 
-		return 0;
+			return 0;
+		}
 	}
 
 	if (WARN_ON_ONCE(!event))
---

> A related question for AMD: with IBS_CAPS_DIS, perf_ibs_stop() now
> leaves IBS_{FETCH|OP}_CTL[En]=1. APM vol. 2, section 15.38, says that
> IbsFetchEn and IbsOpEn must be 0 at VMRUN of an SEV-ES or SEV-SNP
> guest with IBS virtualization enabled, and should be 0 for other
> guests.

I think the intention is, host IBS should be disabled (could be from
CTL2[Dis]) before running VMRUN. But I'll confirm with the HW team.

Thanks,
Ravi