Re: [PATCH V2 7/7] perf/x86/intel/uncore: Implement global init callback for GNR uncore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `uncore_die_to_cpu()` without holding `cpus_read_lock()` introduces a race condition with CPU hotplug, potentially aborting PMU discovery.
- [High] Use of the unsafe `wrmsrq_on_cpu()` function with an unvalidated MSR index can trigger a General Protection Fault (#GP) and crash the kernel.
- [High] The required global initialization write is only performed during discovery and is missed upon system resume, leaving PMON units uninitialized after sleep.
- [Medium] PMU discovery permanently fails for a die if all of its CPUs are offline during module initialization.
--

commit d885055ecaea97356a710a83b6d4de7ee7d6ba76
Author: Zide Chen <zide.chen@intel.com>
Subject: perf/x86/intel/uncore: Implement global init callback for GNR uncore

This commit implements a global initialization callback for GNR uncore to
explicitly write 0 to the global control register at boot. This triggers
hardware initialization properly for UBOX PMON units on Sierra Forest and
Clearwater Forest.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 4b3a1fa5b41b7..7857959c6e823 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
[ ... ]
> @@ -1731,6 +1731,16 @@ static int uncore_mmio_global_init(u64 ctl)
>  	return 0;
>  }
>  
> +static int uncore_msr_global_init(int die, u64 msr)
> +{
> +	int cpu = uncore_die_to_cpu(die);
> +
> +	if (cpu == -1)
> +		return -ENODEV;

[Severity: Medium]
If a die has no online CPUs during module load or boot, this will return
-ENODEV and abort parsing the discovery table for that die. Since there
is no mechanism to re-trigger discovery when a CPU on that die is later
brought online via hotplug, will the uncore PMUs for that die remain
permanently unavailable?

[Severity: High]
Is this safe against CPU hotplug? uncore_discovery_pci() invokes
__parse_discovery_table() without holding cpus_read_lock(). If the
selected CPU goes offline in the narrow window between uncore_die_to_cpu()
and the subsequent wrmsrq_on_cpu() call, smp_call_function_single() will
return -ENXIO and silently abort PMU discovery for the entire die.

> +
> +	return wrmsrq_on_cpu(cpu, msr, 0);
> +}

[Severity: High]
Can this trigger a General Protection Fault (#GP)? The msr parameter is
derived from a 64-bit value dynamically read from the hardware/firmware
discovery table. If the firmware provides an invalid value, an MMIO physical
address, or if the table is corrupted, wrmsrq_on_cpu() will execute a bare
wrmsr instruction. Should this use wrmsrq_safe_on_cpu() to gracefully handle
invalid MSR indices?

[Severity: High]
Since domain->global_init() is only called once during initial uncore
discovery, what happens during S3 suspend or hibernation? The hardware drops
power and loses its state, reverting the FRZ_ALL bit back to the default.
Without a suspend/resume callback to re-trigger this explicit global
initialization write, will the UBOX PMON units silently fail to work after
the system resumes from sleep?

[ ... ]
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index af2217b44a814..e36613d934b1f 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
[ ... ]
> @@ -287,7 +287,7 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>  	if (!io_addr)
>  		return -ENOMEM;
>  
> -	if (domain->global_init && domain->global_init(global.ctl)) {
> +	if (domain->global_init && domain->global_init(die, global.ctl)) {
>  		ret = -ENODEV;
>  		goto out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527151154.130505-1-zide.chen@intel.com?part=7