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

[Ilpo Järvinen]

On Fri, 22 May 2026, Ilpo Järvinen wrote:

> 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@xxxxxxx>
> > Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@xxxxxxx>
> > Signed-off-by: Muralidhara M K <muralidhara.mk@xxxxxxx>
> > ---
> > 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.

Hi again,

While looking at the other patches, I also noticed hsmp_ioctl() has a 
similar userspace driver check (in pre-existing code):

	if (msg.msg_id < HSMP_TEST || msg.msg_id >= HSMP_MSG_ID_MAX)

--
 i.

> 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 };
> > 
> 
>