Re: [PATCH v7 02/12] x86/amd_nb: Add support for northbridges on Aldebaran

[Yazen Ghannam]

On Thu, Feb 03, 2022 at 11:49:32AM -0600, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@xxxxxxx>
> 
> On newer systems the CPUs manage MCA errors reported from the GPUs.
> Enumerate the GPU nodes with the AMD NB framework to support EDAC.
> 
> GPU nodes are enumerated in sequential order based on the PCI hierarchy,
> and the first GPU node is assumed to have an "AMD Node ID" value after
> CPU Nodes are fully populated.
> 
> Aldebaran is an AMD GPU, GPU drivers are part of the DRM framework
> https://lists.freedesktop.org/archives/amd-gfx/2021-February/059694.html
> 
> Each Aldebaran GPU has 2 Data Fabrics, which are enumerated as 2 nodes.
> With this implementation detail, the Data Fabric on the GPU nodes can be
> accessed the same way as the Data Fabric on CPU nodes.
> 
> Handled new device support and enumeration by calling separate function

s/Handled/handle/

> in init_amd_nbs with completely separate data structures.

s/init_amd_nbs/init_amd_nbs()/

Function names should have "()" to clearly show they are functions.

> 
> Signed-off-by: Muralidhara M K <muralimk@xxxxxxx>
> Co-developed-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> ---
> Link:
> https://lkml.kernel.org/r/20211028130106.15701-2-nchatrad@xxxxxxx
> 
> v6->v7:
> * API amd_gpu_node_start_id() introduced to reuse in future patches.
> 
> v5->v6:
> * Defined seperate structure for GPU NBs and handled GPu enumearation
>   seperately by defining new function amd_cache_gpu_devices
> * Defined amd_get_gpu_node_id which will be used by mce module
> 
> v4->v5:
> * Modified amd_get_node_map() and checking return value
> 
> v3->v4:
> * renamed struct name from nmap to nodemap
> * split amd_get_node_map and addressed minor comments
> 
> v2->v3:
> * Use word "gpu" instead of "noncpu" in the patch
> * Do not create pci_dev_ids arrays for gpu nodes
> * Identify the gpu node start index from DF18F1 registers on the GPU nodes.
>   Export cpu node count and gpu start node id
> 
> v1->v2:
> * Added Reviewed-by Yazen Ghannam
> 
> v0->v1
> * Modified the commit message and comments in the code
> * Squashed patch 1/7: "x86/amd_nb: Add Aldebaran device to PCI IDs"
> 
> 
>  arch/x86/include/asm/amd_nb.h |   9 ++
>  arch/x86/kernel/amd_nb.c      | 149 +++++++++++++++++++++++++++++++++-
>  include/linux/pci_ids.h       |   1 +
>  3 files changed, 155 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index 00d1a400b7a1..cb8bc59d17a0 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -73,6 +73,12 @@ struct amd_northbridge_info {
>  	struct amd_northbridge *nb;
>  };
>  
> +struct amd_gpu_nb_info {
> +	u16 gpu_num;
> +	struct amd_northbridge *gpu_nb;
> +	u16 gpu_node_start_id;

The members should be aligned with tabs.

Is the "gpu" prefix needed for the members since the struct type indicates
this?

Also, I think grouping members with similar types and leaving pointers to the
end may help avoid padding, etc.

> +};
> +
>  #define AMD_NB_GART			BIT(0)
>  #define AMD_NB_L3_INDEX_DISABLE		BIT(1)
>  #define AMD_NB_L3_PARTITIONING		BIT(2)
> @@ -80,8 +86,11 @@ struct amd_northbridge_info {
>  #ifdef CONFIG_AMD_NB
>  
>  u16 amd_nb_num(void);
> +u16 amd_gpu_nb_num(void);
> +u16 amd_gpu_node_start_id(void);
>  bool amd_nb_has_feature(unsigned int feature);
>  struct amd_northbridge *node_to_amd_nb(int node);
> +int amd_get_gpu_node_system_id(u64 ipid);
>  
>  static inline u16 amd_pci_dev_to_node_id(struct pci_dev *pdev)
>  {
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 020c906f7934..dfa7c7516321 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -20,6 +20,7 @@
>  #define PCI_DEVICE_ID_AMD_17H_M30H_ROOT	0x1480
>  #define PCI_DEVICE_ID_AMD_17H_M60H_ROOT	0x1630
>  #define PCI_DEVICE_ID_AMD_19H_M10H_ROOT	0x14a4
> +#define PCI_DEVICE_ID_AMD_ALDEBARAN_ROOT 0x14bb
>  #define PCI_DEVICE_ID_AMD_17H_DF_F4	0x1464
>  #define PCI_DEVICE_ID_AMD_17H_M10H_DF_F4 0x15ec
>  #define PCI_DEVICE_ID_AMD_17H_M30H_DF_F4 0x1494
> @@ -30,6 +31,14 @@
>  #define PCI_DEVICE_ID_AMD_19H_M40H_ROOT	0x14b5
>  #define PCI_DEVICE_ID_AMD_19H_M40H_DF_F4 0x167d
>  #define PCI_DEVICE_ID_AMD_19H_M50H_DF_F4 0x166e
> +#define PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F4 0x14d4
> +
> +/* GPU Data Fabric ID Device 24 Function 1 */
> +#define PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1 0x14d1
> +
> +/* DF18xF1 register on Aldebaran GPU */
> +#define REG_LOCAL_NODE_TYPE_MAP		0x144
> +
>  
>  /* Protect the PCI config register pairs used for SMN. */
>  static DEFINE_MUTEX(smn_mutex);
> @@ -104,6 +113,21 @@ static const struct pci_device_id hygon_nb_link_ids[] = {
>  	{}
>  };
>  
> +static const struct pci_device_id amd_gpu_root_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_ALDEBARAN_ROOT) },
> +	{}
> +};
> +
> +static const struct pci_device_id amd_gpu_misc_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F3) },
> +	{}
> +};
> +
> +static const struct pci_device_id amd_gpu_link_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F4) },
> +	{}
> +};
> +
>  const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[] __initconst = {
>  	{ 0x00, 0x18, 0x20 },
>  	{ 0xff, 0x00, 0x20 },
> @@ -112,6 +136,8 @@ const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[] __initconst = {
>  };
>  
>  static struct amd_northbridge_info amd_northbridges;
> +/* GPU specific structure declaration */
> +static struct amd_gpu_nb_info amd_gpu_nbs;
>  
>  u16 amd_nb_num(void)
>  {
> @@ -119,6 +145,20 @@ u16 amd_nb_num(void)
>  }
>  EXPORT_SYMBOL_GPL(amd_nb_num);
>  
> +/* Total number of GPU nbs in a system */
> +u16 amd_gpu_nb_num(void)
> +{
> +	return amd_gpu_nbs.gpu_num;
> +}
> +EXPORT_SYMBOL_GPL(amd_gpu_nb_num);
> +
> +/* Start index of hardware provided GPU node ID */
> +u16 amd_gpu_node_start_id(void)
> +{
> +	return amd_gpu_nbs.gpu_node_start_id;
> +}
> +EXPORT_SYMBOL_GPL(amd_gpu_node_start_id);
> +
>  bool amd_nb_has_feature(unsigned int feature)
>  {
>  	return ((amd_northbridges.flags & feature) == feature);
> @@ -127,10 +167,60 @@ EXPORT_SYMBOL_GPL(amd_nb_has_feature);
>  
>  struct amd_northbridge *node_to_amd_nb(int node)
>  {
> -	return (node < amd_northbridges.num) ? &amd_northbridges.nb[node] : NULL;
> +	if (node < amd_northbridges.num)
> +		return &amd_northbridges.nb[node];
> +	else if (node >= amd_northbridges.num &&
> +		 node < amd_gpu_nbs.gpu_num + amd_northbridges.num)
> +		return &amd_gpu_nbs.gpu_nb[node - amd_northbridges.num];
> +	else
> +		return NULL;
>  }
>  EXPORT_SYMBOL_GPL(node_to_amd_nb);
>  
> +/*
> + * On SMCA banks of the GPU nodes, the hardware node id information is
> + * available in register MCA_IPID[47:44](InstanceIdHi).
> + *
> + * Convert the hardware node ID to a value used by linux where
> + * GPU nodes are sequentially after the CPU nodes.
> + */
> +int amd_get_gpu_node_system_id(u64 ipid)
> +{
> +	return ((ipid >> 44 & 0xF) - amd_gpu_node_start_id()
> +				   + amd_northbridges.num);
> +}
> +EXPORT_SYMBOL_GPL(amd_get_gpu_node_system_id);

Why does this function need to be in amd_nb.c?

It seems like it's only needed in EDAC. And the necessary info is already
being exported.

> +
> +/*
> + * AMD CPUs and GPUs whose data fabrics can be connected via custom xGMI
> + * links, come with registers to gather local and remote node type map info.
> + *
> + * This function, reads the register in DF function 1 from "Local Node Type"
> + * which refers to GPU node.
> + */
> +static int amd_gpu_df_nodemap(void)

The purpose of this function is to find the "gpu_node_start_id", so I think
the function name should reflect that.

> +{
> +	struct pci_dev *pdev;
> +	u32 tmp;
> +
> +	pdev = pci_get_device(PCI_VENDOR_ID_AMD,
> +			      PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1, NULL);
> +	if (!pdev) {
> +		pr_debug("DF Func1 PCI device not found on this node.\n");
> +		return -ENODEV;
> +	}
> +
> +	if (pci_read_config_dword(pdev, REG_LOCAL_NODE_TYPE_MAP, &tmp))
> +		goto out;
> +	amd_gpu_nbs.gpu_node_start_id = tmp & 0xFFF;
> +
> +	return 0;
> +out:
> +	pr_warn("PCI config access failed\n");
> +	pci_dev_put(pdev);

Shouldn't this "put" also happen on the success path?

> +	return -ENODEV;
> +}
> +
>  static struct pci_dev *next_northbridge(struct pci_dev *dev,
>  					const struct pci_device_id *ids)
>  {
> @@ -147,7 +237,7 @@ static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)
>  	struct pci_dev *root;
>  	int err = -ENODEV;
>  
> -	if (node >= amd_northbridges.num)
> +	if (node >= amd_northbridges.num + amd_gpu_nbs.gpu_num)
>  		goto out;
>  
>  	root = node_to_amd_nb(node)->root;
> @@ -210,14 +300,14 @@ int amd_cache_northbridges(void)
>  	}
>  
>  	misc = NULL;
> -	while ((misc = next_northbridge(misc, misc_ids)) != NULL)
> +	while ((misc = next_northbridge(misc, misc_ids)))
>  		misc_count++;
>  
>  	if (!misc_count)
>  		return -ENODEV;
>  
>  	root = NULL;
> -	while ((root = next_northbridge(root, root_ids)) != NULL)
> +	while ((root = next_northbridge(root, root_ids)))

I think these last two changes aren't necessary for this feature.

>  		root_count++;
>  
>  	if (root_count) {
> @@ -292,6 +382,56 @@ int amd_cache_northbridges(void)
>  }
>  EXPORT_SYMBOL_GPL(amd_cache_northbridges);
>  
> +int amd_cache_gpu_devices(void)
> +{
> +	const struct pci_device_id *misc_ids = amd_gpu_misc_ids;
> +	const struct pci_device_id *link_ids = amd_gpu_link_ids;
> +	const struct pci_device_id *root_ids = amd_gpu_root_ids;
> +	struct pci_dev *root, *misc, *link;
> +	struct amd_northbridge *gpu_nb;
> +	u16 misc_count = 0;
> +	u16 root_count = 0;
> +	int ret;
> +	u16 i;
> +
> +	if (amd_gpu_nbs.gpu_num)
> +		return 0;
> +
> +	ret = amd_gpu_df_nodemap();
> +	if (ret)
> +		return ret;
> +
> +	misc = NULL;
> +	while ((misc = next_northbridge(misc, misc_ids)))
> +		misc_count++;
> +
> +	if (!misc_count)
> +		return -ENODEV;
> +
> +	root = NULL;
> +	while ((root = next_northbridge(root, root_ids)))
> +		root_count++;
> +
> +	gpu_nb = kcalloc(misc_count, sizeof(struct amd_northbridge), GFP_KERNEL);
> +	if (!gpu_nb)
> +		return -ENOMEM;
> +
> +	amd_gpu_nbs.gpu_nb = gpu_nb;
> +	amd_gpu_nbs.gpu_num = misc_count;
> +
> +	link = NULL, misc = NULL, root = NULL;
> +	for (i = amd_northbridges.num; i < (amd_northbridges.num + amd_gpu_nbs.gpu_num); i++) {
> +		node_to_amd_nb(i)->root = root =
> +			next_northbridge(root, root_ids);
> +		node_to_amd_nb(i)->misc = misc =
> +			next_northbridge(misc, misc_ids);
> +		node_to_amd_nb(i)->link = link =
> +			next_northbridge(link, link_ids);
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_cache_gpu_devices);

I don't think this needs to be exported since it's called in init_amd_nbs()
below.

A module can use amd_nb_num() to see if anything was initialized rather than
calling amd_cache_*() again.

The existing callers of amd_cache_*() functions need to be cleaned up.

> +
>  /*
>   * Ignores subdevice/subvendor but as far as I can figure out
>   * they're useless anyways
> @@ -497,6 +637,7 @@ static __init int init_amd_nbs(void)
>  {
>  	amd_cache_northbridges();
>  	amd_cache_gart();
> +	amd_cache_gpu_devices();
>  
>  	fix_erratum_688();
>

...

Thanks,
Yazen