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

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

Hi Song, Carlos

On 2/17/2022 3:54 AM, Carlos Bilbao wrote:
> Hello,
>
> On 2/16/2022 4:06 PM, Song Liu wrote:
>> On Wed, Feb 16, 2022 at 5:34 AM Naveen Krishna Chatradhi
>> <nchatrad@amd.com> wrote:
>>> From: Suma Hegde <suma.hegde@amd.com>
>>>
>>> Recent Fam19h EPYC server line of processors from AMD support system
>>> management functionality via HSMP (Host System Management Port) interface.
>>>
>>> The Host System Management Port (HSMP) is an interface to provide
>>> OS-level software with access to system management functions via a
>>> set of mailbox registers.
>>>
>>> More details on the interface can be found in chapter
>>> "7 Host System Management Port (HSMP)" of the following PPR
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F55898_B1_pub_0.50.zip&amp;data=04%7C01%7Ccarlos.bilbao%40amd.com%7Cb33402053aa6443bf4f308d9f198a7fe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637806460908911848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=F%2BQgWoHlueI1CW14pJHhY4%2FwM5rOTOj0D3IO%2B7yhrbQ%3D&amp;reserved=0
>>>
>>> This patch adds new amd_hsmp module under the drivers/platforms/x86/
>>> which creates miscdevice with an IOCTL interface to the user space.
>>> /dev/hsmp is for running the hsmp mailbox commands.
>>>
>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>> Reviewed-by: Carlos Bilbao <carlos.bilbao@amd.com>
>> Acked-by: Song Liu <song@kernel.org>
>>
>> With a couple minor comments below.
>>
>>> ---
>> [...]
>>
>>> +
>>> +struct hsmp_message {
>>> +       __u32   msg_id;                 /* Message ID */
>>> +       __u16   num_args;               /* Number of input argument words in message */
>>> +       __u16   response_sz;            /* Number of expected output/response words */
>>> +       __u32   buf[HSMP_MAX_MSG_LEN];  /* argument/response buffer */
>> How about we call these args instead of buf?

Now that we are using this member for both input arguments and output 
response, I've changed it to a

generic name, no problem will change it back to args.

>>
>> [...]
>>
>>> +
>>> +static long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>>> +{
>>> +       int __user *arguser = (int  __user *)arg;
>>> +       struct hsmp_message msg = { 0 };
>>> +       int ret;
>>> +
>>> +       if (copy_struct_from_user(&msg, sizeof(msg), arguser, sizeof(struct hsmp_message)))
>>> +               return -EFAULT;
>>> +
>>> +       ret = validate_message(&msg);
>> We call validate_message() twice in this path. This is not a big issue, but it
>> will be nice if we can avoid the extra check.
> Yes, we can probably just rely on hsmp_send_message. We shouldn't remove it
> from there since we export that function.

We are using msg_id to reference elements from the table. so, i will 
validate the msg.msg_id

is with in the array bounds of hsmp_msg_desc_table[] here.

>
>> [...]
> I have reviewed the patch, compiled with several configurations (including
> allyes) and functionally tested. Also not a single hole on pahole.
> Everything looks in great shape to me.
Thank you.
>
> Thanks,
> Carlos