Re: [RFC PATCH v2 4/5] x86/amd_node: Add smn_set_roots() and smn_activate() for external SMN registration

[Mario Limonciello <mario.limonciello@amd.com>]

On 4/23/26 01:04, Lin Wang wrote:
> Guard the AMD SMN init (amd_smn_init) to X86_VENDOR_AMD only, removing
> the PCI_VENDOR_ID_HYGON check from get_next_root() so that Hygon host
> bridges are no longer enumerated by the AMD path.
> 
> Split the original monolithic root registration into two explicit phases:
> 
>    smn_set_roots()  -- pure data registration: saves the per-node root
>                        array and node count, with no side effects.
> 
>    smn_activate()   -- enables the SMN access layer: creates the optional
>                        debugfs interface (gated by amd_smn_debugfs_enable)
>                        and sets smn_exclusive so that amd_smn_read/write()
>                        begin accepting requests.  Takes a debugfs_name
>                        parameter so each vendor can use its own directory
>                        name (e.g. "amd_smn", "hygon_smn").
> 
> This two-phase design keeps the API names honest (set_roots is a pure
> setter, activate is an explicit state transition) and lets each vendor
> control when the shared SMN layer becomes live.
> 
> Export both functions so that any vendor-specific init module can hand a
> pre-built per-node root array to the shared SMN layer.
> 
> No functional change for AMD systems.
> 
> Signed-off-by: Lin Wang <wanglin@open-hieco.net>
> ---
>   arch/x86/include/asm/amd/node.h |   4 ++
>   arch/x86/kernel/amd_node.c      | 106 +++++++++++++++++++++++++-------
>   2 files changed, 87 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/amd/node.h b/arch/x86/include/asm/amd/node.h
> index a672b8765fa8..2705b4f59361 100644
> --- a/arch/x86/include/asm/amd/node.h
> +++ b/arch/x86/include/asm/amd/node.h
> @@ -32,12 +32,16 @@ static inline u16 amd_num_nodes(void)
>   #ifdef CONFIG_AMD_NODE
>   int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
>   int __must_check amd_smn_write(u16 node, u32 address, u32 value);
> +int smn_set_roots(struct pci_dev **roots, u16 num_nodes);
> +int smn_activate(const char *debugfs_name);
>   
>   /* Should only be used by the HSMP driver. */
>   int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write);
>   #else
>   static inline int __must_check amd_smn_read(u16 node, u32 address, u32 *value) { return -ENODEV; }
>   static inline int __must_check amd_smn_write(u16 node, u32 address, u32 value) { return -ENODEV; }
> +static inline int smn_set_roots(struct pci_dev **roots, u16 num_nodes) { return -ENODEV; }
> +static inline int smn_activate(const char *debugfs_name) { return -ENODEV; }
>   
>   static inline int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write)
>   {
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index 0be01725a2a4..76dbf847e567 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -9,6 +9,7 @@
>    */
>   
>   #include <linux/debugfs.h>
> +#include <asm/processor.h>
>   #include <asm/amd/node.h>
>   
>   /*
> @@ -34,7 +35,8 @@ 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));
>   }
>   
> -static struct pci_dev **amd_roots;
> +static struct pci_dev **smn_roots;
> +static u16 smn_num_nodes;
>   
>   /* Protect the PCI config register pairs used for SMN. */
>   static DEFINE_MUTEX(smn_mutex);
> @@ -88,10 +90,10 @@ static int __amd_smn_rw(u8 i_off, u8 d_off, u16 node, u32 address, u32 *value, b
>   	struct pci_dev *root;
>   	int err = -ENODEV;
>   
> -	if (node >= amd_num_nodes())
> +	if (node >= smn_num_nodes)
>   		return err;
>   
> -	root = amd_roots[node];
> +	root = smn_roots[node];
>   	if (!root)
>   		return err;
>   
> @@ -151,7 +153,7 @@ static ssize_t smn_node_write(struct file *file, const char __user *userbuf,
>   	if (ret)
>   		return ret;
>   
> -	if (node >= amd_num_nodes())
> +	if (node >= smn_num_nodes)
>   		return -ENODEV;
>   
>   	debug_node = node;
> @@ -225,8 +227,7 @@ static struct pci_dev *get_next_root(struct pci_dev *root)
>   		if (root->devfn)
>   			continue;
>   
> -		if (root->vendor != PCI_VENDOR_ID_AMD &&
> -		    root->vendor != PCI_VENDOR_ID_HYGON)
> +		if (root->vendor != PCI_VENDOR_ID_AMD)
>   			continue;
>   
>   		break;
> @@ -244,17 +245,78 @@ static int __init amd_smn_enable_dfs(char *str)
>   }
>   __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
>   
> +/*
> + * Register a pre-built per-node SMN root array.  The caller allocates
> + * @roots and transfers ownership to the SMN layer; the array must remain
> + * valid for the lifetime of the system.
> + *
> + * This is a pure data registration; the SMN access layer is not yet
> + * usable until smn_activate() is called.
> + *
> + * Not concurrency-safe.  Intended to be called exactly once during boot
> + * (e.g. from an fs_initcall).  The caller is responsible for any
> + * serialization required by its own init path.
> + */
> +int smn_set_roots(struct pci_dev **roots, u16 num_nodes)
> +{
> +	if (!roots || !num_nodes)
> +		return -EINVAL;
> +
> +	if (smn_roots)
> +		return -EBUSY;
> +
> +	smn_roots = roots;
> +	smn_num_nodes = num_nodes;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(smn_set_roots);
> +
> +/*
> + * Activate the SMN access layer after roots have been registered via
> + * smn_set_roots().  Creates the optional debugfs interface (gated by
> + * the amd_smn_debugfs_enable boot parameter) and sets smn_exclusive so
> + * that amd_smn_read()/amd_smn_write() begin accepting requests.
> + *
> + * @debugfs_name: directory name under arch_debugfs_dir (e.g. "amd_smn")
> + *
> + * Same serialization rules as smn_set_roots(): must be called exactly
> + * once, at boot, after smn_set_roots() succeeds.
> + */
> +int smn_activate(const char *debugfs_name)
> +{
> +	if (!smn_roots || !smn_num_nodes)
> +		return -EINVAL;

You don't need the !smn_num_nodes check here.  smn_set_roots() already 
has it.

> +
> +	if (smn_exclusive)
> +		return -EBUSY;
> +
> +	if (enable_dfs) {
> +		debugfs_dir = debugfs_create_dir(debugfs_name, arch_debugfs_dir);
> +
> +		debugfs_create_file("node",	0600, debugfs_dir, NULL, &smn_node_fops);
> +		debugfs_create_file("address",	0600, debugfs_dir, NULL, &smn_address_fops);
> +		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
> +	}
> +
> +	smn_exclusive = true;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(smn_activate);
> +
>   static int __init amd_smn_init(void)
>   {
>   	u16 count, num_roots, roots_per_node, node, num_nodes;
> -	struct pci_dev *root;
> +	struct pci_dev **roots, *root;
> +	int ret;
>   
> -	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
> +	if (!cpu_feature_enabled(X86_FEATURE_ZEN) ||
> +	    boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>   		return 0;
>   
>   	guard(mutex)(&smn_mutex);
>   
> -	if (amd_roots)
> +	if (smn_roots)
>   		return 0;
>   
>   	num_roots = 0;
> @@ -268,7 +330,9 @@ static int __init amd_smn_init(void)
>   		 * entire PCI config space for simplicity rather than covering
>   		 * specific registers piecemeal.
>   		 */
> -		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
> +		if (!pci_request_config_region_exclusive(root, 0,
> +							 PCI_CFG_SPACE_SIZE,
> +							 NULL)) {
>   			pci_err(root, "Failed to reserve config space\n");
>   			return -EEXIST;
>   		}
> @@ -276,14 +340,14 @@ static int __init amd_smn_init(void)
>   		num_roots++;
>   	}
>   
> -	pr_debug("Found %d AMD root devices\n", num_roots);
> +	pr_debug("Found %d SMN root devices\n", num_roots);
>   
>   	if (!num_roots)
>   		return -ENODEV;
>   
>   	num_nodes = amd_num_nodes();
> -	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
> -	if (!amd_roots)
> +	roots = kzalloc_objs(*roots, num_nodes);
> +	if (!roots)
>   		return -ENOMEM;
>   
>   	roots_per_node = num_roots / num_nodes;
> @@ -297,20 +361,16 @@ static int __init amd_smn_init(void)
>   			continue;
>   
>   		pci_dbg(root, "is root for AMD node %u\n", node);
> -		amd_roots[node++] = root;
> +		roots[node++] = root;
>   	}
>   
> -	if (enable_dfs) {
> -		debugfs_dir = debugfs_create_dir("amd_smn", arch_debugfs_dir);
> -
> -		debugfs_create_file("node",	0600, debugfs_dir, NULL, &smn_node_fops);
> -		debugfs_create_file("address",	0600, debugfs_dir, NULL, &smn_address_fops);
> -		debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
> +	ret = smn_set_roots(roots, num_nodes);
> +	if (ret) {
> +		kfree(roots);
> +		return ret;
>   	}
>   
> -	smn_exclusive = true;
> -
> -	return 0;
> +	return smn_activate("amd_smn");
>   }
>   
>   fs_initcall(amd_smn_init);