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

[Petr Tesarik]

On Wed, 12 Aug 2026 10:37:20 +0800
"Mi, Dapeng" <dapeng1.mi@xxxxxxxxxxxxxxx> 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 <sandipan.das@xxxxxxx>
> > ---
> >  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?

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

Petr T