Re: [PATCH 1/1] perf/x86: Do not read fixed counter control register if there is none

On 2026/7/17 23:51, Petr Tesarik wrote:
> On Fri, 17 Jul 2026 15:18:58 +0000
> [email protected] wrote:
> 
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>
>> New issues:
>> - [Medium] Using `x86_pmu.fixedctr` as a capability flag is conceptually flawed and suppresses valid debug output on Zhaoxin.
>>
>> Pre-existing issues:
>> - [High] Zhaoxin PMU fails to initialize `x86_pmu.fixedctr`, leading to broken fixed counters and unsafe RDMSR accesses.
>> - [High] Undefined behavior in `GENMASK_ULL` during Zhaoxin PMU initialization if fixed counters are missing.
>> --
>>
>> --- Patch [1]: [PATCH 1/1] perf/x86: Do not read fixed counter control register if there is none ---
>> commit d0dd53a6709a65141e2d105b19abad4d851825d4
>> Author: Petr Tesarik <[email protected]>
>>
>> perf/x86: Do not read fixed counter control register if there is none
>>
>> This patch skips reading the MSR_ARCH_PERFMON_FIXED_CTR_CTRL register
>> during perf debug output if fixed counters are not implemented. This avoids
>> a #GP on AMD CPUs with PerfMonV2 that do not implement this register.
>>
>>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>>> index 576baa9a52c5b..3055d59e0e82f 100644
>>> --- a/arch/x86/events/core.c
>>> +++ b/arch/x86/events/core.c
>>> @@ -1572,13 +1572,16 @@ void perf_event_print_debug(void)
>>>   		rdmsrq(MSR_CORE_PERF_GLOBAL_CTRL, ctrl);
>>>   		rdmsrq(MSR_CORE_PERF_GLOBAL_STATUS, status);
>>>   		rdmsrq(MSR_CORE_PERF_GLOBAL_OVF_CTRL, overflow);
>>> -		rdmsrq(MSR_ARCH_PERFMON_FIXED_CTR_CTRL, fixed);
>>>   
>>>   		pr_info("\n");
>>>   		pr_info("CPU#%d: ctrl:       %016llx\n", cpu, ctrl);
>>>   		pr_info("CPU#%d: status:     %016llx\n", cpu, status);
>>>   		pr_info("CPU#%d: overflow:   %016llx\n", cpu, overflow);
>>> -		pr_info("CPU#%d: fixed:      %016llx\n", cpu, fixed);
>>> +
>>> +		if (x86_pmu.fixedctr) {
>>
>> [Severity: Medium]
>> Is it safe to use x86_pmu.fixedctr as a capability flag here?
> 
> Good question. In fact, I thought about it, and yes, introducing a new
> flag would be the cleanest solution, but it seemed overkill just for
> this debugging output.
> 
> AFAIK the MSR offset of the fixed perf counters cannot be zero, because
> those MSR addresses have been assigned to other architectural MSRs since
> Pentium times...
> 
>> The x86_pmu.fixedctr field is an MSR address, not a boolean capability
>> flag. While this check works for most architectures, the Zhaoxin PMU driver
>> fails to initialize this field, leaving it as 0. Because of this, the check
>> evaluates to false and inadvertently suppresses the debug output of
>> MSR_ARCH_PERFMON_FIXED_CTR_CTRL on Zhaoxin systems.
> 
> I don't know about Zhaoxin. Do these syste have fixed performance
> counters and a corresponding global control register?
> 
Yes, Zhaoxin system have fixed performance counters and a corresponding 
global control register.

> Adding Tony W Wang to the recipient list...
> 
> Petr T
> 
> --
> 
>>> +			rdmsrq(MSR_ARCH_PERFMON_FIXED_CTR_CTRL,
>>> fixed);
>>> +			pr_info("CPU#%d: fixed:      %016llx\n",
>>> cpu, fixed);
>>> +		}
>>>   		if (pebs_constraints) {
>>>   			rdmsrq(MSR_IA32_PEBS_ENABLE, pebs);
>>>   			pr_info("CPU#%d: pebs:       %016llx\n",
>>> cpu, pebs);
>> [ ... ]
>>
>>
>> [Severity: High]
>> This is a pre-existing issue and was not introduced by the patch under
>> review, but the lack of initialization for x86_pmu.fixedctr causes
>> other issues.
>>
>> In the Zhaoxin PMU driver setup:
>>
>> arch/x86/events/zhaoxin/core.c:zhaoxin_pmu
>> static const struct x86_pmu zhaoxin_pmu __initconst = {
>>      ...
>>      /* missing .fixedctr = MSR_ARCH_PERFMON_FIXED_CTR0 */
>> };
>>
This issue has indeed resulted in an “perf -e instructions” bug on 
Zhaoxin processors. Although Tanze submitted a patch in 2025, it seems 
it has not yet been merged to mainstream.
https://lkml.org/lkml/2025/6/19/141


>> Because this remains 0, x86_pmu_fixed_ctr_addr() returns 0 + idx
>> instead of the correct MSR. This causes fixed counter 0 to silently
>> return without updating in x86_perf_event_update():
>>
>> arch/x86/events/core.c:x86_perf_event_update() {
>>      ...
>>      if (unlikely(!hwc->event_base))
>>          return 0;
>>      ...
>> }
>>
>> It also leads to unsafe MSR reads when iterating over the fixed
>> counter mask in perf_event_print_debug():
>>
>> arch/x86/events/core.c:perf_event_print_debug() {
>>      ...
>>      for_each_set_bit(idx, fixed_cntr_mask, X86_PMC_IDX_MAX) {
>>          ...
>>          rdmsrq(x86_pmu_fixed_ctr_addr(idx), pmc_count);
>>          ...
>>      }
>> }
>>
>> Could this lead to reads from arbitrary low MSRs like MSR 0, 1, or 2,
>> potentially triggering a system crash if a user programs fixed counter
>> events or triggers SysRq-P?
>>
>>
>> [Severity: High]
>> This is a pre-existing issue and was not introduced by the patch under
>> review, but does this bitmask generation in the Zhaoxin PMU
>> initialization risk undefined behavior?
>>
>> arch/x86/events/zhaoxin/core.c:zhaoxin_pmu_init() {
>>      ...
>>      x86_pmu.fixed_cntr_mask64 =
>> GENMASK_ULL(edx.split.num_counters_fixed - 1, 0); ...
>> }
>>
>> If a hypervisor configures the CPU to report 0 fixed counters,
>> edx.split.num_counters_fixed - 1 wraps to -1. Expanding
>> GENMASK_ULL(-1, 0) results in a right-shift by 64, which is undefined
>> behavior in C. On x86, this yields ~0ULL, making the kernel assume 64
>> fixed counters exist.
>>
>> Could this cause the kernel to attempt reading 64 undefined MSRs
>> during debug output or event initialization, leading to #GP
>> exceptions?
>>
This issue may exist. Can the code be changed as follows?

if (edx.split.num_counters_fixed == 0) {
     x86_pmu.fixed_cntr_mask64 = 0;
} else {
     x86_pmu.fixed_cntr_mask64 = 
GENMASK_ULL(edx.split.num_counters_fixed - 1, 0);
}


In addition, this issue also appears to exist in the Intel PMU driver, 
should we fix them together?

Sincerely!
TonyWWang-oc