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

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

[Jim Mattson]

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.

Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race")
Assisted-by: LLM
Signed-off-by: Jim Mattson <jmattson@google.com>
---
I reproduced the race on hardware with IBS_CAPS_DIS. To make the window
wider, I added a debug-only udelay() between set_bit(IBS_STARTED) and
perf_ibs_enable_event() in perf_ibs_start(). A throttled ibs_op event
gave about 1000 perf_ibs_start() calls per second, and a cycles event
gave NMIs from the core PMU. In 60 seconds:

  - Without this patch, IBS_OP_CTL[Val] was 1 at every
    perf_ibs_start() call (60003 of 60003). The IBS op NMI handler took
    the stale sample and re-enabled the event during the window 17
    times.
  - With this patch, the handler never took a sample during the window.

Without the udelay(), the window is only a few instructions long.

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. Can the hardware set CTL[Val] or change CTL in any other way
after CTL2[Dis] is set to 1? If not, perf_ibs_stop() can safely clear
CTL[En] with a read-modify-write after it sets CTL2[Dis].

 arch/x86/events/amd/ibs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 3531f9c23b8c..3a5475006b59 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -580,8 +580,15 @@ static void perf_ibs_start(struct perf_event *event, int flags)
 	 * Doing so prevents a race condition in which an NMI due to other
 	 * source might accidentally activate the event before we enable
 	 * it ourselves.
+	 *
+	 * With IBS_CAPS_DIS, perf_ibs_disable_event() only sets CTL2[Dis]
+	 * and leaves the previous sample in CTL, so clear CTL explicitly.
+	 * Anything in CTL is stale, so it does not matter if this write
+	 * discards a late Val.
 	 */
 	perf_ibs_disable_event(perf_ibs, hwc, 0);
+	if (ibs_caps & IBS_CAPS_DIS)
+		wrmsrq(hwc->config_base, 0);
 
 	/*
 	 * Set STARTED before enabling the hardware, such that a subsequent NMI
-- 
2.56.0.385.gd3acb90ef8-goog

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

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

[Jim Mattson]

On Fri, Oct 09, 2026 at 09:19:49AM +0530, Ravi Bangoria wrote:
> 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.

Your approach also fixes the race, but I'm not convinced that it's an
improvement. There's a lot more complexity here.

[...]

> +	if (config & perf_ibs->valid_mask) {
> +		ctl = config;
> +		while (delay-- && ctl & perf_ibs->valid_mask) {
> +			udelay(1);
> +			rdmsrq(hwc->config_base, ctl);
> +		}
> +	}

This loop will time out when running in NMI context:

    perf_ibs_handle_irq() ->
      perf_event_overflow() ->
        __perf_event_overflow() ->
          __perf_event_account_interrupt() ->
            perf_event_throttle_group() ->
              perf_event_throttle() ->
                perf_ibs_stop() ->
                  perf_ibs_stop_dis() 

Should the loop just be skipped when in_nmi()?

[...]

> +	/* 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);

enable_mask and valid_mask are disjoint. Should '&' be '|'?

> +				wrmsrq(perf_ibs->msr, ctl);
> +				return 1;
> +			}
> +			return 0;
> +		}
> +

[...]

>       if (WARN_ON_ONCE(!event))

With IBS_CAPS_DIS, we are now treating CTL2[Dis] as authoritative on the
question of "is IBS idle." However, on the BSP after S3 resume or on any
LPU after an AE #VMEXIT from an SEV-ES/SNP guest with IBS virtualization
enabled, CTL2[Dis] will be reset to 0 and may no longer be authoritative.

> 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.

Another question for the HW team: can the hardware set CTL[Val], or change
CTL in any other way, after CTL2[Dis] has been set? The answer may
obviate the need for the 50us poll.

--jim