Re: [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs

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

On 10/29/2025 6:39 PM, Mi, Dapeng wrote:
> 
> On 10/30/2025 6:07 AM, Zide Chen wrote:
>> In snbep_pci2phy_map_init(), if there are more than 8 nodes,
>> uncore_device_to_die(ubox_dev) may return -1 if all CPUs
>> associated with the UBOX device are offline.  This is not an error
>> and we still need to populate map->pbus_to_dieid[].
>>
>> If NUMA is disabled on a NUMA-capable platform, pcibus_to_node()
>> returns NUMA_NO_NODE and uncore_device_to_die() returns -1.  As a
>> result, in spr_update_device_location(), which is used on Intel SPR,
>> GNR etc., the PMON units are ignored and not added to the RB tree.
> 
> It seems spr_update_device_location() currently is only called by SPR,
> please double check. 

Woops, it's used by SPR and EMR, not GNR.
> BTW, is this a SPR specific issue or a common issue? Thanks.
This is a general statement: uncore_device_to_die() can't be used when
numa=off. However, in the current code, it is misused only in
spr_update_device_location().
>>
>> Use uncore_pcibus_to_dieid() instead, which retrieves topology
>> information from the GIDNIDMAP register of the UBOX device.
>>
>> Fixes: 9a7832ce3d92 ("perf/x86/intel/uncore: With > 8 nodes, get pci bus die id from NUMA info")
>> Fixes: 65248a9a9ee1 ("perf/x86/uncore: Add a quirk for UPI on SPR")
>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>> ---
>>  arch/x86/events/intel/uncore.c       |  1 +
>>  arch/x86/events/intel/uncore_snbep.c | 13 ++++++-------
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index 5c3aeea5c78d..84d6e481f18f 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
>> @@ -77,6 +77,7 @@ int uncore_die_to_segment(int die)
>>  	return bus ? pci_domain_nr(bus) : -EINVAL;
>>  }
>>  
>> +/* Note: This API can only be used when NUMA information is available. */
>>  int uncore_device_to_die(struct pci_dev *dev)
>>  {
>>  	int node = pcibus_to_node(dev->bus);
>> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
>> index ad4d11762ecf..e68467f617f9 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -1471,13 +1471,7 @@ static int snbep_pci2phy_map_init(int devid, int nodeid_loc, int idmap_loc, bool
>>  			}
>>  
>>  			map->pbus_to_dieid[bus] = die_id = uncore_device_to_die(ubox_dev);
>> -
>>  			raw_spin_unlock(&pci2phy_map_lock);
>> -
>> -			if (WARN_ON_ONCE(die_id == -1)) {
>> -				err = -EINVAL;
>> -				break;
>> -			}
>>  		}
>>  	}
>>  
>> @@ -6530,7 +6524,7 @@ static void spr_update_device_location(int type_id)
>>  
>>  	while ((dev = pci_get_device(PCI_VENDOR_ID_INTEL, device, dev)) != NULL) {
>>  
>> -		die = uncore_device_to_die(dev);
>> +		die = uncore_pcibus_to_dieid(dev->bus);
>>  		if (die < 0)
>>  			continue;
>>  
>> @@ -6554,6 +6548,11 @@ static void spr_update_device_location(int type_id)
>>  
>>  int spr_uncore_pci_init(void)
>>  {
>> +	int ret = snbep_pci2phy_map_init(0x3250, SKX_CPUNODEID, SKX_GIDNIDMAP, true);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>>  	/*
>>  	 * The discovery table of UPI on some SPR variant is broken,
>>  	 * which impacts the detection of both UPI and M3UPI uncore PMON.