Re: [PATCH v2 2/3] perf/x86: Move MSR_CORE_PERF_GLOBAL_* dump to vendor code

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unintended removal of MSR_IA32_DEBUGCTLMSR from AMD PMU debug dump.
--

--- Patch [2]: [PATCH v2 2/3] perf/x86: Move MSR_CORE_PERF_GLOBAL_* dump to vendor code ---
commit 9c3227849508af744eb4359d394119d9f9ee749a
Author: Sandipan Das <[email protected]>

perf/x86: Move MSR_CORE_PERF_GLOBAL_* dump to vendor code

This commit moves the dumping of MSR_CORE_PERF_GLOBAL_* and
MSR_ARCH_PERFMON_FIXED_CTR_CTRL registers from common code to vendor-specific
code. This prevents unchecked MSR access errors on AMD processors that report
PerfMonV2 support but lack these Intel Architectural PerfMon registers.

Closes: https://lore.kernel.org/all/[email protected]/

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index ae773f8d55c39..4c70cb0148716 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c

[ ... ]

> @@ -1561,38 +1561,15 @@ static void x86_pmu_start(struct perf_event *event, int flags)
>  
>  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;
> -	struct event_constraint *pebs_constraints;
> +	u64 pmc_ctrl, pmc_count, prev_left;
>  	struct cpu_hw_events *cpuc;
> -	u64 pebs, debugctl;
>  	int idx;
>  
>  	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 (x86_pmu.version >= 2) {
> -		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 (pebs_constraints) {
> -			rdmsrq(MSR_IA32_PEBS_ENABLE, pebs);
> -			pr_info("CPU#%d: pebs:       %016llx\n", cpu, pebs);
> -		}
> -		if (x86_pmu.lbr_nr) {
> -			rdmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
> -			pr_info("CPU#%d: debugctl:   %016llx\n", cpu, debugctl);
> -		}

[Severity: Medium]
Will this unintentionally drop the MSR_IA32_DEBUGCTLMSR dump for AMD CPUs?

AMD processors with LBR support use MSR_IA32_DEBUGCTLMSR for LBR freezing, but
since this read was moved entirely to the Intel-specific intel_pmu_print_debug(),
SysRq-P will no longer dump this register for AMD processors relying on the
common x86_pmu_print_debug() function.

Should the debugctl dump be preserved in common code, or added to an AMD-specific
print_debug function?

> -	}
> +
>  	pr_info("CPU#%d: active:     %016llx\n", cpu, *(u64 *)cpuc->active_mask);
>  
>  	for_each_set_bit(idx, cntr_mask, X86_PMC_IDX_MAX) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2