Re: [PATCH V6 4/5] perf/x86/intel/uncore: Fix PMON enumeration with NUMA disabled

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 3/31/2026 5:24 AM, Zide Chen wrote:
> When NUMA is disabled on a NUMA-capable platform, UPI and M3UPI PMON
> units are not enumerated.
>
> In this case, pcibus_to_node() always returns NUMA_NO_NODE, causing
> uncore_device_to_die() to return -1 for all PCI devices. As a result,
> the corresponding PMON units are not added to the RB tree.
>
> These PMON units are per-die resources, and their utility when NUMA is
> disabled is limited.  The driver does not prohibit their use, and the
> enumeration should still work correctly.
>
> Fix this by using uncore_pcibus_to_dieid(), which works regardless of
> whether NUMA is enabled.  This requires calling
> snbep_pci2phy_map_init() in spr_uncore_pci_init().
>
> Since pci_init() is called before mmio_init(), remove the redundant
> snbep_pci2phy_map_init() call from spr_uncore_mmio_init().  If
> snbep_pci2phy_map_init() fails, uncore driver should be bailed out,
> so the fallback path in spr_uncore_mmio_init() can be removed.
>
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> V6:
> - Split from patch v5 3/4.
> - Remove the redundant call in spr_uncore_mmio_init().
> - Update commit messages.
> ---
>  arch/x86/events/intel/uncore.c       |  1 +
>  arch/x86/events/intel/uncore_snbep.c | 26 +++++++++++---------------
>  2 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 786bd51a0d89..e9cc1ba921c5 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -67,6 +67,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 8ee06d4659bb..73da1e88e286 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -6415,7 +6415,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;
>  
> @@ -6439,6 +6439,10 @@ 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.
> @@ -6460,21 +6464,13 @@ int spr_uncore_pci_init(void)
>  
>  void spr_uncore_mmio_init(void)
>  {
> -	int ret = snbep_pci2phy_map_init(0x3250, SKX_CPUNODEID, SKX_GIDNIDMAP, true);
> +	uncore_mmio_uncores = uncore_get_uncores(UNCORE_ACCESS_MMIO,
> +						 UNCORE_SPR_MMIO_EXTRA_UNCORES,
> +						 spr_mmio_uncores,
> +						 UNCORE_SPR_NUM_UNCORE_TYPES,
> +						 spr_uncores);
>  
> -	if (ret) {
> -		uncore_mmio_uncores = uncore_get_uncores(UNCORE_ACCESS_MMIO, 0, NULL,
> -							 UNCORE_SPR_NUM_UNCORE_TYPES,
> -							 spr_uncores);
> -	} else {
> -		uncore_mmio_uncores = uncore_get_uncores(UNCORE_ACCESS_MMIO,
> -							 UNCORE_SPR_MMIO_EXTRA_UNCORES,
> -							 spr_mmio_uncores,
> -							 UNCORE_SPR_NUM_UNCORE_TYPES,
> -							 spr_uncores);
> -
> -		spr_uncore_imc_free_running.num_boxes = uncore_type_max_boxes(uncore_mmio_uncores, UNCORE_SPR_IMC) / 2;
> -	}
> +	spr_uncore_imc_free_running.num_boxes = uncore_type_max_boxes(uncore_mmio_uncores, UNCORE_SPR_IMC) / 2;

I'm not sure if we can directly remove the snbep_pci2phy_map_init() call
here. In theory, the snbep_pci2phy_map_init() call in spr_uncore_pci_init()
could fail and then spr_uncore_mmio_init() doesn't know it and directly
initializes MMIO PMU, then it could lead to the MMIO initialization fails.

Currently the PCI, CPU and MMIO initialization are totally independent,
only when the 3 types initialization all fail, then uncore PMU can abort.

``` 

   if (uncore_init->pci_init) {
        pret = uncore_init->pci_init();
        if (!pret)
            pret = uncore_pci_init();
    }

    if (uncore_init->cpu_init) {
        uncore_init->cpu_init();
        cret = uncore_cpu_init();
    }

    if (uncore_init->mmio_init) {
        uncore_init->mmio_init();
        mret = uncore_mmio_init();
    }

    if (cret && pret && mret) {
        ret = -ENODEV;
        goto free_discovery;
    }
```


>  }
>  
>  /* end of SPR uncore support */