Re: [PATCH 3/7] platform/x86/amd/hsmp: Add ACPI client support via the SMNR method

On 7/29/26 11:40, Muralidhara M K wrote:
> On the Family 1Ah client platforms (Models 80h-8Fh and E0h-E3h) the ACPI
> HSMP device (HID AMDI0097, ACPI path \_SB_.TELD) is present but, unlike
> server platforms, exposes neither a _CRS memory window nor a _DSD
> mailbox-offset package, and names its socket differently. The probe
> therefore failed in three places:
> 
>    - hsmp_get_uid() requires the server "IDXX" _UID form and rejects
>      anything shorter than three characters. The client device carries a
>      bare socket number ("0"), so the probe aborted with -EINVAL before
>      it reached any of the code below, and without logging a reason.
>    - hsmp_read_acpi_crs()/_dsd() found no _CRS/_DSD and bailed out.
>    - hsmp_cache_proto_ver() issued HSMP_CLIENT_GET_INTERFACE_VER
>      (msg 0x3), which this client SMU rejects with "invalid message"
>      (0xFE) even though the mailbox is otherwise functional.
> 
> Handle the client explicitly:
> 
>    - hsmp_get_uid(): strip the "ID" prefix only when it is present, so
>      both the server "IDXX" form and a bare socket number are accepted.
>      A _UID that is neither still fails, as kstrtou16() rejects it.
>    - hsmp_parse_acpi_table(): for is_client_platform(), skip _CRS/_DSD
>      and take the fixed client SMN mailbox addresses from the platform
>      descriptor, accessed through firmware. Reads go via the read-only
>      SMNR ACPI method
>      (TELD.SMNR -> CpmReadSmnRegister, the same accessor used by the
>      AMDHSMP Windows driver's amdtelemetry_Acpi_SMNR / 'RNMS'); writes
>      go via the kernel SMN helper (amd_smn_hsmp_rdwr), as this device
>      has no ACPI SMN write method.
>    - init_acpi(): treat the interface-version query as non-fatal on the
>      client, and call hsmp_get_tbl_dram_base() whatever version was
>      reported so the metric table is fetched with
>      HSMP_CLIENT_GET_METRICS_TABLE_DRAM_ADDR, mirroring plat.c.
> 
> As on the server path, the client branch publishes sock->dev last with
> smp_store_release(). sock->dev is the readiness gate the data plane
> tests with smp_load_acquire(), so it must not become visible before
> the accessor and mailbox offsets that this socket's mailbox access
> depends on.
> 
> is_client_platform() covers every Family 1Ah client platform (Medusa1,
> Olympic Ridge and Medusa2), which all share the same client SMN
> mailbox, so one branch handles them. Server behaviour (pure
> _DSD/_CRS/MMIO) is unchanged: the _UID handling still strips the "ID"
> prefix those platforms use.
> 

Why even bother with the ACPI one? Couldn't amd_smn_hsmp_rdwr() handle 
both read and write?

> Signed-off-by: Muralidhara M K <[email protected]>
> ---
>   drivers/platform/x86/amd/hsmp/acpi.c | 125 +++++++++++++++++++++++++--
>   1 file changed, 116 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 8257cd1da48e..bbf2b9a8a408 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -10,6 +10,7 @@
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
>   #include <asm/amd/hsmp.h>
> +#include <asm/amd/node.h>
>   
>   #include <linux/acpi.h>
>   #include <linux/array_size.h>
> @@ -71,6 +72,68 @@ static int amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
>   	return 0;
>   }
>   
> +/*
> + * Family 1Ah client platforms (Models 80h-8Fh and E0h-E3h) expose the HSMP
> + * ACPI device (HID AMDI0097, ACPI path \_SB_.TELD) but, unlike server
> + * platforms, the device provides neither a _CRS memory window nor a _DSD
> + * mailbox-offset package.  The mailbox lives in SMN register space and is
> + * reached through firmware:
> + *
> + *   - Reads  : the read-only ACPI method SMNR(addr) returns the 32-bit
> + *              value of the SMN register at absolute SMN address 'addr'
> + *              (TELD.SMNR -> CpmReadSmnRegister).  This is the ACPI BIOS
> + *              telemetry accessor referenced by the AMDHSMP Windows driver
> + *              (amdtelemetry_Acpi_SMNR, method 'RNMS').
> + *   - Writes : there is no corresponding ACPI write method, so SMN writes
> + *              are issued through the kernel SMN helper (amd_smn_hsmp_rdwr).
> + *
> + * The SMN mailbox addresses themselves cannot be discovered from firmware
> + * on the client (no _DSD), so they are fixed; the SMN_HSMP_*_RM
> + * addresses are defined in hsmp.h and shared with the platform driver
> + * (plat.c).
> + */
> +
> +/* ACPI method on \_SB_.TELD that reads one SMN register: SMNR(addr) */
> +#define HSMP_ACPI_SMNR_METHOD		"SMNR"
> +
> +static int hsmp_acpi_smnr_read(struct hsmp_socket *sock, u32 smn_addr, u32 *value)
> +{
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +	unsigned long long out;
> +	acpi_status status;
> +
> +	arg.type		= ACPI_TYPE_INTEGER;
> +	arg.integer.value	= smn_addr;
> +	arg_list.count		= 1;
> +	arg_list.pointer	= &arg;
> +
> +	status = acpi_evaluate_integer(ACPI_HANDLE(sock->dev),
> +				       HSMP_ACPI_SMNR_METHOD, &arg_list, &out);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err_ratelimited(sock->dev,
> +				    "SMNR(0x%08x) ACPI method failed: %s\n",
> +				    smn_addr, acpi_format_exception(status));
> +		return -EIO;
> +	}
> +
> +	*value = (u32)out;
> +	return 0;
> +}
> +
> +static int amd_hsmp_acpi_smn_rdwr(struct hsmp_socket *sock, u32 offset,
> +				  u32 *value, bool write)
> +{
> +	u32 smn_addr = sock->mbinfo.base_addr + offset;
> +
> +	/* Read path uses the firmware SMNR ACPI BIOS method. */
> +	if (!write)
> +		return hsmp_acpi_smnr_read(sock, smn_addr, value);
> +
> +	/* No ACPI write method exists; drive SMN writes through the kernel. */
> +	return amd_smn_hsmp_rdwr(sock->sock_ind, smn_addr, value, true);
> +}
> +
>   /* This is the UUID used for HSMP */
>   static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
>   						0xa6, 0x9f, 0x4e, 0xa2,
> @@ -89,15 +152,19 @@ static inline int hsmp_get_uid(struct device *dev, u16 *sock_ind)
>   	char *uid;
>   
>   	/*
> -	 * UID (ID00, ID01..IDXX) is used for differentiating sockets,
> -	 * read it and strip the "ID" part of it and convert the remaining
> -	 * bytes to integer.
> +	 * Server firmware differentiates the sockets with "ID00", "ID01"..
> +	 * "IDXX", so strip the "ID" before converting the rest.  The client
> +	 * device carries a bare socket number with no prefix to strip, so
> +	 * only skip one when it is actually there.
>   	 */
>   	uid = acpi_device_uid(ACPI_COMPANION(dev));
> -	if (!uid || strlen(uid) < 3)
> +	if (!uid)
>   		return -EINVAL;
>   
> -	return kstrtou16(uid + 2, 10, sock_ind);
> +	if (!strncmp(uid, "ID", 2))
> +		uid += 2;
> +
> +	return kstrtou16(uid, 10, sock_ind);
>   }
>   
>   static acpi_status hsmp_resource(struct acpi_resource *res, void *data)
> @@ -240,12 +307,35 @@ static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
>   	int ret;
>   
>   	sock->sock_ind		= sock_ind;
> -	sock->amd_hsmp_rdwr	= amd_hsmp_acpi_rdwr;
>   
>   	sema_init(&sock->hsmp_sem, 1);
>   
>   	dev_set_drvdata(dev, sock);
>   
> +	/*
> +	 * On the Family 1Ah client platforms the bound ACPI device
> +	 * (\_SB_.TELD) has no _CRS/_DSD, so use the fixed client SMN mailbox
> +	 * addresses and access them via firmware - reads through the SMNR
> +	 * ACPI BIOS method, writes through the kernel SMN helper.
> +	 */
> +	if (is_client_platform()) {
> +		sock->amd_hsmp_rdwr		= amd_hsmp_acpi_smn_rdwr;
> +		sock->mbinfo			= *hsmp_pdev->desc->mbinfo;
> +		dev_info(dev,
> +			 "Client platform: SMN mailbox via SMNR ACPI method (reads) + kernel SMN (writes)\n");
> +		/*
> +		 * Publish sock->dev last, for the same reason as the server
> +		 * path below: it is the readiness gate for the data plane, so
> +		 * it must not become visible before the accessor and the
> +		 * mailbox offsets this socket needs.
> +		 */
> +		smp_store_release(&sock->dev, dev);
> +
> +		return 0;
> +	}
> +
> +	sock->amd_hsmp_rdwr	= amd_hsmp_acpi_rdwr;
> +
>   	/* Read MP1 base address from CRS method */
>   	ret = hsmp_read_acpi_crs(dev, sock);
>   	if (ret)
> @@ -553,11 +643,28 @@ static int init_acpi(struct device *dev)
>   
>   	ret = hsmp_cache_proto_ver(sock_ind);
>   	if (ret) {
> -		dev_err(dev, "Failed to read HSMP protocol version\n");
> -		return ret;
> +		/*
> +		 * Some client SMU builds reject the interface-version query
> +		 * with "invalid message" even though the mailbox is functional
> +		 * (the preceding test message succeeds).  The client does not
> +		 * need the version to reach its metric table, so treat this as
> +		 * non-fatal there, matching the platform driver.
> +		 */
> +		if (is_client_platform()) {
> +			dev_warn(dev,
> +				 "Interface version query unsupported on client SMU; continuing\n");
> +		} else {
> +			dev_err(dev, "Failed to read HSMP protocol version\n");
> +			return ret;
> +		}
>   	}
>   
> -	if (hsmp_pdev->proto_ver >= HSMP_PROTO_VER6) {
> +	/*
> +	 * On client parts the metric table is fetched via
> +	 * HSMP_CLIENT_GET_METRICS_TABLE_DRAM_ADDR whatever interface version
> +	 * was reported, since that query may be unsupported, mirroring plat.c.
> +	 */
> +	if (is_client_platform() || hsmp_pdev->proto_ver >= HSMP_PROTO_VER6) {
>   		ret = hsmp_get_tbl_dram_base(sock_ind);
>   		if (ret)
>   			dev_info(dev, "Failed to init metric table\n");