Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface

["Chatradhi, Naveen Krishna" <nchatrad@amd.com>]

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@amd.com> wrote:
>> On 2/10/22 14:32, Song Liu wrote:
>>> On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@amd.com> wrote:
>>>> On 2/9/22 19:10, Song Liu wrote:
>>>>> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> 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.zip


Bringing 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