[Patch v3 0/4] perf/x86/intel: Fix bugs of auto counter reload sampling

[Patch v3 0/4] perf/x86/intel: Fix bugs of auto counter reload sampling

[Dapeng Mi]

Changes:
v2 -> v3:
- Fix the issue that user-space ACR-mask is not properly validated.
  (Sashiko, patch 1/4)

v1 -> v2:
- Clear stale mask for all events (Sashiko, Patch 1/4)
- Enable auto counter reload for DMR. (Patch 3/4)
- Remove duplicated CFG_C MSR value tracking. (Patch 4/4)

This small patch-set fixes several issues in auto counter reload support.
- The stale ACR mask is not properly validated before setting a new one.
  Patch 1/4 fixes this issue.
- PMI is enabled by default for self-reloaded ACR events which causes
  suspicious NMI warning. Patch 2/4 fixes this issue.
- ACR sampling is not really enabled on DMR. Patch 3/4 fixes the issue.
- Two variables are used to trace CFG_C MSR value independently for ACR
  and arch-PEBS. It's error-prone and fragile. Patch 4/4 fixes this
  issue.

Besides an ACR unit test is added into perf tests which would be posted
in a separate session.

Tests:

Run below ACR sampling commands on CWF, DMR, PTL and NVL (hybrid
platforms), no issues are found.

1. Non-PEBS ACR sampling
perf record -e '{instructions/period=20000,acr_mask=0x2/u,cycles/period=40000,acr_mask=0x3/u}' ~/test

2. PEBS ACR sampling

perf record -e '{instructions/period=20000,acr_mask=0x2/pu,cycles/period=40000,acr_mask=0x3/u}' ~/test

3. Perf-tools ACR sampling test

The patch (https://lore.kernel.org/all/20260420024528.2130065-1-dapeng1.mi@linux.intel.com/)
adds ACR sampling test case in perf tools record test.

perf test 148

History:
v1: https://lore.kernel.org/all/20260413010157.535990-1-dapeng1.mi@linux.intel.com/
v2: https://lore.kernel.org/all/20260420024528.2130065-1-dapeng1.mi@linux.intel.com/

Dapeng Mi (4):
  perf/x86/intel: Improve validation and configuration of ACR masks
  perf/x86/intel: Disable PMI for self-reloaded ACR events
  perf/x86/intel: Enable auto counter reload for DMR
  perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

 arch/x86/events/intel/core.c | 63 ++++++++++++++++++++++++++----------
 arch/x86/events/perf_event.h | 14 ++++++--
 2 files changed, 57 insertions(+), 20 deletions(-)

-- 
2.34.1

[Patch v3 1/4] perf/x86/intel: Improve validation and configuration of ACR masks

[Dapeng Mi]

Currently there are several issues on the user space ACR mask validation
and configuration.
- The validation for user space ACR mask (attr.config2) is incomplete,
  e.g., the ACR mask could include the index which belongs to another
  ACR events group, but it's not validated.
- An early return on an invalid ACR mask caused all subsequent ACR groups
  to be skipped.
- The stale hardware ACR mask (hw.config1) is not cleared before setting
  new hardware ACR mask.

The following changes address all of the above issues.
- Calculate the indices bitmap for each ACR events group. Any bits in
  the user-space mask not present in the group's bitmap are now dropped.
- Instead of an early return on invalid bits, drop only the invalid
  portions and continue iterating through all ACR events to ensure full
  configuration.
- Explicitly clear the stale hardware ACR mask for each event prior to
  writing the new configuration.

Cc: stable@vger.kernel.org
Fixes: ec980e4facef ("perf/x86/intel: Support auto counter reload")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---

v3: new patch.

 arch/x86/events/intel/core.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 4768236c054b..1a2c268018a2 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3332,23 +3332,41 @@ static void intel_pmu_enable_event(struct perf_event *event)
 static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
 {
 	struct perf_event *event, *leader;
-	int i, j, idx;
+	int i, j, k, bit, idx;
+	u64 group_mask;
 
 	for (i = 0; i < cpuc->n_events; i++) {
 		leader = cpuc->event_list[i];
 		if (!is_acr_event_group(leader))
 			continue;
 
-		/* The ACR events must be contiguous. */
+		/* Find the last event of the ACR group. */
 		for (j = i; j < cpuc->n_events; j++) {
 			event = cpuc->event_list[j];
 			if (event->group_leader != leader->group_leader)
 				break;
-			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
-				if (i + idx >= cpuc->n_events ||
-				    !is_acr_event_group(cpuc->event_list[i + idx]))
-					return;
-				__set_bit(cpuc->assign[i + idx], (unsigned long *)&event->hw.config1);
+		}
+
+		/* Figure out the group indices bitmap. */
+		group_mask = 0;
+		for (k = i; k < j; k++)
+			group_mask |= BIT_ULL(cpuc->assign[k]);
+
+		/*
+		 * Translate the user-space ACR mask (attr.config2) into the physical
+		 * counter bitmask (hw.config1) for each ACR event in the group.
+		 * NOTE: ACR event contiguity is guaranteed by intel_pmu_hw_config().
+		 */
+		for (k = i; k < j; k++) {
+			event = cpuc->event_list[k];
+			event->hw.config1 = 0;
+			for_each_set_bit(bit, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
+				idx = i + bit;
+				if (idx >= cpuc->n_events ||
+				    !(BIT_ULL(cpuc->assign[idx]) & group_mask) ||
+				    !is_acr_event_group(cpuc->event_list[idx]))
+					continue;
+				__set_bit(cpuc->assign[idx], (unsigned long *)&event->hw.config1);
 			}
 		}
 		i = j - 1;
-- 
2.34.1

Re: [Patch v3 1/4] perf/x86/intel: Improve validation and configuration of ACR masks

[sashiko-bot]

Sashiko AI review found 3 potential issue(s):
- [High] The ACR feature incorrectly maps logical sibling indices from user space to physical array indices in `event_list`, which silently corrupts the configuration if any sibling is disabled.
- [High] The updated ACR hardware configuration (`hw.config1`) is not synchronized to the PMU hardware register if the event itself does not change counters, leaving the hardware state stale.
- [Low] The patch uses a complex and brittle hardware counter bitmask to verify group membership instead of simply checking array bounds.
--

commit 0bc290b3247e55dbfb3b7cee24f6926da6f5adfb
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
Subject: perf/x86/intel: Improve validation and configuration of ACR masks

This commit addresses several issues with user space ACR mask validation and
configuration. It calculates an index bitmap for each ACR group to drop invalid
user-space mask bits, avoids early returns on invalid bits, and explicitly
clears stale hardware ACR masks.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index d9488ade0f8e..dafa233dd757 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
[ ... ]
> @@ -3332,23 +3332,41 @@ static void intel_pmu_enable_event(struct perf_event *event)
>  static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>  {
>  	struct perf_event *event, *leader;
> -	int i, j, idx;
> +	int i, j, k, bit, idx;
> +	u64 group_mask;
>  
>  	for (i = 0; i < cpuc->n_events; i++) {
>  		leader = cpuc->event_list[i];
>  		if (!is_acr_event_group(leader))
>  			continue;
>  
> -		/* The ACR events must be contiguous. */
> +		/* Find the last event of the ACR group. */
>  		for (j = i; j < cpuc->n_events; j++) {
>  			event = cpuc->event_list[j];
>  			if (event->group_leader != leader->group_leader)
>  				break;
> -			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> -				if (i + idx >= cpuc->n_events ||
> -				    !is_acr_event_group(cpuc->event_list[i + idx]))
> -					return;
> -				__set_bit(cpuc->assign[i + idx], (unsigned long *)&event->hw.config1);
> +		}
> +
> +		/* Figure out the group indices bitmap. */
> +		group_mask = 0;
> +		for (k = i; k < j; k++)
> +			group_mask |= BIT_ULL(cpuc->assign[k]);

Since x86_schedule_events() guarantees strictly unique hardware counters for
all concurrently active core PMU events, a counter is present in group_mask if
and only if its index falls within [i, j).

Given that idx = i + bit guarantees idx >= i, is it possible to simplify this
by just checking if idx >= j instead of constructing and checking the
group_mask?

> +
> +		/*
> +		 * Translate the user-space ACR mask (attr.config2) into the physical
> +		 * counter bitmask (hw.config1) for each ACR event in the group.
> +		 * NOTE: ACR event contiguity is guaranteed by intel_pmu_hw_config().
> +		 */
> +		for (k = i; k < j; k++) {
> +			event = cpuc->event_list[k];
> +			event->hw.config1 = 0;

If Event A moves to a new counter but Event B itself does not move,
match_prev_assignment() would evaluate to true for Event B, and
x86_pmu_start() would be skipped.

Does this mean the updated hw.config1 for Event B is never written to the
physical hardware MSR, breaking the Auto Counter Reload functionality?

> +			for_each_set_bit(bit, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> +				idx = i + bit;

User space provides the ACR mask (attr.config2) using absolute logical bit
indices (e.g., bit 0 = leader, bit 1 = first sibling). However,
collect_events() skips events that are in PERF_EVENT_STATE_OFF.

If an earlier sibling is disabled, it is not added to event_list, causing all
subsequent active siblings to shift down in the array.

Does idx point to the wrong event in the active array if any sibling is
disabled? Should the mask resolve against the logical sibling_list rather
than the compressed active array?

> +				if (idx >= cpuc->n_events ||
> +				    !(BIT_ULL(cpuc->assign[idx]) & group_mask) ||
> +				    !is_acr_event_group(cpuc->event_list[idx]))
> +					continue;
> +				__set_bit(cpuc->assign[idx], (unsigned long *)&event->hw.config1);
>  			}
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427085513.3728672-1-dapeng1.mi@linux.intel.com?part=1

[Patch v3 2/4] perf/x86/intel: Disable PMI for self-reloaded ACR events

[Dapeng Mi]

On platforms with Auto Counter Reload (ACR) support, such as NVL, a
"NMI received for unknown reason 30" warning is observed when running
multiple events in a group with ACR enabled:

  $ perf record -e '{instructions/period=20000,acr_mask=0x2/u,\
    cycles/period=40000,acr_mask=0x3/u}' ./test

The warning occurs because the Performance Monitoring Interrupt (PMI)
is enabled for the self-reloaded event (the cycles event in this case).
According to the Intel SDM, the overflow bit
(IA32_PERF_GLOBAL_STATUS.PMCn_OVF) is never set for self-reloaded events.
Since the bit is not set, the perf NMI handler cannot identify the source
of the interrupt, leading to the "unknown reason" message.

Furthermore, enabling PMI for self-reloaded events is unnecessary and
can lead to extraneous records that pollute the user's requested data.

Disable the interrupt bit for all events configured with ACR self-reload.

Reported-by: Andi Kleen <ak@linux.intel.com>
Cc: stable@vger.kernel.org
Fixes: ec980e4facef ("perf/x86/intel: Support auto counter reload")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/events/intel/core.c | 17 +++++++++++++----
 arch/x86/events/perf_event.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 1a2c268018a2..c1841fa89908 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3118,11 +3118,11 @@ static void intel_pmu_enable_fixed(struct perf_event *event)
 	intel_set_masks(event, idx);
 
 	/*
-	 * Enable IRQ generation (0x8), if not PEBS,
-	 * and enable ring-3 counting (0x2) and ring-0 counting (0x1)
-	 * if requested:
+	 * Enable IRQ generation (0x8), if not PEBS and self-reloaded
+	 * ACR event, and enable ring-3 counting (0x2) and ring-0
+	 * counting (0x1) if requested:
 	 */
-	if (!event->attr.precise_ip)
+	if (!event->attr.precise_ip && !is_acr_self_reload_event(event))
 		bits |= INTEL_FIXED_0_ENABLE_PMI;
 	if (hwc->config & ARCH_PERFMON_EVENTSEL_USR)
 		bits |= INTEL_FIXED_0_USER;
@@ -3306,6 +3306,15 @@ static void intel_pmu_enable_event(struct perf_event *event)
 		intel_set_masks(event, idx);
 		static_call_cond(intel_pmu_enable_acr_event)(event);
 		static_call_cond(intel_pmu_enable_event_ext)(event);
+		/*
+		 * For self-reloaded ACR event, don't enable PMI since
+		 * HW won't set overflow bit in GLOBAL_STATUS. Otherwise,
+		 * the PMI would be recognized as a suspicious NMI.
+		 */
+		if (is_acr_self_reload_event(event))
+			hwc->config &= ~ARCH_PERFMON_EVENTSEL_INT;
+		else if (!event->attr.precise_ip)
+			hwc->config |= ARCH_PERFMON_EVENTSEL_INT;
 		__x86_pmu_enable_event(hwc, enable_mask);
 		break;
 	case INTEL_PMC_IDX_FIXED ... INTEL_PMC_IDX_FIXED_BTS - 1:
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index fad87d3c8b2c..524668dcf4cc 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -137,6 +137,16 @@ static inline bool is_acr_event_group(struct perf_event *event)
 	return check_leader_group(event->group_leader, PERF_X86_EVENT_ACR);
 }
 
+static inline bool is_acr_self_reload_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (hwc->idx < 0)
+		return false;
+
+	return test_bit(hwc->idx, (unsigned long *)&hwc->config1);
+}
+
 struct amd_nb {
 	int nb_id;  /* NorthBridge id */
 	int refcnt; /* reference count */
-- 
2.34.1

[Patch v3 3/4] perf/x86/intel: Enable auto counter reload for DMR

[Dapeng Mi]

Panther cove µarch starts to support auto counter reload (ACR), but the
static_call intel_pmu_enable_acr_event() is not updated for the Panther
Cove µarch used by DMR. It leads to the auto counter reload is not
really enabled on DMR.

Update static_call intel_pmu_enable_acr_event() in intel_pmu_init_pnc().

Cc: stable@vger.kernel.org
Fixes: d345b6bb8860 ("perf/x86/intel: Add core PMU support for DMR")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/events/intel/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index c1841fa89908..60568a9ce06b 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -7518,6 +7518,7 @@ static __always_inline void intel_pmu_init_pnc(struct pmu *pmu)
 	hybrid(pmu, event_constraints) = intel_pnc_event_constraints;
 	hybrid(pmu, pebs_constraints) = intel_pnc_pebs_event_constraints;
 	hybrid(pmu, extra_regs) = intel_pnc_extra_regs;
+	static_call_update(intel_pmu_enable_acr_event, intel_pmu_enable_acr);
 }
 
 static __always_inline void intel_pmu_init_skt(struct pmu *pmu)
-- 
2.34.1

[Patch v3 4/4] perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

[Dapeng Mi]

Both Auto Counter Reload (ACR) and Architectural PEBS use the PERF_CFG_C
MSRs to configure event behavior. Currently, the driver maintains two
independent variables acr_cfg_c and cfg_c_val to cache the values intended
for these MSRs.

Using separate variables to track a single hardware register state is
error-prone and can lead to configuration conflicts. Consolidate the
tracking into a single cfg_c_val variable to ensure a unified and
consistent view of the PERF_CFG_C MSR state.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/events/intel/core.c | 13 +++++++------
 arch/x86/events/perf_event.h |  4 +---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 60568a9ce06b..013e6e02706d 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3169,10 +3169,10 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
 		wrmsrl(msr_b + msr_offset, mask);
 		cpuc->acr_cfg_b[idx] = mask;
 	}
-	/* Only need to update the reload value when there is a valid config value. */
-	if (mask && cpuc->acr_cfg_c[idx] != reload) {
+	/* Only update CFG_C reload when ACR is actively enabled (mask != 0) */
+	if (mask && ((cpuc->cfg_c_val[idx] & ARCH_PEBS_RELOAD) != reload)) {
 		wrmsrl(msr_c + msr_offset, reload);
-		cpuc->acr_cfg_c[idx] = reload;
+		cpuc->cfg_c_val[idx] = reload;
 	}
 }
 
@@ -3198,14 +3198,15 @@ static void intel_pmu_enable_event_ext(struct perf_event *event)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct hw_perf_event *hwc = &event->hw;
-	union arch_pebs_index old, new;
-	struct arch_pebs_cap cap;
 	u64 ext = 0;
 
-	cap = hybrid(cpuc->pmu, arch_pebs_cap);
+	if (is_acr_event_group(event))
+		ext |= (-hwc->sample_period) & ARCH_PEBS_RELOAD;
 
 	if (event->attr.precise_ip) {
 		u64 pebs_data_cfg = intel_get_arch_pebs_data_config(event);
+		struct arch_pebs_cap cap = hybrid(cpuc->pmu, arch_pebs_cap);
+		union arch_pebs_index old, new;
 
 		ext |= ARCH_PEBS_EN;
 		if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD)
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 524668dcf4cc..40d6fe0afc4a 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -322,10 +322,8 @@ struct cpu_hw_events {
 	u64			fixed_ctrl_val;
 	u64			active_fixed_ctrl_val;
 
-	/* Intel ACR configuration */
+	/* Intel ACR/arch-PEBS configuration */
 	u64			acr_cfg_b[X86_PMC_IDX_MAX];
-	u64			acr_cfg_c[X86_PMC_IDX_MAX];
-	/* Cached CFG_C values */
 	u64			cfg_c_val[X86_PMC_IDX_MAX];
 
 	/*
-- 
2.34.1

Re: [Patch v3 4/4] perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [Medium] intel_pmu_config_acr() clobbers upper Architectural PEBS configuration bits in cfg_c_val and the PERF_CFG_C MSR.
--

commit 5ded8e15814c1efc4e7b5c766abdd782de4d360c
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

This commit consolidates the tracking of the PERF_CFG_C MSR into a single
variable, cfg_c_val, to maintain a unified view of the MSR state for both
Auto Counter Reload (ACR) and Architectural PEBS configurations.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
[ ... ]
> @@ -3169,10 +3169,10 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
>  		wrmsrl(msr_b + msr_offset, mask);
>  		cpuc->acr_cfg_b[idx] = mask;
>  	}
> -	/* Only need to update the reload value when there is a valid config value. */
> -	if (mask && cpuc->acr_cfg_c[idx] != reload) {
> +	/* Only update CFG_C reload when ACR is actively enabled (mask != 0) */
> +	if (mask && ((cpuc->cfg_c_val[idx] & ARCH_PEBS_RELOAD) != reload)) {
>  		wrmsrl(msr_c + msr_offset, reload);
> -		cpuc->acr_cfg_c[idx] = reload;
> +		cpuc->cfg_c_val[idx] = reload;
>  	}
>  }

Can this assignment wipe out the upper Architectural PEBS configuration bits?

Since reload is passed as a u32 and cfg_c_val is a 64-bit value, assigning 
reload directly zero-extends the value. This will clear the upper 32 bits, 
unintentionally removing any existing Architectural PEBS configurations 
like ARCH_PEBS_EN from the software cache, and writing zeroes to the 
upper bits of the physical MSR.

Although intel_pmu_enable_event_ext() recalculates the full MSR state shortly
after and detects the mismatch, resulting in a second wrmsrq() to restore the
missing PEBS bits, this causes an inefficient double MSR write and seems to 
defeat the goal of maintaining a consistent MSR view.

Would it be better to use a read-modify-write pattern here instead? 
For example:

cpuc->cfg_c_val[idx] = (cpuc->cfg_c_val[idx] & ~ARCH_PEBS_RELOAD) | reload;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427085513.3728672-1-dapeng1.mi@linux.intel.com?part=4