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

["Chen, Zide" <zide.chen@intel.com>]

On 5/12/2026 6:49 PM, sashiko-bot@kernel.org wrote:
> 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?

You're right, it's problematic, but not a regression — it's caused by a
pre-existing bug.

With or without this patch, intel_uncore_find_discovery_unit() could
return NULL, causing intel_generic_uncore_box_ctl() to return 0, and all
callers should check the return value before accessing hardware.

The CPU offline scenario you described is just one example of this. I'll
fix it in a separate patch.


> 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?
>