Hi Song, On 2/11/2022 3:10 AM, Song Liu wrote:
[CAUTION: External Email] On Thu, Feb 10, 2022 at 12:42 PM Nathan Fontenot <nafonten@xxxxxxx> wrote:On 2/10/22 14:32, Song Liu wrote:On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@xxxxxxx> wrote:On 2/9/22 19:10, Song Liu wrote:On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@xxxxxxx> wrote: [...]+ +static const struct hsmp_msg_desc msg_desc_table[] = { + /* num_args, response_size, type */ + {0, 0, RSVD}, /* RESERVED */ + {1, 1, GET}, /* HSMP_TEST */ + {0, 1, GET}, /* HSMP_GET_SMU_VER */ + {0, 1, GET}, /* HSMP_GET_PROTO_VER */ + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */ + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */ + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */ + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */ + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */ + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */ + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */ + {0, 1, GET}, /* HSMP_GET_PROC_HOT */ + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */ + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */ + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */ + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */ + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */ + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */ + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */ + {0, 0, RSVD}, /* RESERVED */ + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */ + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */ +};The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for validating user data. These aren't part of the user API. Perhaps these should be defined in the driver itself instead of being a part of the uapi header.This was my idea. While I agree it is a little weird to have these tables in a uapi header, I think it is helpful to give the user some reference about proper num_args and response_size for each message. I don't have a better idea to achieve this.I like the idea to give users a reference on args and responses for each HSMP function. If this table is kept in the uapi header perhaps we should add a short description of what the expected args and responses are for each HSMP function with a pointer to the full documentation of the HSMP functions in the PPR.I guess we can use unions do give full descriptions, like: struct hsmp_message { __u32 msg_id; /* Message ID */ __u16 num_args; /* Number of arguments in message */ __u16 response_sz; /* Number of expected response words */ union { struct { __u32 args[HSMP_MAX_MSG_LEN]; }; /* ensure size of args */ struct { __u32 test_arg1; } hsmp_test; /* args for other commands */ } args; union { struct { __u32 response[HSMP_MAX_MSG_LEN]; }; /* ensure size of response */ struct { __u32 test_response1; /* or better name */ } hsmp_test; /* reponse for other commands */ } response; __u16 sock_ind; /* socket number */ };I was thinking of keeping the msg_desc_table as is, just provide more details about the expected args and responses in a comment. I think creating a union of structs for each HSMP function (and there are more functions coming) would get a bit messy.Yeah, I think msg_desc_table with detailed comments also works.
HSMP mailbox messages are evolving and each platform defines a supported list of messages.
On a given platform these messages are described in the PPR. Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zipBringing detailed description of these messages from PPR into the kernel would be a
duplicating effort. Which will also bring in challenges such as maintaining the details
for every supported platform and submitting kernel patches for every platform.
We would like to avoid bringing more details of these messages to the kernel documentation.
Such a structure can be described as part of esmi_oob_library to ease user space tool development.
btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above have at most 2 args/responses.The PPR spec defines the args and responses as having up to 8 so we ned to keep the max length at 8. No current HSMP has more than 2 though.Got it. Thanks for the explanation. Song
