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

[Sandipan Das <sandipan.das@amd.com>]

On 14-08-2026 23:31, sashiko-bot@kernel.org wrote:
> 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 <sandipan.das@amd.com>
> 
> 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/20260717150315.2868314-1-ptesarik@suse.com/
> 
>> 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?
> 

If the intent is to just see if LBR is enabled, then amd_pmu_v2_print_debug()
already does that by dumping MSR_AMD_DBG_EXTN_CFG. I am not sure it matters
here whether freeze is enabled as well.

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