[PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Dapeng Mi]

When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
 access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.

[   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
[   55.611280] Call Trace:
[   55.611282]  <TASK>
[   55.611284]  ? intel_pmu_config_acr+0x87/0x160
[   55.611289]  intel_pmu_enable_acr+0x6d/0x80
[   55.611291]  intel_pmu_enable_event+0xce/0x460
[   55.611293]  x86_pmu_start+0x78/0xb0
[   55.611297]  x86_pmu_enable+0x218/0x3a0
[   55.611300]  ? x86_pmu_enable+0x121/0x3a0
[   55.611302]  perf_pmu_enable+0x40/0x50
[   55.611307]  ctx_resched+0x19d/0x220
[   55.611309]  __perf_install_in_context+0x284/0x2f0
[   55.611311]  ? __pfx_remote_function+0x10/0x10
[   55.611314]  remote_function+0x52/0x70
[   55.611317]  ? __pfx_remote_function+0x10/0x10
[   55.611319]  generic_exec_single+0x84/0x150
[   55.611323]  smp_call_function_single+0xc5/0x1a0
[   55.611326]  ? __pfx_remote_function+0x10/0x10
[   55.611329]  perf_install_in_context+0xd1/0x1e0
[   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
[   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
[   55.611336]  __x64_sys_perf_event_open+0x26/0x30
[   55.611337]  x64_sys_call+0x1d8e/0x20c0
[   55.611339]  do_syscall_64+0x4f/0x120
[   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
which requires to enable auto counter reload on GP counter 0.

The root cause of causing this issue is the check for auto counter
reload (ACR) counter mask from user space is incorrect in
intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
mask from user space could be set into hw.config1 and then written into
CFG_B MSRs and trigger the MSR access warning.

e.g., User may create a perf event with ACR counter mask (config2=0xcb),
and there is only 1 event created, so "cpuc->n_events" is 1.

The correct check condition should be "i + idx >= cpuc->n_events"
instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
the counter mask would traverse twice and an invalid "cpuc->assign[1]"
bit (bit 0) is set into hw.config1 and cause MSR accessing error.

Besides, also check if the ACR counter mask corresponding events are
ACR events. If not, filter out these counter mask. If a event is not a
ACR event, it could be scheduled to an HW counter which doesn't support
ACR. It's invalid to add their counter index in ACR counter mask.

Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
user could set any invalid ACR counter mask and the warning message
could mislead users.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 3a319cf6d364..8d046b1a237e 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
 			if (event->group_leader != leader->group_leader)
 				break;
 			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
-				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
+				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);
 			}

base-commit: e7d952cc39fca34386ec9f15f68cb2eaac01b5ae
-- 
2.43.0

[PATCH 2/2] perf/x86/intel: Fix wrong index calculation in intel_pmu_config_acr()

[Dapeng Mi]

To calculate fixed counter MSR address, the HW counter index "idx" is
subtracted by INTEL_PMC_IDX_FIXED. It leads to the ACR mask value of
fixed counters is incorrectly saved to the positions of GP counters
in acr_cfg_b[], e.g. For fixed counter 0, its ACR counter mask should be
saved to acr_cfg_b[32], but it's saved to acr_cfg_b[0] incorrectly.

Fix this issue.

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 8d046b1a237e..b0fee684ec8c 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2899,6 +2899,7 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	int msr_b, msr_c;
+	int msr_offset;
 
 	if (!mask && !cpuc->acr_cfg_b[idx])
 		return;
@@ -2906,19 +2907,20 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
 	if (idx < INTEL_PMC_IDX_FIXED) {
 		msr_b = MSR_IA32_PMC_V6_GP0_CFG_B;
 		msr_c = MSR_IA32_PMC_V6_GP0_CFG_C;
+		msr_offset = x86_pmu.addr_offset(idx, false);
 	} else {
 		msr_b = MSR_IA32_PMC_V6_FX0_CFG_B;
 		msr_c = MSR_IA32_PMC_V6_FX0_CFG_C;
-		idx -= INTEL_PMC_IDX_FIXED;
+		msr_offset = x86_pmu.addr_offset(idx - INTEL_PMC_IDX_FIXED, false);
 	}
 
 	if (cpuc->acr_cfg_b[idx] != mask) {
-		wrmsrl(msr_b + x86_pmu.addr_offset(idx, false), mask);
+		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) {
-		wrmsrl(msr_c + x86_pmu.addr_offset(idx, false), reload);
+		wrmsrl(msr_c + msr_offset, reload);
 		cpuc->acr_cfg_c[idx] = reload;
 	}
 }
-- 
2.43.0

Re: [PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Liang, Kan]

On 2025-05-29 4:02 a.m., Dapeng Mi wrote:
> When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
>  access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.
> 
> [   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
> [   55.611280] Call Trace:
> [   55.611282]  <TASK>
> [   55.611284]  ? intel_pmu_config_acr+0x87/0x160
> [   55.611289]  intel_pmu_enable_acr+0x6d/0x80
> [   55.611291]  intel_pmu_enable_event+0xce/0x460
> [   55.611293]  x86_pmu_start+0x78/0xb0
> [   55.611297]  x86_pmu_enable+0x218/0x3a0
> [   55.611300]  ? x86_pmu_enable+0x121/0x3a0
> [   55.611302]  perf_pmu_enable+0x40/0x50
> [   55.611307]  ctx_resched+0x19d/0x220
> [   55.611309]  __perf_install_in_context+0x284/0x2f0
> [   55.611311]  ? __pfx_remote_function+0x10/0x10
> [   55.611314]  remote_function+0x52/0x70
> [   55.611317]  ? __pfx_remote_function+0x10/0x10
> [   55.611319]  generic_exec_single+0x84/0x150
> [   55.611323]  smp_call_function_single+0xc5/0x1a0
> [   55.611326]  ? __pfx_remote_function+0x10/0x10
> [   55.611329]  perf_install_in_context+0xd1/0x1e0
> [   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
> [   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
> [   55.611336]  __x64_sys_perf_event_open+0x26/0x30
> [   55.611337]  x64_sys_call+0x1d8e/0x20c0
> [   55.611339]  do_syscall_64+0x4f/0x120
> [   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
> thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
> which requires to enable auto counter reload on GP counter 0.
> 
> The root cause of causing this issue is the check for auto counter
> reload (ACR) counter mask from user space is incorrect in
> intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
> mask from user space could be set into hw.config1 and then written into
> CFG_B MSRs and trigger the MSR access warning.
> 
> e.g., User may create a perf event with ACR counter mask (config2=0xcb),
> and there is only 1 event created, so "cpuc->n_events" is 1.
> 
> The correct check condition should be "i + idx >= cpuc->n_events"
> instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
> the counter mask would traverse twice and an invalid "cpuc->assign[1]"
> bit (bit 0) is set into hw.config1 and cause MSR accessing error.
> 
> Besides, also check if the ACR counter mask corresponding events are
> ACR events. If not, filter out these counter mask. If a event is not a
> ACR event, it could be scheduled to an HW counter which doesn't support
> ACR. It's invalid to add their counter index in ACR counter mask.
> 
> Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
> user could set any invalid ACR counter mask and the warning message
> could mislead users.
> 
> Fixes: ec980e4facef ("perf/x86/intel: Support auto counter reload")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Thanks for the fix.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> ---
>  arch/x86/events/intel/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 3a319cf6d364..8d046b1a237e 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>  			if (event->group_leader != leader->group_leader)
>  				break;
>  			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> -				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
> +				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);
>  			}
> 
> base-commit: e7d952cc39fca34386ec9f15f68cb2eaac01b5ae

Re: [PATCH 2/2] perf/x86/intel: Fix wrong index calculation in intel_pmu_config_acr()

[Liang, Kan]

On 2025-05-29 4:02 a.m., Dapeng Mi wrote:
> To calculate fixed counter MSR address, the HW counter index "idx" is
> subtracted by INTEL_PMC_IDX_FIXED. It leads to the ACR mask value of
> fixed counters is incorrectly saved to the positions of GP counters
> in acr_cfg_b[], e.g. For fixed counter 0, its ACR counter mask should be
> saved to acr_cfg_b[32], but it's saved to acr_cfg_b[0] incorrectly.
> 
> Fix this issue.
> 
> Fixes: ec980e4facef ("perf/x86/intel: Support auto counter reload")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> ---
>  arch/x86/events/intel/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 8d046b1a237e..b0fee684ec8c 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2899,6 +2899,7 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	int msr_b, msr_c;
> +	int msr_offset;
>  
>  	if (!mask && !cpuc->acr_cfg_b[idx])
>  		return;
> @@ -2906,19 +2907,20 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
>  	if (idx < INTEL_PMC_IDX_FIXED) {
>  		msr_b = MSR_IA32_PMC_V6_GP0_CFG_B;
>  		msr_c = MSR_IA32_PMC_V6_GP0_CFG_C;
> +		msr_offset = x86_pmu.addr_offset(idx, false);
>  	} else {
>  		msr_b = MSR_IA32_PMC_V6_FX0_CFG_B;
>  		msr_c = MSR_IA32_PMC_V6_FX0_CFG_C;
> -		idx -= INTEL_PMC_IDX_FIXED;
> +		msr_offset = x86_pmu.addr_offset(idx - INTEL_PMC_IDX_FIXED, false);
>  	}
>  
>  	if (cpuc->acr_cfg_b[idx] != mask) {
> -		wrmsrl(msr_b + x86_pmu.addr_offset(idx, false), mask);
> +		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) {
> -		wrmsrl(msr_c + x86_pmu.addr_offset(idx, false), reload);
> +		wrmsrl(msr_c + msr_offset, reload);
>  		cpuc->acr_cfg_c[idx] = reload;
>  	}
>  }

Re: [PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Ingo Molnar]

* Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:

> When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
>  access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.
> 
> [   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
> [   55.611280] Call Trace:
> [   55.611282]  <TASK>
> [   55.611284]  ? intel_pmu_config_acr+0x87/0x160
> [   55.611289]  intel_pmu_enable_acr+0x6d/0x80
> [   55.611291]  intel_pmu_enable_event+0xce/0x460
> [   55.611293]  x86_pmu_start+0x78/0xb0
> [   55.611297]  x86_pmu_enable+0x218/0x3a0
> [   55.611300]  ? x86_pmu_enable+0x121/0x3a0
> [   55.611302]  perf_pmu_enable+0x40/0x50
> [   55.611307]  ctx_resched+0x19d/0x220
> [   55.611309]  __perf_install_in_context+0x284/0x2f0
> [   55.611311]  ? __pfx_remote_function+0x10/0x10
> [   55.611314]  remote_function+0x52/0x70
> [   55.611317]  ? __pfx_remote_function+0x10/0x10
> [   55.611319]  generic_exec_single+0x84/0x150
> [   55.611323]  smp_call_function_single+0xc5/0x1a0
> [   55.611326]  ? __pfx_remote_function+0x10/0x10
> [   55.611329]  perf_install_in_context+0xd1/0x1e0
> [   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
> [   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
> [   55.611336]  __x64_sys_perf_event_open+0x26/0x30
> [   55.611337]  x64_sys_call+0x1d8e/0x20c0
> [   55.611339]  do_syscall_64+0x4f/0x120
> [   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
> thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
> which requires to enable auto counter reload on GP counter 0.
> 
> The root cause of causing this issue is the check for auto counter
> reload (ACR) counter mask from user space is incorrect in
> intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
> mask from user space could be set into hw.config1 and then written into
> CFG_B MSRs and trigger the MSR access warning.
> 
> e.g., User may create a perf event with ACR counter mask (config2=0xcb),
> and there is only 1 event created, so "cpuc->n_events" is 1.
> 
> The correct check condition should be "i + idx >= cpuc->n_events"
> instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
> the counter mask would traverse twice and an invalid "cpuc->assign[1]"
> bit (bit 0) is set into hw.config1 and cause MSR accessing error.
> 
> Besides, also check if the ACR counter mask corresponding events are
> ACR events. If not, filter out these counter mask. If a event is not a
> ACR event, it could be scheduled to an HW counter which doesn't support
> ACR. It's invalid to add their counter index in ACR counter mask.
> 
> Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
> user could set any invalid ACR counter mask and the warning message
> could mislead users.
> 
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 3a319cf6d364..8d046b1a237e 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>  			if (event->group_leader != leader->group_leader)
>  				break;
>  			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> -				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
> +				if (i + idx >= cpuc->n_events ||
> +				    !is_acr_event_group(cpuc->event_list[i + idx]))
>  					return;

Is this a normal condition?

 - If it's normal then the 'return' is destructive, isn't it? Shouldn't 
   it be a 'break', if this condition is legitimate?

 - If it's not normal, then the WARN_ON_ONCE() was justified, right?

Thanks,

	Ingo

Re: [PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Mi, Dapeng]

On 5/31/2025 4:01 PM, Ingo Molnar wrote:
> * Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
>> When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
>>  access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.
>>
>> [   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
>> [   55.611280] Call Trace:
>> [   55.611282]  <TASK>
>> [   55.611284]  ? intel_pmu_config_acr+0x87/0x160
>> [   55.611289]  intel_pmu_enable_acr+0x6d/0x80
>> [   55.611291]  intel_pmu_enable_event+0xce/0x460
>> [   55.611293]  x86_pmu_start+0x78/0xb0
>> [   55.611297]  x86_pmu_enable+0x218/0x3a0
>> [   55.611300]  ? x86_pmu_enable+0x121/0x3a0
>> [   55.611302]  perf_pmu_enable+0x40/0x50
>> [   55.611307]  ctx_resched+0x19d/0x220
>> [   55.611309]  __perf_install_in_context+0x284/0x2f0
>> [   55.611311]  ? __pfx_remote_function+0x10/0x10
>> [   55.611314]  remote_function+0x52/0x70
>> [   55.611317]  ? __pfx_remote_function+0x10/0x10
>> [   55.611319]  generic_exec_single+0x84/0x150
>> [   55.611323]  smp_call_function_single+0xc5/0x1a0
>> [   55.611326]  ? __pfx_remote_function+0x10/0x10
>> [   55.611329]  perf_install_in_context+0xd1/0x1e0
>> [   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
>> [   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
>> [   55.611336]  __x64_sys_perf_event_open+0x26/0x30
>> [   55.611337]  x64_sys_call+0x1d8e/0x20c0
>> [   55.611339]  do_syscall_64+0x4f/0x120
>> [   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
>> thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
>> which requires to enable auto counter reload on GP counter 0.
>>
>> The root cause of causing this issue is the check for auto counter
>> reload (ACR) counter mask from user space is incorrect in
>> intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
>> mask from user space could be set into hw.config1 and then written into
>> CFG_B MSRs and trigger the MSR access warning.
>>
>> e.g., User may create a perf event with ACR counter mask (config2=0xcb),
>> and there is only 1 event created, so "cpuc->n_events" is 1.
>>
>> The correct check condition should be "i + idx >= cpuc->n_events"
>> instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
>> the counter mask would traverse twice and an invalid "cpuc->assign[1]"
>> bit (bit 0) is set into hw.config1 and cause MSR accessing error.
>>
>> Besides, also check if the ACR counter mask corresponding events are
>> ACR events. If not, filter out these counter mask. If a event is not a
>> ACR event, it could be scheduled to an HW counter which doesn't support
>> ACR. It's invalid to add their counter index in ACR counter mask.
>>
>> Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
>> user could set any invalid ACR counter mask and the warning message
>> could mislead users.
>>
>> 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 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 3a319cf6d364..8d046b1a237e 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>>  			if (event->group_leader != leader->group_leader)
>>  				break;
>>  			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
>> -				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
>> +				if (i + idx >= cpuc->n_events ||
>> +				    !is_acr_event_group(cpuc->event_list[i + idx]))
>>  					return;
> Is this a normal condition?
>
>  - If it's normal then the 'return' is destructive, isn't it? Shouldn't 
>    it be a 'break', if this condition is legitimate?
>
>  - If it's not normal, then the WARN_ON_ONCE() was justified, right?

It's not normal.Strictly speaking, it's an invalid user configuration. It
looks not reasonable to trigger a kernel space warning for an invalid user
space configuration. It would mislead users and let users think there is
something wrong in kernel. That's why to remove the WARN_ON_ONCE(). Thanks.


>
> Thanks,
>
> 	Ingo

Re: [PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Mi, Dapeng]

Kindly ping...    Thanks.

On 6/3/2025 10:14 AM, Mi, Dapeng wrote:
> On 5/31/2025 4:01 PM, Ingo Molnar wrote:
>> * Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>>
>>> When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
>>>  access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.
>>>
>>> [   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
>>> [   55.611280] Call Trace:
>>> [   55.611282]  <TASK>
>>> [   55.611284]  ? intel_pmu_config_acr+0x87/0x160
>>> [   55.611289]  intel_pmu_enable_acr+0x6d/0x80
>>> [   55.611291]  intel_pmu_enable_event+0xce/0x460
>>> [   55.611293]  x86_pmu_start+0x78/0xb0
>>> [   55.611297]  x86_pmu_enable+0x218/0x3a0
>>> [   55.611300]  ? x86_pmu_enable+0x121/0x3a0
>>> [   55.611302]  perf_pmu_enable+0x40/0x50
>>> [   55.611307]  ctx_resched+0x19d/0x220
>>> [   55.611309]  __perf_install_in_context+0x284/0x2f0
>>> [   55.611311]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611314]  remote_function+0x52/0x70
>>> [   55.611317]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611319]  generic_exec_single+0x84/0x150
>>> [   55.611323]  smp_call_function_single+0xc5/0x1a0
>>> [   55.611326]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611329]  perf_install_in_context+0xd1/0x1e0
>>> [   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
>>> [   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
>>> [   55.611336]  __x64_sys_perf_event_open+0x26/0x30
>>> [   55.611337]  x64_sys_call+0x1d8e/0x20c0
>>> [   55.611339]  do_syscall_64+0x4f/0x120
>>> [   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>
>>> On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
>>> thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
>>> which requires to enable auto counter reload on GP counter 0.
>>>
>>> The root cause of causing this issue is the check for auto counter
>>> reload (ACR) counter mask from user space is incorrect in
>>> intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
>>> mask from user space could be set into hw.config1 and then written into
>>> CFG_B MSRs and trigger the MSR access warning.
>>>
>>> e.g., User may create a perf event with ACR counter mask (config2=0xcb),
>>> and there is only 1 event created, so "cpuc->n_events" is 1.
>>>
>>> The correct check condition should be "i + idx >= cpuc->n_events"
>>> instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
>>> the counter mask would traverse twice and an invalid "cpuc->assign[1]"
>>> bit (bit 0) is set into hw.config1 and cause MSR accessing error.
>>>
>>> Besides, also check if the ACR counter mask corresponding events are
>>> ACR events. If not, filter out these counter mask. If a event is not a
>>> ACR event, it could be scheduled to an HW counter which doesn't support
>>> ACR. It's invalid to add their counter index in ACR counter mask.
>>>
>>> Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
>>> user could set any invalid ACR counter mask and the warning message
>>> could mislead users.
>>>
>>> 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 | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>> index 3a319cf6d364..8d046b1a237e 100644
>>> --- a/arch/x86/events/intel/core.c
>>> +++ b/arch/x86/events/intel/core.c
>>> @@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>>>  			if (event->group_leader != leader->group_leader)
>>>  				break;
>>>  			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
>>> -				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
>>> +				if (i + idx >= cpuc->n_events ||
>>> +				    !is_acr_event_group(cpuc->event_list[i + idx]))
>>>  					return;
>> Is this a normal condition?
>>
>>  - If it's normal then the 'return' is destructive, isn't it? Shouldn't 
>>    it be a 'break', if this condition is legitimate?
>>
>>  - If it's not normal, then the WARN_ON_ONCE() was justified, right?
> It's not normal.Strictly speaking, it's an invalid user configuration. It
> looks not reasonable to trigger a kernel space warning for an invalid user
> space configuration. It would mislead users and let users think there is
> something wrong in kernel. That's why to remove the WARN_ON_ONCE(). Thanks.
>
>
>> Thanks,
>>
>> 	Ingo

Re: [PATCH 1/2] perf/x86/intel: Fix IA32_PMC_x_CFG_B MSRs access error

[Mi, Dapeng]

Kindly ping.

@Ingo @Peter, not sure if you have bandwidth to review this change? Thanks.

On 6/3/2025 10:14 AM, Mi, Dapeng wrote:
> On 5/31/2025 4:01 PM, Ingo Molnar wrote:
>> * Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>>
>>> When running perf_fuzzer on PTL, sometimes the below "unchecked MSR
>>>  access error" is seen when accessing IA32_PMC_x_CFG_B MSRs.
>>>
>>> [   55.611268] unchecked MSR access error: WRMSR to 0x1986 (tried to write 0x0000000200000001) at rIP: 0xffffffffac564b28 (native_write_msr+0x8/0x30)
>>> [   55.611280] Call Trace:
>>> [   55.611282]  <TASK>
>>> [   55.611284]  ? intel_pmu_config_acr+0x87/0x160
>>> [   55.611289]  intel_pmu_enable_acr+0x6d/0x80
>>> [   55.611291]  intel_pmu_enable_event+0xce/0x460
>>> [   55.611293]  x86_pmu_start+0x78/0xb0
>>> [   55.611297]  x86_pmu_enable+0x218/0x3a0
>>> [   55.611300]  ? x86_pmu_enable+0x121/0x3a0
>>> [   55.611302]  perf_pmu_enable+0x40/0x50
>>> [   55.611307]  ctx_resched+0x19d/0x220
>>> [   55.611309]  __perf_install_in_context+0x284/0x2f0
>>> [   55.611311]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611314]  remote_function+0x52/0x70
>>> [   55.611317]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611319]  generic_exec_single+0x84/0x150
>>> [   55.611323]  smp_call_function_single+0xc5/0x1a0
>>> [   55.611326]  ? __pfx_remote_function+0x10/0x10
>>> [   55.611329]  perf_install_in_context+0xd1/0x1e0
>>> [   55.611331]  ? __pfx___perf_install_in_context+0x10/0x10
>>> [   55.611333]  __do_sys_perf_event_open+0xa76/0x1040
>>> [   55.611336]  __x64_sys_perf_event_open+0x26/0x30
>>> [   55.611337]  x64_sys_call+0x1d8e/0x20c0
>>> [   55.611339]  do_syscall_64+0x4f/0x120
>>> [   55.611343]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>
>>> On PTL, GP counter 0 and 1 doesn't support auto counter reload feature,
>>> thus it would trigger a #GP when trying to write 1 on bit 0 of CFG_B MSR
>>> which requires to enable auto counter reload on GP counter 0.
>>>
>>> The root cause of causing this issue is the check for auto counter
>>> reload (ACR) counter mask from user space is incorrect in
>>> intel_pmu_acr_late_setup() helper. It leads to an invalid ACR counter
>>> mask from user space could be set into hw.config1 and then written into
>>> CFG_B MSRs and trigger the MSR access warning.
>>>
>>> e.g., User may create a perf event with ACR counter mask (config2=0xcb),
>>> and there is only 1 event created, so "cpuc->n_events" is 1.
>>>
>>> The correct check condition should be "i + idx >= cpuc->n_events"
>>> instead of "i + idx > cpuc->n_events" (it looks a typo). Otherwise,
>>> the counter mask would traverse twice and an invalid "cpuc->assign[1]"
>>> bit (bit 0) is set into hw.config1 and cause MSR accessing error.
>>>
>>> Besides, also check if the ACR counter mask corresponding events are
>>> ACR events. If not, filter out these counter mask. If a event is not a
>>> ACR event, it could be scheduled to an HW counter which doesn't support
>>> ACR. It's invalid to add their counter index in ACR counter mask.
>>>
>>> Furthermore, remove the WARN_ON_ONCE() since it's easily triggered as
>>> user could set any invalid ACR counter mask and the warning message
>>> could mislead users.
>>>
>>> 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 | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>> index 3a319cf6d364..8d046b1a237e 100644
>>> --- a/arch/x86/events/intel/core.c
>>> +++ b/arch/x86/events/intel/core.c
>>> @@ -2994,7 +2994,8 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>>>  			if (event->group_leader != leader->group_leader)
>>>  				break;
>>>  			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
>>> -				if (WARN_ON_ONCE(i + idx > cpuc->n_events))
>>> +				if (i + idx >= cpuc->n_events ||
>>> +				    !is_acr_event_group(cpuc->event_list[i + idx]))
>>>  					return;
>> Is this a normal condition?
>>
>>  - If it's normal then the 'return' is destructive, isn't it? Shouldn't 
>>    it be a 'break', if this condition is legitimate?
>>
>>  - If it's not normal, then the WARN_ON_ONCE() was justified, right?
> It's not normal.Strictly speaking, it's an invalid user configuration. It
> looks not reasonable to trigger a kernel space warning for an invalid user
> space configuration. It would mislead users and let users think there is
> something wrong in kernel. That's why to remove the WARN_ON_ONCE(). Thanks.
>
>
>> Thanks,
>>
>> 	Ingo