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

[Carlos Bilbao <carlos.bilbao@amd.com>]

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?
> 
> [...]
> 
>> +
>> +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.

> 
> [...]

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.

Thanks,
Carlos