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

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

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@amd.com>
> ---
>  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.


> +}
> +
>  void x86_pmu_stop(struct perf_event *event, int flags)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index a8afea8d38f0..465bf513bb82 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1052,6 +1052,8 @@ struct x86_pmu {
>  	int				num_hybrid_pmus;
>  	struct x86_hybrid_pmu		*hybrid_pmu;
>  	enum intel_cpu_type (*get_hybrid_cpu_type)	(void);
> +
> +	void (*print_debug)(int cpu);
>  };
>  
>  struct x86_perf_task_context_opt {
> @@ -1316,6 +1318,8 @@ int x86_pmu_handle_irq(struct pt_regs *regs);
>  
>  void x86_pmu_show_pmu_cap(struct pmu *pmu);
>  
> +void x86_pmu_print_debug(int cpu);
> +
>  static inline int x86_pmu_num_counters(struct pmu *pmu)
>  {
>  	return hweight64(hybrid(pmu, cntr_mask64));