On Wed, Feb 16, 2022 at 10:25 PM Chatradhi, Naveen Krishna <nchatrad@xxxxxxx> wrote: > > 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@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? > > 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. Yeah, let's keep both checks for now. Song
