Re: [PATCH v7 7/7] x86/hygon: Add Family 0x18 SMN access

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Fri, Oct 09, 2026 at 05:01:11PM +0800, Lin Wang wrote:
> All DF nodes in a Hygon socket share one SMN ingress, so SMN accesses
> for a node have to go through a root of the node's socket rather than a
> root per node.
> 
> After DF enumeration, map each DF node to the first root of its socket.
> The root socket ID cannot be read back, so group the roots by socket
> in PCI enumeration order. Hold a reference to every root, reserve its
> config space, and serialize each index/data transaction. Enable SMN
> access only after setup succeeds; on failure the DF node cache remains
> usable without it.
> 
> Signed-off-by: Lin Wang <wanglin@open-hieco.net>
> ---
>  arch/x86/include/asm/hygon/node.h |  36 +++++
>  arch/x86/kernel/hygon_node.c      | 233 +++++++++++++++++++++++++++++-
>  2 files changed, 268 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/hygon/node.h b/arch/x86/include/asm/hygon/node.h
> index 5a0a86e90ea2..321a40d41ed7 100644
> --- a/arch/x86/include/asm/hygon/node.h
> +++ b/arch/x86/include/asm/hygon/node.h
> @@ -96,6 +96,30 @@ struct pci_dev *hygon_node_get_func(u16 node, u8 func);
>   */
>  int hygon_pci_dev_to_df_node(struct pci_dev *pdev);
>  
> +/**
> + * hygon_smn_read() - Read a 32-bit value from a Hygon SMN address
> + * @node: DF node index in [0, hygon_node_num())
> + * @address: SMN address
> + * @value: Output value
> + *
> + * Return: 0 on success, -EINVAL if @value is NULL, -ENODEV if Hygon
> + * SMN is not initialized or @node is out of range, or a negative
> + * errno from the underlying PCI config access.
> + */
> +int __must_check hygon_smn_read(u16 node, u32 address, u32 *value);
> +
> +/**
> + * hygon_smn_write() - Write a 32-bit value to a Hygon SMN address
> + * @node: DF node index in [0, hygon_node_num())
> + * @address: SMN address
> + * @value: Value to write
> + *
> + * Return: 0 on success, -ENODEV if Hygon SMN is not initialized or
> + * @node is out of range, or a negative errno from the underlying
> + * PCI config access.
> + */
> +int __must_check hygon_smn_write(u16 node, u32 address, u32 value);
> +
>  #else /* !CONFIG_HYGON_NODE */
>  
>  static inline u16 hygon_node_num(void)
> @@ -123,6 +147,18 @@ static inline int hygon_pci_dev_to_df_node(struct pci_dev *pdev)
>  	return -ENODEV;
>  }
>  
> +static inline int __must_check hygon_smn_read(u16 node, u32 address,
> +					      u32 *value)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int __must_check hygon_smn_write(u16 node, u32 address,
> +					       u32 value)
> +{
> +	return -ENODEV;
> +}
> +
>  #endif /* CONFIG_HYGON_NODE */
>  
>  #endif /* _ASM_X86_HYGON_NODE_H */
> diff --git a/arch/x86/kernel/hygon_node.c b/arch/x86/kernel/hygon_node.c
> index 4d492513a433..b4afa296b4ca 100644
> --- a/arch/x86/kernel/hygon_node.c
> +++ b/arch/x86/kernel/hygon_node.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Hygon Family 0x18 Data Fabric node enumeration
> + * Hygon Family 0x18 Data Fabric node enumeration and SMN access
>   *
>   * A DF instance exposes sibling PCI functions in one slot. Function 3
>   * is the misc device used for enumeration, function 4 is the link
> @@ -17,9 +17,11 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/cleanup.h>
>  #include <linux/cpufeature.h>
>  #include <linux/export.h>
>  #include <linux/init.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/pci_ids.h>
>  #include <linux/processor.h>
> @@ -633,6 +635,231 @@ int hygon_pci_dev_to_df_node(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL_GPL(hygon_pci_dev_to_df_node);
>  
> +/*
> + * SMN index/data register pair offsets in the host-bridge PCI config
> + * space. Reads and writes to a (node, address) pair are issued as a
> + * two-step transaction: write the SMN address to the index register,
> + * then read or write the value at the data register.
> + */
> +#define SMN_INDEX_OFFSET	0x60
> +#define SMN_DATA_OFFSET		0x64
> +
> +/*
> + * Runtime SMN state.
> + *
> + * The smn_reserved_roots array owns the independent PCI references and
> + * config-space reservations for the lifetime of the built-in node layer. The
> + * smn_roots array contains per-node aliases and is published only after
> + * setup succeeds.
> + */
> +static struct pci_dev	**smn_roots;
> +static struct pci_dev	**smn_reserved_roots;
> +static u16		smn_num_nodes;
> +static DEFINE_MUTEX(smn_mutex);
> +
> +/*
> + * Walk PCI host-bridge devices matching the Hygon vendor. The SMN
> + * index/data registers live in function 0 of each root complex. This
> + * follows pci_get_class() iterator semantics: @root is consumed and the
> + * returned device has an iterator reference. A retained device needs an
> + * independent reference before the iterator advances.
> + */
> +static struct pci_dev * __init get_next_root(struct pci_dev *root)
> +{
> +	while ((root = pci_get_class(PCI_CLASS_BRIDGE_HOST << 8, root))) {
> +		if (root->devfn)
> +			continue;
> +		if (root->vendor != PCI_VENDOR_ID_HYGON)
> +			continue;
> +		break;
> +	}
> +	return root;
> +}
> +
> +/* Release each config region before dropping its owning device reference. */
> +static void __init release_reserved_roots(struct pci_dev **roots, u16 count)
> +{
> +	u16 i;
> +
> +	for (i = 0; i < count; i++) {
> +		pci_release_config_region(roots[i], 0, PCI_CFG_SPACE_SIZE);
> +		pci_dev_put(roots[i]);
> +	}
> +}
> +
> +/*
> + * Select one root from each contiguous per-socket enumeration group,
> + * then map every DF node to the root for its socket. The root socket ID
> + * cannot be read back, so the grouping follows PCI enumeration order.
> + *
> + * The smn_reserved_roots array owns the PCI references and config
> + * regions. The smn_roots array contains per-node aliases. Enable SMN
> + * access only after both arrays are complete.
> + */
> +static int __init smn_setup(void)
> +{
> +	u16 num_roots, roots_per_socket, node, num_nodes, num_sockets, reserved;
> +	struct pci_dev **reserved_roots, **roots;
> +	struct pci_dev *root = NULL;
> +	u8 socket_id;
> +	int ret;
> +
> +	num_roots = 0;
> +	while ((root = get_next_root(root)))
> +		num_roots++;
> +
> +	pr_debug("Found %u Hygon SMN root devices\n", num_roots);
> +
> +	if (!num_roots)
> +		return -ENODEV;
> +
> +	num_nodes = df_cache.num_nodes;
> +	if (!num_nodes)
> +		return -ENODEV;
> +
> +	num_sockets = df_cache.num_sockets;
> +	if (!num_sockets)
> +		return -ENODEV;
> +
> +	if (num_roots % num_sockets) {
> +		pr_err("Root count %u not divisible by socket count %u\n",
> +		       num_roots, num_sockets);
> +		return -ENODEV;
> +	}
> +
> +	roots = kzalloc_objs(*roots, num_nodes);
> +	if (!roots)
> +		return -ENOMEM;
> +
> +	reserved_roots = kzalloc_objs(*reserved_roots, num_roots);
> +	if (!reserved_roots) {
> +		kfree(roots);
> +		return -ENOMEM;
> +	}
> +
> +	/* Reserve every root before publishing the node-to-root mapping. */
> +	roots_per_socket = num_roots / num_sockets;
> +	reserved = 0;
> +	while ((root = get_next_root(root))) {
> +		if (reserved >= num_roots) {
> +			ret = -ENODEV;
> +			goto err_release;
> +		}
> +
> +		pci_dbg(root, "Reserving PCI config space\n");
> +
> +		/*
> +		 * Mark the entire PCI config space kernel-exclusive because it
> +		 * contains the SMN index/data registers.
> +		 */
> +		if (!pci_request_config_region_exclusive(root, 0,
> +							 PCI_CFG_SPACE_SIZE,
> +							 NULL)) {
> +			pci_err(root, "Failed to reserve config space\n");
> +			ret = -EEXIST;
> +			goto err_release;
> +		}
> +
> +		reserved_roots[reserved++] = pci_dev_get(root);
> +	}
> +
> +	if (reserved != num_roots) {
> +		pr_err("Root enumeration changed: expected %u roots, got %u\n",
> +		       num_roots, reserved);
> +		ret = -ENODEV;
> +		goto err_release;
> +	}
> +
> +	for (node = 0; node < num_nodes; node++) {
> +		socket_id = df_cache.nodes[node].socket_id;
> +
> +		if (socket_id >= num_sockets) {
> +			ret = -ENODEV;
> +			goto err_release;
> +		}
> +
> +		roots[node] = reserved_roots[socket_id * roots_per_socket];
> +
> +		pci_dbg(roots[node],
> +			"is root for Hygon node %u (socket %u)\n",
> +			node, socket_id);
> +	}
> +
> +	smn_reserved_roots = reserved_roots;
> +	smn_num_nodes = num_nodes;
> +	smn_roots = roots;
> +	return 0;
> +
> +err_release:
> +	pci_dev_put(root);
> +	release_reserved_roots(reserved_roots, reserved);
> +	kfree(reserved_roots);
> +	kfree(roots);
> +	return ret;
> +}
> +
> +/*
> + * Serialize the PCI index/data pair between in-kernel SMN users.
> + * The access sequence follows amd_smn_read() and amd_smn_write().
> + * The Hygon node-to-root mapping comes from smn_setup().
> + */
> +static int smn_rw(u16 node, u32 address, u32 *value, bool write)
> +{
> +	struct pci_dev *root;
> +	int err;
> +
> +	if (!smn_roots || node >= smn_num_nodes)
> +		return -ENODEV;
> +
> +	root = smn_roots[node];
> +	if (!root)
> +		return -ENODEV;
> +
> +	guard(mutex)(&smn_mutex);
> +
> +	err = pci_write_config_dword(root, SMN_INDEX_OFFSET, address);
> +	if (err) {
> +		pr_warn("SMN index write failed (addr 0x%x)\n", address);
> +		return pcibios_err_to_errno(err);
> +	}
> +
> +	err = write ? pci_write_config_dword(root, SMN_DATA_OFFSET, *value)
> +		    : pci_read_config_dword(root, SMN_DATA_OFFSET, value);
> +
> +	return pcibios_err_to_errno(err);
> +}
> +
> +int hygon_smn_read(u16 node, u32 address, u32 *value)
> +{
> +	int err;
> +
> +	if (!value)
> +		return -EINVAL;
> +
> +	err = smn_rw(node, address, value, false);
> +
> +	/* Clear the output so callers do not consume a stale value. */
> +	if (err) {
> +		*value = 0;
> +		return err;
> +	}
> +
> +	/* Treat the PCI all-ones value as a missing device. */
> +	if (PCI_POSSIBLE_ERROR(*value)) {
> +		*value = 0;
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(hygon_smn_read);
> +
> +int hygon_smn_write(u16 node, u32 address, u32 value)
> +{
> +	return smn_rw(node, address, &value, true);
> +}
> +EXPORT_SYMBOL_GPL(hygon_smn_write);
> +

Same point as before for exported functions.

Overall, looks good to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen