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

[Nathan Fontenot <nafonten@amd.com>]

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.

> 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.

-Nathan

> 
> Thanks,
> Song