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

["M K, Muralidhara" <muralimk@amd.com>]

On 6/10/2026 4:32 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Thu, 28 May 2026, Muralidhara M K wrote:
> 
>> The metric table needs to be delivered to userspace as a single
>> atomic snapshot, but the current sysfs metrics_bin path is a file
>> read: userspace can read it in chunks and observe a torn snapshot
>> if an SMU refresh happens between read() calls. The same path is
>> also bounded by PAGE_SIZE, so the ~13 KB hsmp_metric_table_zen6
>> used on Family 1Ah Model 50h-5Fh cannot be returned at all,
>> regardless of how userspace reads it. Rather than extend sysfs to
>> lift both restrictions, expose the metric table through the
>> existing HSMP character device using a new ioctl that always copies
>> the full table in one shot.
>>
>> Add struct hsmp_telemetry_data and HSMP_IOCTL_GET_TELEMETRY_DATA
>> to the UAPI header. Under the surrounding #pragma pack(4), placing
>> the __u64 user pointer first gives a tight 16-byte layout that is
>> identical for 32- and 64-bit callers, and the trailing __u16
>> reserved field is rejected with -EINVAL if non-zero so future
>> kernels can repurpose it without breaking already-deployed
>> userspace. Userspace sizes its buffer using the matching UAPI
>> metric-table struct for the running platform; sizes that disagree
>> with the firmware-reported table size are rejected so a short copy
>> can never silently truncate the snapshot.
>>
>> Dispatch hsmp_ioctl() on the ioctl command: the existing message
>> handler is factored out as hsmp_ioctl_msg() for HSMP_IOCTL_CMD, and
>> HSMP_IOCTL_GET_TELEMETRY_DATA goes to a new
>> hsmp_ioctl_get_telemetry() helper.
>>
>> The user-controlled indices reaching kernel arrays via these ioctl
>> paths are clamped with array_index_nospec() to mitigate Spectre v1
>> (CVE-2017-5753): req.sock_ind before indexing hsmp_pdev.sock[], and
>> msg.msg_id once at the entry to hsmp_ioctl_msg() so all six
>> downstream hsmp_msg_desc_table[] dereferences inherit the clamp
>> from a single mask.
>>
>> 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>
>> ---
>>   Documentation/arch/x86/amd_hsmp.rst  |  30 ++++++-
>>   arch/x86/include/uapi/asm/amd_hsmp.h |  43 ++++++++++
>>   drivers/platform/x86/amd/hsmp/hsmp.c | 117 ++++++++++++++++++++++++++-
>>   3 files changed, 188 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
>> index 8bb411f0d70d..74c259507888 100644
>> --- a/Documentation/arch/x86/amd_hsmp.rst
>> +++ b/Documentation/arch/x86/amd_hsmp.rst
>> @@ -68,6 +68,14 @@ under per socket sysfs directory created at
>>
>>   Note: lseek() is not supported as entire metrics table is read.
>>
>> +The sysfs metrics_bin path supports only HSMP protocol version 6 and,
>> +because it is a file read, can return a torn snapshot if userspace
>> +reads in pieces.  Protocol version 7 metric tables
>> +(``struct hsmp_metric_table_zen6``, ~13 KB) also exceed PAGE_SIZE, so
>> +a read returns ``-EOPNOTSUPP``.  For atomic reads on any protocol
>> +version, use the ``HSMP_IOCTL_GET_TELEMETRY_DATA`` ioctl on /dev/hsmp
>> +(see below).
>> +
>>   Metrics table definitions will be documented as part of Public PPR.
>>   The same is defined in the amd_hsmp.h header.
>>
>> @@ -167,7 +175,7 @@ Next thing, open the device file, as follows::
>>       exit(1);
>>     }
>>
>> -The following IOCTL is defined:
>> +The following IOCTLs are defined:
>>
>>   ``ioctl(file, HSMP_IOCTL_CMD, struct hsmp_message *msg)``
>>     The argument is a pointer to a::
>> @@ -180,6 +188,26 @@ The following IOCTL is defined:
>>        __u16   sock_ind;                       /* socket number */
>>       };
>>
>> +``ioctl(file, HSMP_IOCTL_GET_TELEMETRY_DATA, struct hsmp_telemetry_data *req)``
>> +  Atomically fetch the firmware metric (telemetry) table for a socket.
>> +  The ioctl copies the table in one shot, so unlike the metrics_bin
>> +  sysfs path it cannot return a torn snapshot and is not bounded by
>> +  PAGE_SIZE.  Required for HSMP protocol version 7+ (e.g. Family 1Ah
>> +  Model 50h-5Fh, ~13 KB ``struct hsmp_metric_table_zen6``).  Argument::
>> +
>> +    struct hsmp_telemetry_data {
>> +     __u64 buf; /* User pointer to destination buffer */
>> +     __u32 size; /* Size of @buf, must equal firmware-reported table size */
>> +     __u16 sock_ind; /* Socket index */
>> +     __u16 reserved; /* Reserved, must be zero */
>> +    };
>> +
>> +  Userspace picks the layout matching the running protocol version
>> +  (``struct hsmp_metric_table`` for v6, ``struct hsmp_metric_table_zen6``
>> +  for v7, queried via the ``protocol_version`` sysfs attribute) and
>> +  uses ``sizeof()`` of that struct for ``size``.  The kernel rejects
>> +  a mismatched ``size`` or non-zero ``reserved`` with ``-EINVAL``.
>> +
>>   The ioctl would return a non-zero on failure; you can read errno to see
>>   what happened. The transaction returns 0 on success.
>>
>> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
>> index 438ac38d0dc8..50b28ccc7597 100644
>> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
>> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
>> @@ -686,6 +686,40 @@ struct hsmp_metric_table_zen6 {
>>        struct hsmp_metric_table_zen6_ccd ccd[HSMP_F1A_M50_M5F_MAX_CCDS];
>>   };
>>
>> +/**
>> + * 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()
>>
>> @@ -693,4 +727,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..67f0074bb532 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -12,8 +12,11 @@
>>   #include <linux/acpi.h>
>>   #include <linux/delay.h>
>>   #include <linux/device.h>
>> +#include <linux/nospec.h>
>>   #include <linux/semaphore.h>
>> +#include <linux/slab.h>
>>   #include <linux/sysfs.h>
>> +#include <linux/uaccess.h>
>>
>>   #include "hsmp.h"
>>
>> @@ -287,7 +290,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 };
>> @@ -303,6 +306,19 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>>        if (msg.msg_id < HSMP_TEST || msg.msg_id >= HSMP_MSG_ID_MAX)
>>                return -ENOMSG;
>>
>> +     /*
>> +      * Sanitize the user-controlled msg_id against speculative
>> +      * execution.  The bounds check above retires the out-of-range
>> +      * case with -ENOMSG, but a mispredicted branch can still let the
>> +      * CPU speculatively use msg_id as an index into
>> +      * hsmp_msg_desc_table[] (here and in validate_message() /
>> +      * is_get_msg() called downstream via hsmp_send_message()), and
>> +      * pull arbitrary kernel memory into the cache (Spectre v1,
>> +      * CVE-2017-5753).  Clamp once into msg.msg_id so every downstream
>> +      * dereference sees the sanitized value.
>> +      */
>> +     msg.msg_id = array_index_nospec(msg.msg_id, HSMP_MSG_ID_MAX);
>> +
> 
> This is a good change, but it looks to me it's fixing an existing problem,
> that is unrelated to this patch which is supposed to be about:
> 
> "Add IOCTL_GET_TELEMETRY_DATA for metric table reads"
> 
> So I think it should be done in own patch.
> 
Sure. I will make this as a seperate patch.

>>        switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
>>        case FMODE_WRITE:
>>                /*
>> @@ -343,6 +359,105 @@ 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.
> 
> Here the comment says "at least" but you check it's exactly equal to the
> required size?
> 
>> + */
>> +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;
>> +     unsigned int sock_ind;
>> +     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;
>> +
>> +     /*
>> +      * Reserved fields must be zero so future kernels can safely
>> +      * repurpose them without breaking already-deployed userspace.
>> +      */
>> +     if (req.reserved)
>> +             return -EINVAL;
>> +
>> +     if (!hsmp_pdev.sock || req.sock_ind >= hsmp_pdev.num_sockets)
>> +             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;
>> +
>> +     /*
>> +      * Sanitize the user-controlled socket index against speculative
>> +      * execution.  The bounds check above retires the out-of-range case
>> +      * with -ENODEV, but a mispredicted branch can still let the CPU
>> +      * speculatively use sock_ind as an index into hsmp_pdev.sock[] and
>> +      * pull arbitrary kernel memory into the cache (Spectre v1, CVE-2017-
>> +      * 5753).  array_index_nospec() turns the bounds check into a
>> +      * data-flow clamp so the speculative load is in-range too.
>> +      */
>> +     sock_ind = array_index_nospec(req.sock_ind, hsmp_pdev.num_sockets);
>> +     sock = &hsmp_pdev.sock[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.
>