Re: [PATCH 7/7] Documentation/arch/x86/amd_hsmp: Document the SMN and MSR IOCTLs

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

On 7/29/26 11:40, Muralidhara M K wrote:
> Describe HSMP_IOCTL_SMN_CMD and HSMP_IOCTL_MSR_CMD: the payload structs,
> the CAP_SYS_RAWIO and open-mode requirements, and the rule that both are
> read-only, so a request with @write set is refused with -EPERM even for a
> fully privileged caller on an O_RDWR handle.
> 
> Also note that reading the protocol_version attribute can fail on the
> Fam1Ah client platforms, because some client SMU builds reject the
> interface-version query, which is why the telemetry ioctl rather than
> that attribute is the way to reach the client telemetry layout.

I'm happy that these are (at least) defaulting to read-only, but I don't 
understand their purpose.

We already have /dev/cpu/CPUNUM/msr, and we also have amd-node which 
provides access to SMN via /sys/kernel/debug/x86/.

Can you better describe the needs of why these new messages must exist 
and we can't have structured data for them?

Even if amd_hsmp needs to use SMN or MSR to fetch specific data data, 
this feels a lot safer to have a structured specific fetch method than 
such a wide (albeit permissioned) pipe.

> 
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
>   Documentation/arch/x86/amd_hsmp.rst | 44 +++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
> index 8a9d6fe7d4cf..0f03201a9f68 100644
> --- a/Documentation/arch/x86/amd_hsmp.rst
> +++ b/Documentation/arch/x86/amd_hsmp.rst
> @@ -229,6 +229,50 @@ The following IOCTLs are defined:
>     ``sizeof(struct hsmp_metric_table)``.  Later version metrics table
>     layout is documented in the Public PPR.
>   
> +  On the Fam1Ah client platforms reading ``protocol_version`` can fail,
> +  because some client SMU builds reject the interface-version query.
> +  Those parts have their own telemetry layout and this ioctl is the way
> +  to read it.

It seems to me you don't actually describe the telemetry layout or what 
should be done with SMN.

> +
> +``ioctl(file, HSMP_IOCTL_SMN_CMD, struct hsmp_smn_message *smn)``
> +  Read a 32-bit SMN register on the socket given by ``sock_ind``.  This
> +  reaches registers outside the HSMP message ABI, so it requires
> +  ``CAP_SYS_RAWIO`` as well as a handle opened for reading, the latter
> +  because the value read is written back into the request struct.
> +
> +  The ioctl is read-only.  ``write`` must be zero, and a request that
> +  sets it is rejected with ``-EPERM`` even with ``CAP_SYS_RAWIO`` and an
> +  ``O_RDWR`` handle; the field exists only so that write support could be
> +  added later without changing the layout.  ``reserved`` must be zero.
> +  Argument::
> +
> +    struct hsmp_smn_message {
> +        __u32 smn_address; /* SMN address to read */
> +        __u32 value;       /* Value read, filled in by the kernel */
> +        __u16 sock_ind;    /* Socket index */
> +        __u8  write;       /* Must be zero */
> +        __u8  reserved;    /* Reserved, must be zero */
> +    };
> +
> +``ioctl(file, HSMP_IOCTL_MSR_CMD, struct hsmp_msr_message *msr)``
> +  Read a 64-bit MSR on the logical CPU given by ``core_id``, or on
> +  whichever CPU the ioctl runs on when ``core_id`` is
> +  ``HSMP_MSR_ANY_CPU`` (``(__u32)-1``).  Any other ``core_id`` must refer
> +  to an online CPU, otherwise the call fails with ``-EINVAL``.  Requires
> +  ``CAP_SYS_RAWIO`` and a handle opened for reading.
> +
> +  Read-only on the same terms as ``HSMP_IOCTL_SMN_CMD``: a non-zero
> +  ``write`` is rejected with ``-EPERM``, and every byte of ``reserved``
> +  must be zero.  Argument::
> +
> +    struct hsmp_msr_message {
> +        __u32 msr_address; /* MSR index to read */
> +        __u32 core_id;     /* Logical CPU, or HSMP_MSR_ANY_CPU */
> +        __u64 value;       /* Value read, filled in by the kernel */
> +        __u8  write;       /* Must be zero */
> +        __u8  reserved[3]; /* Reserved, must be zero */
> +    };
> +
>   The ioctl would return a non-zero on failure; you can read errno to see
>   what happened. The transaction returns 0 on success.
>