Re: [PATCH 1/3] perf/x86: Add x86_pmu::print_debug

On 8/13/2026 8:57 PM, Petr Tesarik wrote:
> On Wed, 12 Aug 2026 10:37:20 +0800
> "Mi, Dapeng" <[email protected]> wrote:
>
>> On 8/6/2026 6:03 PM, Sandipan Das wrote:
>>> perf_event_print_debug() dumps the global control and status MSRs
>>> whenever x86_pmu.version >= 2, reading registers that exist only on
>>> Intel-compatible PMUs. This is not safe since x86_pmu.version is not
>>> Intel-specific and is now set by other vendors whose global registers
>>> use different addresses.
>>>
>>> As a first step, split perf_event_print_debug() in two. The register
>>> dump moves into a new common helper, x86_pmu_print_debug(), leaving
>>> perf_event_print_debug() to handle the preamble and dispatch to an
>>> optional x86_pmu::print_debug method. This lets each vendor-specific
>>> PMU dump its own global state before chaining into the common helper.
>>> PMUs that do not implement the method, such as those with
>>> x86_pmu.version < 2, get the common helper alone.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Sandipan Das <[email protected]>
>>> ---
>>>  arch/x86/events/core.c       | 29 ++++++++++++++++++++++-------
>>>  arch/x86/events/perf_event.h |  4 ++++
>>>  2 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>>> index af0b67ffb43d..17dc53a62378 100644
>>> --- a/arch/x86/events/core.c
>>> +++ b/arch/x86/events/core.c
>>> @@ -1557,7 +1557,7 @@ static void x86_pmu_start(struct perf_event *event, int flags)
>>>  	perf_event_update_userpage(event);
>>>  }
>>>  
>>> -void perf_event_print_debug(void)
>>> +void x86_pmu_print_debug(int cpu)
>>>  {
>>>  	u64 ctrl, status, overflow, pmc_ctrl, pmc_count, prev_left, fixed;
>>>  	unsigned long *cntr_mask, *fixed_cntr_mask;
>>> @@ -1566,17 +1566,11 @@ void perf_event_print_debug(void)
>>>  	u64 pebs, debugctl;
>>>  	int cpu, idx;
>>>  
>>> -	guard(irqsave)();
>>> -
>>> -	cpu = smp_processor_id();
>>>  	cpuc = &per_cpu(cpu_hw_events, cpu);
>>>  	cntr_mask = hybrid(cpuc->pmu, cntr_mask);
>>>  	fixed_cntr_mask = hybrid(cpuc->pmu, fixed_cntr_mask);
>>>  	pebs_constraints = hybrid(cpuc->pmu, pebs_constraints);
>>>  
>>> -	if (!*(u64 *)cntr_mask)
>>> -		return;
>>> -
>>>  	if (x86_pmu.version >= 2) {
>>>  		rdmsrq(MSR_CORE_PERF_GLOBAL_CTRL, ctrl);
>>>  		rdmsrq(MSR_CORE_PERF_GLOBAL_STATUS, status);
>>> @@ -1622,6 +1616,27 @@ void perf_event_print_debug(void)
>>>  	}
>>>  }
>>>  
>>> +void perf_event_print_debug(void)
>>> +{
>>> +	struct cpu_hw_events *cpuc;
>>> +	unsigned long *cntr_mask;
>>> +	int cpu;
>>> +
>>> +	guard(irqsave)();
>>> +
>>> +	cpu = smp_processor_id();
>>> +	cpuc = &per_cpu(cpu_hw_events, cpu);
>>> +	cntr_mask = hybrid(cpuc->pmu, cntr_mask);
>>> +
>>> +	if (!*(u64 *)cntr_mask)
>>> +		return;
>>> +
>>> +	if (x86_pmu.print_debug)
>>> +		x86_pmu.print_debug(cpu);
>>> +	else
>>> +		x86_pmu_print_debug(cpu);  
>> The logic looks good, but better change this to the static_call() just like
>> other x86_pmu callbacks. It eliminates the branch prediction cost. Thanks.
> Is it worth the extra complexity for code that is used only by SysRq
> debugging?

perf_event_print_debug() is not just called in sysrq debugging, it's also
called by intel_pmu_handle_irq() which is a NMI handler. Any code in NMI
handler should be as simple as possible. Thanks.


>
> Of course, if it's trivial, let's do it anyway.
>
> Petr T