Re: [PATCH 1/6] perf/x86/intel/uncore: Fix discovery unit lookup for multi-die systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unchecked return value causes a write to MSR 0x0 when a PMON discovery unit is not found for the current die, leading to a #GP fault and kernel panic.
--

commit 618972be846a765ebeeda359a995bf217a4d825c
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Fix discovery unit lookup for multi-die systems

This commit updates intel_generic_uncore_box_ctl() to look up discovery
units using box->dieid instead of hardcoding -1. This ensures the
correct node is selected on multi-die systems.

> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 583cbd06b9b88..1d22d7c00ee06 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
> @@ -481,7 +481,7 @@ static u64 intel_generic_uncore_box_ctl(struct intel_uncore_box *box)
>  	struct intel_uncore_discovery_unit *unit;
>  
>  	unit = intel_uncore_find_discovery_unit(box->pmu->type->boxes,
> -						-1, box->pmu->pmu_idx);
> +						box->dieid, box->pmu->pmu_idx);
>  	if (WARN_ON_ONCE(!unit))
>  		return 0;

Could returning 0 here cause a regression during CPU hotplug if a die is
brought online later?

If a CPU die is offline during boot, uncore_discovery_msr() skips it,
meaning no PMON units are added to the discovery RB-tree for that dieid.

When the die is later brought online, uncore_event_cpu_online()
allocates boxes based on the globally maximum number of discovered units
and calls uncore_box_init().

The initialization function unconditionally calls
intel_generic_uncore_msr_init_box(), which writes to the returned address
without checking if it is 0:

arch/x86/events/intel/uncore_discovery.c:intel_generic_uncore_msr_init_box() {
	wrmsrq(intel_generic_uncore_box_ctl(box), GENERIC_PMON_BOX_CTL_INT);
}

Because the lookup now strictly matches box->dieid instead of safely
falling back with -1 to an identical unit on another die,
intel_uncore_find_discovery_unit() will return NULL for this die.

This results in intel_generic_uncore_box_ctl() returning 0. Calling
wrmsrq(0, ...) writes directly to MSR 0x0 (IA32_P5_MC_ADDR), which will
trigger a #GP fault and crash the system.

Similar unvalidated accesses also occur in PCI configuration space
via intel_generic_uncore_pci_init_box():

arch/x86/events/intel/uncore_discovery.c:intel_generic_uncore_pci_init_box() {
	struct pci_dev *pdev = box->pci_dev;
	int box_ctl = intel_pci_uncore_box_ctl(box);

	__set_bit(UNCORE_BOX_FLAG_CTL_OFFS8, &box->flags);
	pci_write_config_dword(pdev, box_ctl, GENERIC_PMON_BOX_CTL_INT);
}

Should the callers be updated to check the return value of
intel_generic_uncore_box_ctl() before attempting hardware accesses?

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