* [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs
2025-10-29 22:07 [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Zide Chen
@ 2025-10-29 22:07 ` Zide Chen
2025-10-30 1:39 ` Mi, Dapeng
2025-11-13 22:33 ` Steve Wahl
2025-10-30 1:37 ` [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Mi, Dapeng
2025-11-13 22:32 ` Steve Wahl
2 siblings, 2 replies; 9+ messages in thread
From: Zide Chen @ 2025-10-29 22:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen, Xudong Hao,
Falcon Thomas, Steve Wahl
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.
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.
--
2.51.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs
2025-10-29 22:07 ` [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs Zide Chen
@ 2025-10-30 1:39 ` Mi, Dapeng
2025-10-30 5:17 ` Chen, Zide
2025-11-13 22:33 ` Steve Wahl
1 sibling, 1 reply; 9+ messages in thread
From: Mi, Dapeng @ 2025-10-30 1:39 UTC (permalink / raw)
To: Zide Chen, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Xudong Hao, Falcon Thomas,
Steve Wahl
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.
BTW, is this a SPR specific issue or a common issue? Thanks.
>
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs
2025-10-30 1:39 ` Mi, Dapeng
@ 2025-10-30 5:17 ` Chen, Zide
0 siblings, 0 replies; 9+ messages in thread
From: Chen, Zide @ 2025-10-30 5:17 UTC (permalink / raw)
To: Mi, Dapeng, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Xudong Hao, Falcon Thomas,
Steve Wahl
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs
2025-10-29 22:07 ` [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs Zide Chen
2025-10-30 1:39 ` Mi, Dapeng
@ 2025-11-13 22:33 ` Steve Wahl
1 sibling, 0 replies; 9+ messages in thread
From: Steve Wahl @ 2025-11-13 22:33 UTC (permalink / raw)
To: Zide Chen
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane, linux-kernel, linux-perf-users,
Dapeng Mi, Xudong Hao, Falcon Thomas, Steve Wahl
Tested-by: Steve Wahl <steve.wahl@hpe.com>
On Wed, Oct 29, 2025 at 03:07:11PM -0700, 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.
>
> 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.
> --
> 2.51.1
>
--
Steve Wahl, Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies
2025-10-29 22:07 [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Zide Chen
2025-10-29 22:07 ` [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs Zide Chen
@ 2025-10-30 1:37 ` Mi, Dapeng
2025-10-30 4:38 ` Chen, Zide
2025-11-13 22:32 ` Steve Wahl
2 siblings, 1 reply; 9+ messages in thread
From: Mi, Dapeng @ 2025-10-30 1:37 UTC (permalink / raw)
To: Zide Chen, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Xudong Hao, Falcon Thomas,
Steve Wahl
On 10/30/2025 6:07 AM, Zide Chen wrote:
> This warning can be triggered if NUMA is disabled and the system
> boots with fewer CPUs than the number of CPUs in die 0.
>
> WARNING: CPU: 9 PID: 7257 at uncore.c:1157 uncore_pci_pmu_register+0x136/0x160 [intel_uncore]
>
> Currently, the discovery table continues to be parsed even if all CPUs
> in the associated die are offline. This can lead to an array overflow
> at "pmu->boxes[die] = box" in uncore_pci_pmu_register(), which may
> trigger the warning above or cause other issues.
>
> Reported-by: Steve Wahl <steve.wahl@hpe.com>
> Fixes: edae1f06c2cd ("perf/x86/intel/uncore: Parse uncore discovery tables")
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> arch/x86/events/intel/uncore.c | 4 ++++
> arch/x86/events/intel/uncore_discovery.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index ee586eb714ec..5c3aeea5c78d 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1380,6 +1380,10 @@ static void uncore_pci_pmus_register(void)
>
> for (node = rb_first(type->boxes); node; node = rb_next(node)) {
> unit = rb_entry(node, struct intel_uncore_discovery_unit, node);
> +
> + if (WARN_ON(unit->die >= uncore_max_dies()))
Base on my understanding, it seems an valid situation which could happen.
If so, we'd better remove the WARN_on to avoid it mislead users. Thanks.
> + continue;
> +
> pdev = pci_get_domain_bus_and_slot(UNCORE_DISCOVERY_PCI_DOMAIN(unit->addr),
> UNCORE_DISCOVERY_PCI_BUS(unit->addr),
> UNCORE_DISCOVERY_PCI_DEVFN(unit->addr));
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 1bf6e4288577..d6aee12139f1 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
> @@ -388,7 +388,7 @@ static bool intel_uncore_has_discovery_tables_pci(int *ignore)
> (val & UNCORE_DISCOVERY_DVSEC2_BIR_MASK) * UNCORE_DISCOVERY_BIR_STEP;
>
> die = get_device_die_id(dev);
> - if (die < 0)
> + if ((die < 0) || (die >= uncore_max_dies()))
> continue;
>
> parse_discovery_table(dev, die, bar_offset, &parsed, ignore);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies
2025-10-30 1:37 ` [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Mi, Dapeng
@ 2025-10-30 4:38 ` Chen, Zide
2025-10-30 6:23 ` Mi, Dapeng
0 siblings, 1 reply; 9+ messages in thread
From: Chen, Zide @ 2025-10-30 4:38 UTC (permalink / raw)
To: Mi, Dapeng, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Xudong Hao, Falcon Thomas,
Steve Wahl
On 10/29/2025 6:37 PM, Mi, Dapeng wrote:
>
> On 10/30/2025 6:07 AM, Zide Chen wrote:
>> This warning can be triggered if NUMA is disabled and the system
>> boots with fewer CPUs than the number of CPUs in die 0.
>>
>> WARNING: CPU: 9 PID: 7257 at uncore.c:1157 uncore_pci_pmu_register+0x136/0x160 [intel_uncore]
>>
>> Currently, the discovery table continues to be parsed even if all CPUs
>> in the associated die are offline. This can lead to an array overflow
>> at "pmu->boxes[die] = box" in uncore_pci_pmu_register(), which may
>> trigger the warning above or cause other issues.
>>
>> Reported-by: Steve Wahl <steve.wahl@hpe.com>
>> Fixes: edae1f06c2cd ("perf/x86/intel/uncore: Parse uncore discovery tables")
>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>> ---
>> arch/x86/events/intel/uncore.c | 4 ++++
>> arch/x86/events/intel/uncore_discovery.c | 2 +-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index ee586eb714ec..5c3aeea5c78d 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
>> @@ -1380,6 +1380,10 @@ static void uncore_pci_pmus_register(void)
>>
>> for (node = rb_first(type->boxes); node; node = rb_next(node)) {
>> unit = rb_entry(node, struct intel_uncore_discovery_unit, node);
>> +
>> + if (WARN_ON(unit->die >= uncore_max_dies()))
>
> Base on my understanding, it seems an valid situation which could happen.
> If so, we'd better remove the WARN_on to avoid it mislead users. Thanks.
Now, for invalid or offline die IDs, we skip parsing the discovery
table, and no PMON units are expected to be inserted into the RB tree.
Therefore, using WARN_ON() here seems appropriate.
I put a WARN_ON() here because invalid die ID could cause array overflow.
>> + continue;
>> +
>> pdev = pci_get_domain_bus_and_slot(UNCORE_DISCOVERY_PCI_DOMAIN(unit->addr),
>> UNCORE_DISCOVERY_PCI_BUS(unit->addr),
>> UNCORE_DISCOVERY_PCI_DEVFN(unit->addr));
>> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
>> index 1bf6e4288577..d6aee12139f1 100644
>> --- a/arch/x86/events/intel/uncore_discovery.c
>> +++ b/arch/x86/events/intel/uncore_discovery.c
>> @@ -388,7 +388,7 @@ static bool intel_uncore_has_discovery_tables_pci(int *ignore)
>> (val & UNCORE_DISCOVERY_DVSEC2_BIR_MASK) * UNCORE_DISCOVERY_BIR_STEP;
>>
>> die = get_device_die_id(dev);
>> - if (die < 0)
>> + if ((die < 0) || (die >= uncore_max_dies()))
>> continue;
>>
>> parse_discovery_table(dev, die, bar_offset, &parsed, ignore);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies
2025-10-30 4:38 ` Chen, Zide
@ 2025-10-30 6:23 ` Mi, Dapeng
0 siblings, 0 replies; 9+ messages in thread
From: Mi, Dapeng @ 2025-10-30 6:23 UTC (permalink / raw)
To: Chen, Zide, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane
Cc: linux-kernel, linux-perf-users, Xudong Hao, Falcon Thomas,
Steve Wahl
On 10/30/2025 12:38 PM, Chen, Zide wrote:
> On 10/29/2025 6:37 PM, Mi, Dapeng wrote:
>> On 10/30/2025 6:07 AM, Zide Chen wrote:
>>> This warning can be triggered if NUMA is disabled and the system
>>> boots with fewer CPUs than the number of CPUs in die 0.
>>>
>>> WARNING: CPU: 9 PID: 7257 at uncore.c:1157 uncore_pci_pmu_register+0x136/0x160 [intel_uncore]
>>>
>>> Currently, the discovery table continues to be parsed even if all CPUs
>>> in the associated die are offline. This can lead to an array overflow
>>> at "pmu->boxes[die] = box" in uncore_pci_pmu_register(), which may
>>> trigger the warning above or cause other issues.
>>>
>>> Reported-by: Steve Wahl <steve.wahl@hpe.com>
>>> Fixes: edae1f06c2cd ("perf/x86/intel/uncore: Parse uncore discovery tables")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>> arch/x86/events/intel/uncore.c | 4 ++++
>>> arch/x86/events/intel/uncore_discovery.c | 2 +-
>>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>> index ee586eb714ec..5c3aeea5c78d 100644
>>> --- a/arch/x86/events/intel/uncore.c
>>> +++ b/arch/x86/events/intel/uncore.c
>>> @@ -1380,6 +1380,10 @@ static void uncore_pci_pmus_register(void)
>>>
>>> for (node = rb_first(type->boxes); node; node = rb_next(node)) {
>>> unit = rb_entry(node, struct intel_uncore_discovery_unit, node);
>>> +
>>> + if (WARN_ON(unit->die >= uncore_max_dies()))
>> Base on my understanding, it seems an valid situation which could happen.
>> If so, we'd better remove the WARN_on to avoid it mislead users. Thanks.
> Now, for invalid or offline die IDs, we skip parsing the discovery
> table, and no PMON units are expected to be inserted into the RB tree.
> Therefore, using WARN_ON() here seems appropriate.
>
> I put a WARN_ON() here because invalid die ID could cause array overflow.
Ok, it's good then. Thanks.
>
> >> + continue;
>>> +
>>> pdev = pci_get_domain_bus_and_slot(UNCORE_DISCOVERY_PCI_DOMAIN(unit->addr),
>>> UNCORE_DISCOVERY_PCI_BUS(unit->addr),
>>> UNCORE_DISCOVERY_PCI_DEVFN(unit->addr));
>>> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
>>> index 1bf6e4288577..d6aee12139f1 100644
>>> --- a/arch/x86/events/intel/uncore_discovery.c
>>> +++ b/arch/x86/events/intel/uncore_discovery.c
>>> @@ -388,7 +388,7 @@ static bool intel_uncore_has_discovery_tables_pci(int *ignore)
>>> (val & UNCORE_DISCOVERY_DVSEC2_BIR_MASK) * UNCORE_DISCOVERY_BIR_STEP;
>>>
>>> die = get_device_die_id(dev);
>>> - if (die < 0)
>>> + if ((die < 0) || (die >= uncore_max_dies()))
>>> continue;
>>>
>>> parse_discovery_table(dev, die, bar_offset, &parsed, ignore);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies
2025-10-29 22:07 [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Zide Chen
2025-10-29 22:07 ` [PATCH 2/2] perf/x86/intel/uncore: Fix die ID init and look up bugs Zide Chen
2025-10-30 1:37 ` [PATCH 1/2] perf/x86/intel/uncore: Skip discovery table for offline dies Mi, Dapeng
@ 2025-11-13 22:32 ` Steve Wahl
2 siblings, 0 replies; 9+ messages in thread
From: Steve Wahl @ 2025-11-13 22:32 UTC (permalink / raw)
To: Zide Chen
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Andi Kleen, Eranian Stephane, linux-kernel, linux-perf-users,
Dapeng Mi, Xudong Hao, Falcon Thomas, Steve Wahl
Tested-by: Steve Wahl <steve.wahl@hpe.com>
On Wed, Oct 29, 2025 at 03:07:10PM -0700, Zide Chen wrote:
> This warning can be triggered if NUMA is disabled and the system
> boots with fewer CPUs than the number of CPUs in die 0.
>
> WARNING: CPU: 9 PID: 7257 at uncore.c:1157 uncore_pci_pmu_register+0x136/0x160 [intel_uncore]
>
> Currently, the discovery table continues to be parsed even if all CPUs
> in the associated die are offline. This can lead to an array overflow
> at "pmu->boxes[die] = box" in uncore_pci_pmu_register(), which may
> trigger the warning above or cause other issues.
>
> Reported-by: Steve Wahl <steve.wahl@hpe.com>
> Fixes: edae1f06c2cd ("perf/x86/intel/uncore: Parse uncore discovery tables")
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> arch/x86/events/intel/uncore.c | 4 ++++
> arch/x86/events/intel/uncore_discovery.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index ee586eb714ec..5c3aeea5c78d 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1380,6 +1380,10 @@ static void uncore_pci_pmus_register(void)
>
> for (node = rb_first(type->boxes); node; node = rb_next(node)) {
> unit = rb_entry(node, struct intel_uncore_discovery_unit, node);
> +
> + if (WARN_ON(unit->die >= uncore_max_dies()))
> + continue;
> +
> pdev = pci_get_domain_bus_and_slot(UNCORE_DISCOVERY_PCI_DOMAIN(unit->addr),
> UNCORE_DISCOVERY_PCI_BUS(unit->addr),
> UNCORE_DISCOVERY_PCI_DEVFN(unit->addr));
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 1bf6e4288577..d6aee12139f1 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
> @@ -388,7 +388,7 @@ static bool intel_uncore_has_discovery_tables_pci(int *ignore)
> (val & UNCORE_DISCOVERY_DVSEC2_BIR_MASK) * UNCORE_DISCOVERY_BIR_STEP;
>
> die = get_device_die_id(dev);
> - if (die < 0)
> + if ((die < 0) || (die >= uncore_max_dies()))
> continue;
>
> parse_discovery_table(dev, die, bar_offset, &parsed, ignore);
> --
> 2.51.1
>
--
Steve Wahl, Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 9+ messages in thread