Message-ID: <94cd8a1f-1f2a-4cd6-8ab8-49b9d1e9fa2d@kernel.org>
Date: Tue, 30 Sep 2025 13:07:47 -0500
From: "Mario Limonciello (AMD) (kernel.org)" <superm1@...nel.org>
To: Yazen Ghannam <yazen.ghannam@....com>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, Filip Barczyk <filip.barczyk@...o.net>
Subject: Re: [PATCH 1/2] x86/amd_node: Fix AMD root device caching



On 9/30/2025 11:45 AM, Yazen Ghannam wrote:
> Recent AMD node rework removed the "search and count" method of caching
> AMD root devices. This depended on the value from a Data Fabric register
> that was expected to hold the PCI bus of one of the root devices
> attached to that fabric.
> 
> However, this expectation is incorrect. The register, when read from PCI
> config space, returns the bitwise-OR of the buses of all attached root
> devices.
> 
> This behavior is benign on AMD reference design boards, since the bus
> numbers are aligned. This results in a bitwise-OR value matching one of
> the buses. For example, 0x00 | 0x40 | 0xA0 | 0xE0 = 0xE0.
> 
> This behavior breaks on boards where the bus numbers are not exactly
> aligned. For example, 0x00 | 0x07 | 0xE0 | 0x15 = 0x1F.
> 
> The bus numbering style in the reference boards is not a requirement.
> The numbering found in other boards is not incorrect. Therefore, the
> root device caching method needs to be adjusted.
> 
> Go back to the "search and count" method used before the recent rework.
> Search for root devices using PCI class code rather than fixed PCI IDs.
> 
> This keeps the goal of the rework (remove dependency on PCI IDs) while
> being able to support various board designs.
> 
> Fixes: 40a5f6ffdfc8 ("x86/amd_nb: Simplify root device search")

Was this a publicly reported failure?

If so is there a link to LKML or a Bugzilla with the details of the 
failure you can include here?

> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> Cc: stable@...r.kernel.org
> ---
>   arch/x86/include/asm/amd/node.h |  1 -
>   arch/x86/kernel/amd_node.c      | 95 ++++++++++++++++-------------------------
>   2 files changed, 36 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/x86/include/asm/amd/node.h b/arch/x86/include/asm/amd/node.h
> index 23fe617898a8..a672b8765fa8 100644
> --- a/arch/x86/include/asm/amd/node.h
> +++ b/arch/x86/include/asm/amd/node.h
> @@ -23,7 +23,6 @@
>   #define AMD_NODE0_PCI_SLOT	0x18
>   
>   struct pci_dev *amd_node_get_func(u16 node, u8 func);
> -struct pci_dev *amd_node_get_root(u16 node);
>   
>   static inline u16 amd_num_nodes(void)
>   {
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index a40176b62eb5..4a3d9ca6e225 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -34,62 +34,6 @@ struct pci_dev *amd_node_get_func(u16 node, u8 func)
>   	return pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(AMD_NODE0_PCI_SLOT + node, func));
>   }
>   
> -#define DF_BLK_INST_CNT		0x040
> -#define	DF_CFG_ADDR_CNTL_LEGACY	0x084
> -#define	DF_CFG_ADDR_CNTL_DF4	0xC04
> -
> -#define DF_MAJOR_REVISION	GENMASK(27, 24)
> -
> -static u16 get_cfg_addr_cntl_offset(struct pci_dev *df_f0)
> -{
> -	u32 reg;
> -
> -	/*
> -	 * Revision fields added for DF4 and later.
> -	 *
> -	 * Major revision of '0' is found pre-DF4. Field is Read-as-Zero.
> -	 */
> -	if (pci_read_config_dword(df_f0, DF_BLK_INST_CNT, &reg))
> -		return 0;
> -
> -	if (reg & DF_MAJOR_REVISION)
> -		return DF_CFG_ADDR_CNTL_DF4;
> -
> -	return DF_CFG_ADDR_CNTL_LEGACY;
> -}
> -
> -struct pci_dev *amd_node_get_root(u16 node)
> -{
> -	struct pci_dev *root;
> -	u16 cntl_off;
> -	u8 bus;
> -
> -	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
> -		return NULL;
> -
> -	/*
> -	 * D18F0xXXX [Config Address Control] (DF::CfgAddressCntl)
> -	 * Bits [7:0] (SecBusNum) holds the bus number of the root device for
> -	 * this Data Fabric instance. The segment, device, and function will be 0.
> -	 */
> -	struct pci_dev *df_f0 __free(pci_dev_put) = amd_node_get_func(node, 0);
> -	if (!df_f0)
> -		return NULL;
> -
> -	cntl_off = get_cfg_addr_cntl_offset(df_f0);
> -	if (!cntl_off)
> -		return NULL;
> -
> -	if (pci_read_config_byte(df_f0, cntl_off, &bus))
> -		return NULL;
> -
> -	/* Grab the pointer for the actual root device instance. */
> -	root = pci_get_domain_bus_and_slot(0, bus, 0);
> -
> -	pci_dbg(root, "is root for AMD node %u\n", node);
> -	return root;
> -}
> -
>   static struct pci_dev **amd_roots;
>   
>   /* Protect the PCI config register pairs used for SMN. */
> @@ -274,16 +218,49 @@ DEFINE_SHOW_STORE_ATTRIBUTE(smn_node);
>   DEFINE_SHOW_STORE_ATTRIBUTE(smn_address);
>   DEFINE_SHOW_STORE_ATTRIBUTE(smn_value);
>   
> +static struct pci_dev *get_next_root(struct pci_dev *root)
> +{
> +	while ((root = pci_get_class(PCI_CLASS_BRIDGE_HOST << 8, root))) {
> +		/* Root device is Device 0 Function 0. */
> +		if (root->devfn)
> +			continue;
> +
> +		if (root->vendor != PCI_VENDOR_ID_AMD &&
> +		    root->vendor != PCI_VENDOR_ID_HYGON)
> +			continue;
> +
> +		break;
> +	}
> +
> +	return root;
> +}
> +
>   static int amd_cache_roots(void)
>   {
> -	u16 node, num_nodes = amd_num_nodes();
> +	u16 count = 0, num_roots = 0, roots_per_node, node = 0, num_nodes = amd_num_nodes();
> +	struct pci_dev *root = NULL;
>   
>   	amd_roots = kcalloc(num_nodes, sizeof(*amd_roots), GFP_KERNEL);
>   	if (!amd_roots)
>   		return -ENOMEM;
>   
> -	for (node = 0; node < num_nodes; node++)
> -		amd_roots[node] = amd_node_get_root(node);
> +	while ((root = get_next_root(root))) {
> +		pci_dbg(root, "is an AMD root device\n");
> +		num_roots++;
> +	}
> +
> +	pr_debug("Found %d AMD root devices\n", num_roots);
> +
> +	roots_per_node = num_roots / num_nodes;
> +
> +	while ((root = get_next_root(root)) && node < num_nodes) {
> +		/* Use one root for each node and skip the rest. */
> +		if (count++ % roots_per_node)
> +			continue;
> +
> +		pci_dbg(root, "is root for AMD node %u\n", node);
> +		amd_roots[node++] = root;
> +	}
>   
>   	return 0;
>   }
> 

Powered by blists - more mailing lists