Re: [PATCH v3 5/7] platform/x86/amd/hsmp: Add IOCTL_GET_TELEMETRY_DATA for metric table reads

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 17 May 2026, Muralidhara M K wrote:

> The metric table for Family 1Ah Model 50h-5Fh
> (struct hsmp_metric_table_zen6) is approximately 13 KB, exceeding the
> PAGE_SIZE (4 KB) cap imposed on the standard sysfs binary attribute
> read path.  Rather than introduce new sysfs infrastructure to support
> binary attributes larger than PAGE_SIZE, expose the metric table
> through the existing HSMP character device using a new ioctl.
> 
> Add struct hsmp_telemetry_data and HSMP_IOCTL_GET_TELEMETRY_DATA to
> the UAPI header.  The request structure carries the socket index, the
> required buffer size and a __u64-encoded user pointer to the
> destination buffer, so the same layout works for 32-bit and 64-bit
> callers.  Fields are ordered with the __u64 user pointer first so all
> members fall on their natural alignment under #pragma pack(4), giving
> a tight 16-byte struct with no implicit padding; the trailing
> reserved __u16 is documented as "set to zero" so future kernels can
> attach meaning to it.  Userspace sizes its buffer using the matching
> UAPI metric table struct (hsmp_metric_table or hsmp_metric_table_zen6)
> for the running platform; sizes that disagree with the firmware-
> reported table size are rejected with -EINVAL so a short copy can
> never silently truncate the snapshot.
> 
> Dispatch hsmp_ioctl() on the ioctl command, route HSMP_IOCTL_CMD to
> the existing message handler (factored out as hsmp_ioctl_msg()) and
> HSMP_IOCTL_GET_TELEMETRY_DATA to a new hsmp_ioctl_get_telemetry()
> helper.  The new helper validates the request, allocates a kernel
> bounce buffer with kvmalloc() so it can hold the full table even
> when it exceeds a single page (zeroing is skipped because the buffer
> is overwritten in full by memcpy_fromio()), calls hsmp_metric_tbl_read()
> to refresh and copy the table from the SMU DRAM region (under the
> per-socket mutex introduced in a follow-up patch), and copies the
> table to userspace.  Unknown ioctl commands now return -ENOTTY instead
> of falling through.
> 
> Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
> Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
> Changes:
> v1->v2: New patch based on bin sysfs
> v2->v3: Replace with IOCTL method
> 
>  arch/x86/include/uapi/asm/amd_hsmp.h | 43 ++++++++++++++
>  drivers/platform/x86/amd/hsmp/hsmp.c | 85 +++++++++++++++++++++++++++-
>  2 files changed, 127 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
> index b86bbc929395..3d085298dd52 100644
> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
> @@ -664,6 +664,40 @@ struct hsmp_metric_table_zen6 {
>  	struct hsmp_metric_table_zen6_ccd ccd[F1A_M50_M5F_MAX_CCD];
>  };
>  
> +/**
> + * struct hsmp_telemetry_data - Request descriptor for HSMP telemetry IOCTL
> + * @buf:       Input. Userspace pointer (encoded as __u64 to keep the layout
> + *             stable between 32-bit and 64-bit callers) to the destination
> + *             buffer that receives the metric table.
> + * @size:      Input. Size in bytes of the buffer pointed to by @buf.  Must
> + *             match the firmware-reported metric table size for the running
> + *             HSMP protocol version (see below); any other value results in
> + *             -EINVAL.  The kernel does not write this field back.
> + * @sock_ind:  Input. Socket index from which the metric table is read.
> + * @reserved:  Reserved for future use.  Callers should set this to zero;
> + *             future kernels may begin interpreting the field, so passing
> + *             a non-zero value today is not forwards compatible.
> + *
> + * Placing @buf first lets all fields fall on their natural alignment under
> + * the surrounding #pragma pack(4), so the struct is a tight 16 bytes with
> + * the same wire layout on 32-bit and 64-bit userspace.
> + *
> + * The exact metric table layout depends on the HSMP protocol version reported
> + * by the firmware:
> + *   - Protocol version 6 -> struct hsmp_metric_table
> + *   - Protocol version 7 -> struct hsmp_metric_table_zen6
> + *
> + * Userspace queries the protocol version (e.g. via the protocol_version sysfs
> + * attribute) and uses sizeof() on the matching UAPI structure for both @size
> + * and the allocation backing @buf.
> + */
> +struct hsmp_telemetry_data {
> +	__u64	buf;
> +	__u32	size;
> +	__u16	sock_ind;
> +	__u16	reserved;
> +};
> +
>  /* Reset to default packing */
>  #pragma pack()
>  
> @@ -671,4 +705,13 @@ struct hsmp_metric_table_zen6 {
>  #define HSMP_BASE_IOCTL_NR	0xF8
>  #define HSMP_IOCTL_CMD		_IOWR(HSMP_BASE_IOCTL_NR, 0, struct hsmp_message)
>  
> +/*
> + * Fetch the firmware metric (telemetry) table for a given socket via the
> + * HSMP character device.  This avoids the PAGE_SIZE limitation of the
> + * sysfs binary attribute path for tables larger than one page (such as the
> + * ~13 KB hsmp_metric_table_zen6 used on Family 1Ah Model 50h-5Fh).
> + */
> +#define HSMP_IOCTL_GET_TELEMETRY_DATA \
> +	_IOWR(HSMP_BASE_IOCTL_NR, 1, struct hsmp_telemetry_data)
> +
>  #endif /*_ASM_X86_AMD_HSMP_H_*/
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index cf9392f99298..3a02d683dea0 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -13,7 +13,9 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/semaphore.h>
> +#include <linux/slab.h>
>  #include <linux/sysfs.h>
> +#include <linux/uaccess.h>
>  
>  #include "hsmp.h"
>  
> @@ -287,7 +289,7 @@ static bool is_get_msg(struct hsmp_message *msg)
>  	return false;
>  }
>  
> -long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> +static long hsmp_ioctl_msg(struct file *fp, unsigned long arg)
>  {
>  	int __user *arguser = (int  __user *)arg;
>  	struct hsmp_message msg = { 0 };
> @@ -343,6 +345,87 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  	return 0;
>  }
>  
> +/*
> + * Fetch the firmware metric (telemetry) table for the requested socket and
> + * copy it to the userspace buffer described by the request.
> + *
> + * The metric table size is variable across HSMP protocol versions and on
> + * Family 1Ah Model 50h-5Fh exceeds PAGE_SIZE.  Userspace must therefore
> + * supply a buffer at least the firmware-reported size in bytes.
> + */
> +static long hsmp_ioctl_get_telemetry(struct file *fp, unsigned long arg)
> +{
> +	void __user *arguser = (void __user *)arg;
> +	struct hsmp_telemetry_data req;
> +	struct hsmp_socket *sock;
> +	void __user *user_buf;
> +	size_t tbl_size;
> +	void *kbuf;
> +	int ret;
> +
> +	/* Telemetry data is read-only; require read access on the fd. */
> +	if (!(fp->f_mode & FMODE_READ))
> +		return -EPERM;
> +
> +	if (copy_from_user(&req, arguser, sizeof(req)))
> +		return -EFAULT;
> +
> +	if (!hsmp_pdev.sock || req.sock_ind >= hsmp_pdev.num_sockets)

Sashiko warns userspace can use this as a speculation device so it needs 
to be protected.

Please also address the req.reserved check mentioned by it with -EINVAL so 
it can actually be used safely in future.

> +		return -ENODEV;
> +
> +	tbl_size = hsmp_pdev.hsmp_table_size;
> +	if (!tbl_size)
> +		return -ENODEV;
> +
> +	/*
> +	 * Userspace must size its buffer using the appropriate UAPI metric
> +	 * table struct for the running protocol version.  Reject mismatched
> +	 * sizes so we never silently truncate or short-write.
> +	 */
> +	if (req.size != tbl_size)
> +		return -EINVAL;
> +
> +	sock = &hsmp_pdev.sock[req.sock_ind];
> +	if (!sock->metric_tbl_addr)
> +		return -ENODEV;
> +
> +	user_buf = u64_to_user_ptr(req.buf);
> +
> +	/*
> +	 * The bounce buffer is overwritten in full by memcpy_fromio() inside
> +	 * hsmp_metric_tbl_read(); use kvmalloc() to avoid the zeroing cost of
> +	 * kvzalloc() on the ~13 KB allocation done on every ioctl call.
> +	 */
> +	kbuf = kvmalloc(tbl_size, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	ret = hsmp_metric_tbl_read(sock, kbuf, tbl_size);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (copy_to_user(user_buf, kbuf, tbl_size))
> +		ret = -EFAULT;
> +	else
> +		ret = 0;
> +
> +out:
> +	kvfree(kbuf);
> +	return ret;
> +}
> +
> +long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case HSMP_IOCTL_CMD:
> +		return hsmp_ioctl_msg(fp, arg);
> +	case HSMP_IOCTL_GET_TELEMETRY_DATA:
> +		return hsmp_ioctl_get_telemetry(fp, arg);
> +	default:
> +		return -ENOTTY;
> +	}
> +}
> +
>  ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
>  {
>  	struct hsmp_message msg = { 0 };
> 

-- 
 i.