Hello, On 2/16/2022 4:06 PM, Song Liu wrote: > On Wed, Feb 16, 2022 at 5:34 AM Naveen Krishna Chatradhi > <nchatrad@xxxxxxx> wrote: >> >> From: Suma Hegde <suma.hegde@xxxxxxx> >> >> 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&data=04%7C01%7Ccarlos.bilbao%40amd.com%7Cb33402053aa6443bf4f308d9f198a7fe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637806460908911848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=F%2BQgWoHlueI1CW14pJHhY4%2FwM5rOTOj0D3IO%2B7yhrbQ%3D&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@xxxxxxx> >> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx> >> Reviewed-by: Carlos Bilbao <carlos.bilbao@xxxxxxx> > > Acked-by: Song Liu <song@xxxxxxxxxx> > > 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
