| Message ID | 20210111225121.820014-11-ben.widawsky@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3,01/16] docs: cxl: Add basic documentation | expand |
On Mon, 11 Jan 2021 14:51:14 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > The send command allows userspace to issue mailbox commands directly to > the hardware. The driver will verify basic properties of the command and > possible inspect the input (or output) payload to determine whether or > not the command is allowed (or might taint the kernel). > > The list of allowed commands and their properties can be determined by > using the QUERY IOCTL for CXL memory devices. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++- > include/uapi/linux/cxl_mem.h | 39 +++++++ > 2 files changed, 239 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index d4eb3f5b9469..f979788b4d9f 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr); > /* protect cxl_mem_idr allocations */ > static DEFINE_MUTEX(cxl_memdev_lock); > > +#undef C > +#define C(a, b) { b } I'm not following why this is here? > +static struct { > + const char *name; > +} command_names[] = { CMDS }; > +#undef C > + > #define CXL_CMD(_id, _flags, sin, sout, f) \ > [CXL_MEM_COMMAND_ID_##_id] = { \ > { \ ... > + > +/** > + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > + * @cxlmd: The CXL memory device to communicate with. > + * @cmd: The validated command. > + * @in_payload: Pointer to userspace's input payload. > + * @out_payload: Pointer to userspace's output payload. > + * @u: The command submitted by userspace. Has output fields. > + * > + * Return: > + * * %0 - Mailbox transaction succeeded. > + * * %-EFAULT - Something happened with copy_to/from_user. > + * * %-EINTR - Mailbox acquisition interrupted. > + * * %-E2BIG - Output payload would overrun buffer. > + * > + * Creates the appropriate mailbox command on behalf of a userspace request. > + * Return value, size, and output payload are all copied out to @u. The > + * parameters for the command must be validated before calling this function. > + * > + * A 0 return code indicates the command executed successfully, not that it was > + * itself successful. IOW, the retval should always be checked if wanting to cmd->retval perhaps to be more explicit? > + * determine the actual result. > + */ > +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd, > + const struct cxl_mem_command *cmd, > + u64 in_payload, > + u64 out_payload, > + struct cxl_send_command __user *u) > +{ > + struct mbox_cmd mbox_cmd = { > + .opcode = cmd->opcode, > + .size_in = cmd->info.size_in, > + .payload = NULL, /* Copied by copy_to|from_user() */ > + }; > + int rc; > + > + if (cmd->info.size_in) { > + /* > + * Directly copy the userspace payload into the hardware. UAPI > + * states that the buffer must already be little endian. > + */ > + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm), > + u64_to_user_ptr(in_payload), > + cmd->info.size_in)) { > + cxl_mem_mbox_put(cxlmd->cxlm); mbox_get is after this point though it shouldn't be given we just wrote into the mbox registers. This seems unlikely to be a high performance path, so perhaps just use a local buffer and let cxl_mem_mbox_send_cmd copy it into the registers. > + return -EFAULT; > + } > + } > + > + rc = cxl_mem_mbox_get(cxlmd->cxlm, true); > + if (rc) > + return rc; > + > + dev_dbg(&cxlmd->dev, > + "Submitting %s command for user\n" > + "\topcode: %x\n" > + "\tsize: %ub\n", > + command_names[cmd->info.id].name, mbox_cmd.opcode, > + cmd->info.size_in); > + > + rc = cxl_mem_mbox_send_cmd(cxlmd->cxlm, &mbox_cmd); > + cxl_mem_mbox_put(cxlmd->cxlm); > + if (rc) > + return rc; > + > + if (mbox_cmd.size_out > cmd->info.size_out) > + return -E2BIG; > + > + rc = put_user(mbox_cmd.return_code, &u->retval); > + if (rc) > + return rc; > + > + rc = put_user(mbox_cmd.size_out, &u->size_out); > + if (rc) > + return rc; > + > + if (mbox_cmd.size_out) > + if (copy_to_user(u64_to_user_ptr(out_payload), > + (__force void *)cxl_payload_regs(cxlmd->cxlm), > + mbox_cmd.size_out)) > + return -EFAULT; > + > + return 0; > +} > + ... > > static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > @@ -479,6 +644,37 @@ static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg > if (j == n_commands) > break; > } > + > + return 0; Ah. That should have been in the earlier patch. Explains why the code works :) > + } else if (cmd == CXL_MEM_SEND_COMMAND) { > + struct cxl_send_command send, __user *u = (void __user *)arg; > + struct cxl_mem_command c; > + int rc; > + > + dev_dbg(dev, "Send IOCTL\n"); > + > + if (copy_from_user(&send, u, sizeof(send))) > + return -EFAULT; > + > + rc = device_lock_interruptible(dev); > + if (rc) > + return rc; > + > + if (!get_live_device(dev)) { > + device_unlock(dev); > + return -ENXIO; > + } > + > + rc = cxl_validate_cmd_from_user(cxlmd->cxlm, &send, &c); > + if (!rc) > + rc = handle_mailbox_cmd_from_user(cxlmd, &c, > + send.in_payload, > + send.out_payload, u); > + > + put_device(dev); > + device_unlock(dev); > + > + return rc; > } > > return -ENOTTY; > @@ -837,7 +1033,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm) > int rc; > > /* Retrieve initial device memory map */ > - rc = cxl_mem_mbox_get(cxlm); > + rc = cxl_mem_mbox_get(cxlm, false); > if (rc) > return rc; > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > index 847f825bbe18..cb4e2bee5228 100644 > --- a/include/uapi/linux/cxl_mem.h > +++ b/include/uapi/linux/cxl_mem.h > @@ -26,6 +26,7 @@ extern "C" { > */ > > #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands) > +#define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command) > > #undef CMDS > #define CMDS \ > @@ -69,6 +70,7 @@ struct cxl_command_info { > #define CXL_MEM_COMMAND_FLAG_NONE 0 > #define CXL_MEM_COMMAND_FLAG_KERNEL BIT(0) > #define CXL_MEM_COMMAND_FLAG_MUTEX BIT(1) > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(31, 2) Instinctively I'd expect FLAG_MASK to be GENMASK(1, 0) and to be used as ~FLAG_MASK. As it's mask of flags, not the mask to leave only valid flags. > > __s32 size_in; > __s32 size_out; > @@ -110,6 +112,43 @@ struct cxl_mem_query_commands { > struct cxl_command_info __user commands[]; /* out: supported commands */ > }; > > +/** > + * struct cxl_send_command - Send a command to a memory device. > + * @id: The command to send to the memory device. This must be one of the > + * commands returned by the query command. > + * @flags: Flags for the command (input). > + * @rsvd: Must be zero. > + * @retval: Return value from the memory device (output). > + * @size_in: Size of the payload to provide to the device (input). > + * @size_out: Size of the payload received from the device (input/output). This > + * field is filled in by userspace to let the driver know how much > + * space was allocated for output. It is populated by the driver to > + * let userspace know how large the output payload actually was. > + * @in_payload: Pointer to memory for payload input (little endian order). > + * @out_payload: Pointer to memory for payload output (little endian order). > + * > + * Mechanism for userspace to send a command to the hardware for processing. The > + * driver will do basic validation on the command sizes, but the payload input > + * and output are not introspected. Userspace is required to allocate large > + * enough buffers for max(size_in, size_out). That sounds like both buffers must be the maximum between size_in and size_out. Is intent that this is the maximum size_in for in_payload and max(size_out) for out_payload? > + */ > +struct cxl_send_command { > + __u32 id; > + __u32 flags; > + __u32 rsvd; > + __u32 retval; > + > + struct { > + __s32 size_in; > + __u64 in_payload; > + }; > + > + struct { > + __s32 size_out; > + __u64 out_payload; > + }; > +}; > + > #if defined(__cplusplus) > } > #endif
On 21-01-14 17:10:38, Jonathan Cameron wrote: > On Mon, 11 Jan 2021 14:51:14 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > The send command allows userspace to issue mailbox commands directly to > > the hardware. The driver will verify basic properties of the command and > > possible inspect the input (or output) payload to determine whether or > > not the command is allowed (or might taint the kernel). > > > > The list of allowed commands and their properties can be determined by > > using the QUERY IOCTL for CXL memory devices. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++- > > include/uapi/linux/cxl_mem.h | 39 +++++++ > > 2 files changed, 239 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index d4eb3f5b9469..f979788b4d9f 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr); > > /* protect cxl_mem_idr allocations */ > > static DEFINE_MUTEX(cxl_memdev_lock); > > > > +#undef C > > +#define C(a, b) { b } > > I'm not following why this is here? > It's used for a debug message in handle_mailbox_cmd_from_user(). This is all the macro magic stolen from ftrace. Or, did I miss the question? > > +static struct { > > + const char *name; > > +} command_names[] = { CMDS }; > > +#undef C > > + > > #define CXL_CMD(_id, _flags, sin, sout, f) \ > > [CXL_MEM_COMMAND_ID_##_id] = { \ > > { \ > ... > > > + > > +/** > > + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > > + * @cxlmd: The CXL memory device to communicate with. > > + * @cmd: The validated command. > > + * @in_payload: Pointer to userspace's input payload. > > + * @out_payload: Pointer to userspace's output payload. > > + * @u: The command submitted by userspace. Has output fields. > > + * > > + * Return: > > + * * %0 - Mailbox transaction succeeded. > > + * * %-EFAULT - Something happened with copy_to/from_user. > > + * * %-EINTR - Mailbox acquisition interrupted. > > + * * %-E2BIG - Output payload would overrun buffer. > > + * > > + * Creates the appropriate mailbox command on behalf of a userspace request. > > + * Return value, size, and output payload are all copied out to @u. The > > + * parameters for the command must be validated before calling this function. > > + * > > + * A 0 return code indicates the command executed successfully, not that it was > > + * itself successful. IOW, the retval should always be checked if wanting to > > cmd->retval perhaps to be more explicit? > > > + * determine the actual result. > > + */ > > +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd, > > + const struct cxl_mem_command *cmd, > > + u64 in_payload, > > + u64 out_payload, > > + struct cxl_send_command __user *u) > > +{ > > + struct mbox_cmd mbox_cmd = { > > + .opcode = cmd->opcode, > > + .size_in = cmd->info.size_in, > > + .payload = NULL, /* Copied by copy_to|from_user() */ > > + }; > > + int rc; > > + > > + if (cmd->info.size_in) { > > + /* > > + * Directly copy the userspace payload into the hardware. UAPI > > + * states that the buffer must already be little endian. > > + */ > > + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm), > > + u64_to_user_ptr(in_payload), > > + cmd->info.size_in)) { > > + cxl_mem_mbox_put(cxlmd->cxlm); > > mbox_get is after this point though it shouldn't be given we just > wrote into the mbox registers. > > This seems unlikely to be a high performance path, so perhaps just > use a local buffer and let cxl_mem_mbox_send_cmd copy it into the registers. > You're correct about the get() needing to be first. I will fix it. As for performance path - so while this does potentially help with performance, it actually ends up being I think a little cleaner to not have to deal with a local buffer. How strongly do you feel about it? I'd say if you don't care so much, let's keep it as is and find a reason to undo later. > > + return -EFAULT; > > + } > > + } > > + > > + rc = cxl_mem_mbox_get(cxlmd->cxlm, true); > > + if (rc) > > + return rc; > > + > > + dev_dbg(&cxlmd->dev, > > + "Submitting %s command for user\n" > > + "\topcode: %x\n" > > + "\tsize: %ub\n", > > + command_names[cmd->info.id].name, mbox_cmd.opcode, > > + cmd->info.size_in); > > + > > + rc = cxl_mem_mbox_send_cmd(cxlmd->cxlm, &mbox_cmd); > > + cxl_mem_mbox_put(cxlmd->cxlm); > > + if (rc) > > + return rc; > > + > > + if (mbox_cmd.size_out > cmd->info.size_out) > > + return -E2BIG; > > + > > + rc = put_user(mbox_cmd.return_code, &u->retval); > > + if (rc) > > + return rc; > > + > > + rc = put_user(mbox_cmd.size_out, &u->size_out); > > + if (rc) > > + return rc; > > + > > + if (mbox_cmd.size_out) > > + if (copy_to_user(u64_to_user_ptr(out_payload), > > + (__force void *)cxl_payload_regs(cxlmd->cxlm), > > + mbox_cmd.size_out)) > > + return -EFAULT; > > + > > + return 0; > > +} > > + > > ... Yeah... > > > > > static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > @@ -479,6 +644,37 @@ static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg > > if (j == n_commands) > > break; > > } > > + > > + return 0; > > Ah. That should have been in the earlier patch. Explains why the code works :) > > > > + } else if (cmd == CXL_MEM_SEND_COMMAND) { > > + struct cxl_send_command send, __user *u = (void __user *)arg; > > + struct cxl_mem_command c; > > + int rc; > > + > > + dev_dbg(dev, "Send IOCTL\n"); > > + > > + if (copy_from_user(&send, u, sizeof(send))) > > + return -EFAULT; > > + > > + rc = device_lock_interruptible(dev); > > + if (rc) > > + return rc; > > + > > + if (!get_live_device(dev)) { > > + device_unlock(dev); > > + return -ENXIO; > > + } > > + > > + rc = cxl_validate_cmd_from_user(cxlmd->cxlm, &send, &c); > > + if (!rc) > > + rc = handle_mailbox_cmd_from_user(cxlmd, &c, > > + send.in_payload, > > + send.out_payload, u); > > + > > + put_device(dev); > > + device_unlock(dev); > > + > > + return rc; > > } > > > > return -ENOTTY; > > @@ -837,7 +1033,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm) > > int rc; > > > > /* Retrieve initial device memory map */ > > - rc = cxl_mem_mbox_get(cxlm); > > + rc = cxl_mem_mbox_get(cxlm, false); > > if (rc) > > return rc; > > > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > > index 847f825bbe18..cb4e2bee5228 100644 > > --- a/include/uapi/linux/cxl_mem.h > > +++ b/include/uapi/linux/cxl_mem.h > > @@ -26,6 +26,7 @@ extern "C" { > > */ > > > > #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands) > > +#define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command) > > > > #undef CMDS > > #define CMDS \ > > @@ -69,6 +70,7 @@ struct cxl_command_info { > > #define CXL_MEM_COMMAND_FLAG_NONE 0 > > #define CXL_MEM_COMMAND_FLAG_KERNEL BIT(0) > > #define CXL_MEM_COMMAND_FLAG_MUTEX BIT(1) > > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(31, 2) > > Instinctively I'd expect FLAG_MASK to be GENMASK(1, 0) > and to be used as ~FLAG_MASK. As it's mask of flags, not > the mask to leave only valid flags. > Fine with me. > > > > __s32 size_in; > > __s32 size_out; > > @@ -110,6 +112,43 @@ struct cxl_mem_query_commands { > > struct cxl_command_info __user commands[]; /* out: supported commands */ > > }; > > > > +/** > > + * struct cxl_send_command - Send a command to a memory device. > > + * @id: The command to send to the memory device. This must be one of the > > + * commands returned by the query command. > > + * @flags: Flags for the command (input). > > + * @rsvd: Must be zero. > > + * @retval: Return value from the memory device (output). > > + * @size_in: Size of the payload to provide to the device (input). > > + * @size_out: Size of the payload received from the device (input/output). This > > + * field is filled in by userspace to let the driver know how much > > + * space was allocated for output. It is populated by the driver to > > + * let userspace know how large the output payload actually was. > > + * @in_payload: Pointer to memory for payload input (little endian order). > > + * @out_payload: Pointer to memory for payload output (little endian order). > > + * > > + * Mechanism for userspace to send a command to the hardware for processing. The > > + * driver will do basic validation on the command sizes, but the payload input > > + * and output are not introspected. Userspace is required to allocate large > > + * enough buffers for max(size_in, size_out). > > That sounds like both buffers must be the maximum between size_in and size_out. > Is intent that this is the maximum size_in for in_payload and max(size_out) for out_payload? This comment reflects the way the interface was in v2. It needs fixing. > > > + */ > > +struct cxl_send_command { > > + __u32 id; > > + __u32 flags; > > + __u32 rsvd; > > + __u32 retval; > > + > > + struct { > > + __s32 size_in; > > + __u64 in_payload; > > + }; > > + > > + struct { > > + __s32 size_out; > > + __u64 out_payload; > > + }; > > +}; > > + > > #if defined(__cplusplus) > > } > > #endif >
On Thu, 21 Jan 2021 10:15:46 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > On 21-01-14 17:10:38, Jonathan Cameron wrote: > > On Mon, 11 Jan 2021 14:51:14 -0800 > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > The send command allows userspace to issue mailbox commands directly to > > > the hardware. The driver will verify basic properties of the command and > > > possible inspect the input (or output) payload to determine whether or > > > not the command is allowed (or might taint the kernel). > > > > > > The list of allowed commands and their properties can be determined by > > > using the QUERY IOCTL for CXL memory devices. > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > --- > > > drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++- > > > include/uapi/linux/cxl_mem.h | 39 +++++++ > > > 2 files changed, 239 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index d4eb3f5b9469..f979788b4d9f 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr); > > > /* protect cxl_mem_idr allocations */ > > > static DEFINE_MUTEX(cxl_memdev_lock); > > > > > > +#undef C > > > +#define C(a, b) { b } > > > > I'm not following why this is here? > > > > It's used for a debug message in handle_mailbox_cmd_from_user(). This is all the > macro magic stolen from ftrace. Or, did I miss the question? > > > > +static struct { > > > + const char *name; > > > +} command_names[] = { CMDS }; > > > +#undef C Mostly that you define it then undef it without use that I can see. > > > + > > > #define CXL_CMD(_id, _flags, sin, sout, f) \ > > > [CXL_MEM_COMMAND_ID_##_id] = { \ > > > { \ > > ... > > > > > + > > > +/** > > > + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > > > + * @cxlmd: The CXL memory device to communicate with. > > > + * @cmd: The validated command. > > > + * @in_payload: Pointer to userspace's input payload. > > > + * @out_payload: Pointer to userspace's output payload. > > > + * @u: The command submitted by userspace. Has output fields. > > > + * > > > + * Return: > > > + * * %0 - Mailbox transaction succeeded. > > > + * * %-EFAULT - Something happened with copy_to/from_user. > > > + * * %-EINTR - Mailbox acquisition interrupted. > > > + * * %-E2BIG - Output payload would overrun buffer. > > > + * > > > + * Creates the appropriate mailbox command on behalf of a userspace request. > > > + * Return value, size, and output payload are all copied out to @u. The > > > + * parameters for the command must be validated before calling this function. > > > + * > > > + * A 0 return code indicates the command executed successfully, not that it was > > > + * itself successful. IOW, the retval should always be checked if wanting to > > > > cmd->retval perhaps to be more explicit? > > > > > + * determine the actual result. > > > + */ > > > +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd, > > > + const struct cxl_mem_command *cmd, > > > + u64 in_payload, > > > + u64 out_payload, > > > + struct cxl_send_command __user *u) > > > +{ > > > + struct mbox_cmd mbox_cmd = { > > > + .opcode = cmd->opcode, > > > + .size_in = cmd->info.size_in, > > > + .payload = NULL, /* Copied by copy_to|from_user() */ > > > + }; > > > + int rc; > > > + > > > + if (cmd->info.size_in) { > > > + /* > > > + * Directly copy the userspace payload into the hardware. UAPI > > > + * states that the buffer must already be little endian. > > > + */ > > > + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm), > > > + u64_to_user_ptr(in_payload), > > > + cmd->info.size_in)) { > > > + cxl_mem_mbox_put(cxlmd->cxlm); > > > > mbox_get is after this point though it shouldn't be given we just > > wrote into the mbox registers. > > > > This seems unlikely to be a high performance path, so perhaps just > > use a local buffer and let cxl_mem_mbox_send_cmd copy it into the registers. > > > > You're correct about the get() needing to be first. I will fix it. As for > performance path - so while this does potentially help with performance, it > actually ends up being I think a little cleaner to not have to deal with a local > buffer. > > How strongly do you feel about it? I'd say if you don't care so much, let's keep > it as is and find a reason to undo later. A slightly interesting corner. The fact that there are no other cases of this particular sequence in kernel bothered me... It's more than possible I've missed something in the following. So with a bounce buffered we'd have copy_from_user() then memcpy_toio() here we end loosing the fact that memcpy_to_io() might not be a 'simple' memcpy(). In the generic asm form it's just a (__force void *) like you have here done using __io_virt() (which might make sense here if you keep this, to make it clear what's going on) However, not all architectures are using the generic form of memcpy_toio() and even if the ones we care about are safe today using the above construct, it's more than possible some future architecture might be more 'exciting'. So basically I'm doubtful that this construct is safe. Jonathan
On 21-01-22 11:43:57, Jonathan Cameron wrote: > On Thu, 21 Jan 2021 10:15:46 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > On 21-01-14 17:10:38, Jonathan Cameron wrote: > > > On Mon, 11 Jan 2021 14:51:14 -0800 > > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > The send command allows userspace to issue mailbox commands directly to > > > > the hardware. The driver will verify basic properties of the command and > > > > possible inspect the input (or output) payload to determine whether or > > > > not the command is allowed (or might taint the kernel). > > > > > > > > The list of allowed commands and their properties can be determined by > > > > using the QUERY IOCTL for CXL memory devices. > > > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > --- > > > > drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++- > > > > include/uapi/linux/cxl_mem.h | 39 +++++++ > > > > 2 files changed, 239 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > index d4eb3f5b9469..f979788b4d9f 100644 > > > > --- a/drivers/cxl/mem.c > > > > +++ b/drivers/cxl/mem.c > > > > @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr); > > > > /* protect cxl_mem_idr allocations */ > > > > static DEFINE_MUTEX(cxl_memdev_lock); > > > > > > > > +#undef C > > > > +#define C(a, b) { b } > > > > > > I'm not following why this is here? > > > > > > > It's used for a debug message in handle_mailbox_cmd_from_user(). This is all the > > macro magic stolen from ftrace. Or, did I miss the question? > > > > > > +static struct { > > > > + const char *name; > > > > +} command_names[] = { CMDS }; > > > > +#undef C > > Mostly that you define it then undef it without use that I can see. > > > > > + > > > > #define CXL_CMD(_id, _flags, sin, sout, f) \ > > > > [CXL_MEM_COMMAND_ID_##_id] = { \ > > > > { \ > > > ... > > > > > > > + > > > > +/** > > > > + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > > > > + * @cxlmd: The CXL memory device to communicate with. > > > > + * @cmd: The validated command. > > > > + * @in_payload: Pointer to userspace's input payload. > > > > + * @out_payload: Pointer to userspace's output payload. > > > > + * @u: The command submitted by userspace. Has output fields. > > > > + * > > > > + * Return: > > > > + * * %0 - Mailbox transaction succeeded. > > > > + * * %-EFAULT - Something happened with copy_to/from_user. > > > > + * * %-EINTR - Mailbox acquisition interrupted. > > > > + * * %-E2BIG - Output payload would overrun buffer. > > > > + * > > > > + * Creates the appropriate mailbox command on behalf of a userspace request. > > > > + * Return value, size, and output payload are all copied out to @u. The > > > > + * parameters for the command must be validated before calling this function. > > > > + * > > > > + * A 0 return code indicates the command executed successfully, not that it was > > > > + * itself successful. IOW, the retval should always be checked if wanting to > > > > > > cmd->retval perhaps to be more explicit? > > > > > > > + * determine the actual result. > > > > + */ > > > > +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd, > > > > + const struct cxl_mem_command *cmd, > > > > + u64 in_payload, > > > > + u64 out_payload, > > > > + struct cxl_send_command __user *u) > > > > +{ > > > > + struct mbox_cmd mbox_cmd = { > > > > + .opcode = cmd->opcode, > > > > + .size_in = cmd->info.size_in, > > > > + .payload = NULL, /* Copied by copy_to|from_user() */ > > > > + }; > > > > + int rc; > > > > + > > > > + if (cmd->info.size_in) { > > > > + /* > > > > + * Directly copy the userspace payload into the hardware. UAPI > > > > + * states that the buffer must already be little endian. > > > > + */ > > > > + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm), > > > > + u64_to_user_ptr(in_payload), > > > > + cmd->info.size_in)) { > > > > + cxl_mem_mbox_put(cxlmd->cxlm); > > > > > > mbox_get is after this point though it shouldn't be given we just > > > wrote into the mbox registers. > > > > > > This seems unlikely to be a high performance path, so perhaps just > > > use a local buffer and let cxl_mem_mbox_send_cmd copy it into the registers. > > > > > > > You're correct about the get() needing to be first. I will fix it. As for > > performance path - so while this does potentially help with performance, it > > actually ends up being I think a little cleaner to not have to deal with a local > > buffer. > > > > How strongly do you feel about it? I'd say if you don't care so much, let's keep > > it as is and find a reason to undo later. > > A slightly interesting corner. The fact that there are no other cases of this > particular sequence in kernel bothered me... It's more than possible I've > missed something in the following. > > So with a bounce buffered we'd have > copy_from_user() > then > memcpy_toio() > > here we end loosing the fact that memcpy_to_io() might not be a 'simple' memcpy(). > In the generic asm form it's just a (__force void *) like you have here done using > __io_virt() (which might make sense here if you keep this, to make it clear > what's going on) > > However, not all architectures are using the generic form of memcpy_toio() > and even if the ones we care about are safe today using the above construct, > it's more than possible some future architecture might be more 'exciting'. > > So basically I'm doubtful that this construct is safe. > > Jonathan > Sounds reasonable. Thanks for digging. I'll go back to the bounce buffer in v4.
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index d4eb3f5b9469..f979788b4d9f 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr); /* protect cxl_mem_idr allocations */ static DEFINE_MUTEX(cxl_memdev_lock); +#undef C +#define C(a, b) { b } +static struct { + const char *name; +} command_names[] = { CMDS }; +#undef C + #define CXL_CMD(_id, _flags, sin, sout, f) \ [CXL_MEM_COMMAND_ID_##_id] = { \ { \ @@ -319,16 +326,25 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, /** * cxl_mem_mbox_get() - Acquire exclusive access to the mailbox. * @cxlm: The memory device to gain access to. + * @interruptible: Allow interrupting mutex acquisition. * * Context: Any context. Takes the mbox_lock. * Return: 0 if exclusive access was acquired. */ -static int cxl_mem_mbox_get(struct cxl_mem *cxlm) +static int cxl_mem_mbox_get(struct cxl_mem *cxlm, bool interruptible) { u64 md_status; - int rc = -EBUSY; + int rc; + + if (interruptible) { + rc = mutex_lock_interruptible(&cxlm->mbox.mutex); + if (rc) + return rc; + } else { + mutex_lock_io(&cxlm->mbox.mutex); + } - mutex_lock_io(&cxlm->mbox.mutex); + rc = -EBUSY; /* * XXX: There is some amount of ambiguity in the 2.0 version of the spec @@ -443,6 +459,155 @@ static int cxl_mem_count_commands(void) } return n; +}; + +/** + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. + * @cxlmd: The CXL memory device to communicate with. + * @cmd: The validated command. + * @in_payload: Pointer to userspace's input payload. + * @out_payload: Pointer to userspace's output payload. + * @u: The command submitted by userspace. Has output fields. + * + * Return: + * * %0 - Mailbox transaction succeeded. + * * %-EFAULT - Something happened with copy_to/from_user. + * * %-EINTR - Mailbox acquisition interrupted. + * * %-E2BIG - Output payload would overrun buffer. + * + * Creates the appropriate mailbox command on behalf of a userspace request. + * Return value, size, and output payload are all copied out to @u. The + * parameters for the command must be validated before calling this function. + * + * A 0 return code indicates the command executed successfully, not that it was + * itself successful. IOW, the retval should always be checked if wanting to + * determine the actual result. + */ +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd, + const struct cxl_mem_command *cmd, + u64 in_payload, + u64 out_payload, + struct cxl_send_command __user *u) +{ + struct mbox_cmd mbox_cmd = { + .opcode = cmd->opcode, + .size_in = cmd->info.size_in, + .payload = NULL, /* Copied by copy_to|from_user() */ + }; + int rc; + + if (cmd->info.size_in) { + /* + * Directly copy the userspace payload into the hardware. UAPI + * states that the buffer must already be little endian. + */ + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm), + u64_to_user_ptr(in_payload), + cmd->info.size_in)) { + cxl_mem_mbox_put(cxlmd->cxlm); + return -EFAULT; + } + } + + rc = cxl_mem_mbox_get(cxlmd->cxlm, true); + if (rc) + return rc; + + dev_dbg(&cxlmd->dev, + "Submitting %s command for user\n" + "\topcode: %x\n" + "\tsize: %ub\n", + command_names[cmd->info.id].name, mbox_cmd.opcode, + cmd->info.size_in); + + rc = cxl_mem_mbox_send_cmd(cxlmd->cxlm, &mbox_cmd); + cxl_mem_mbox_put(cxlmd->cxlm); + if (rc) + return rc; + + if (mbox_cmd.size_out > cmd->info.size_out) + return -E2BIG; + + rc = put_user(mbox_cmd.return_code, &u->retval); + if (rc) + return rc; + + rc = put_user(mbox_cmd.size_out, &u->size_out); + if (rc) + return rc; + + if (mbox_cmd.size_out) + if (copy_to_user(u64_to_user_ptr(out_payload), + (__force void *)cxl_payload_regs(cxlmd->cxlm), + mbox_cmd.size_out)) + return -EFAULT; + + return 0; +} + +/** + * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND. + * @cxlm: &struct cxl_mem device whose mailbox will be used. + * @send_cmd: &struct cxl_send_command copied in from userspace. + * @out_cmd: Sanitized and populated &struct cxl_mem_command. + * + * Return: + * * %0 - @out_cmd is ready to send. + * * %-ENOTTY - Invalid command specified. + * * %-EINVAL - Reserved fields or invalid values were used. + * * %-EPERM - Attempted to use a protected command. + * * %-ENOMEM - Input or output buffer wasn't sized properly. + * + * The result of this command is a fully validated command in @out_cmd that is + * safe to send to the hardware. + * + * See handle_mailbox_cmd_from_user() + */ +static int cxl_validate_cmd_from_user(struct cxl_mem *cxlm, + const struct cxl_send_command *send_cmd, + struct cxl_mem_command *out_cmd) +{ + const struct cxl_command_info *info; + struct cxl_mem_command *c; + + if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX) + return -ENOTTY; + + /* + * The user can never specify an input payload larger than our hardware + * supports, but output can be arbitrarily large, we simply write out as + * much data as the hardware provides. + */ + if (send_cmd->size_in > cxlm->mbox.payload_size) + return -EINVAL; + + if (send_cmd->flags & CXL_MEM_COMMAND_FLAG_MASK) + return -EINVAL; + + if (send_cmd->rsvd) + return -EINVAL; + + /* Convert user's command into the internal representation */ + c = &mem_commands[send_cmd->id]; + info = &c->info; + + if (info->flags & CXL_MEM_COMMAND_FLAG_KERNEL) + return -EPERM; + + /* Check the input buffer is the expected size */ + if (info->size_in >= 0 && info->size_in != send_cmd->size_in) + return -ENOMEM; + + /* Check the output buffer is at least large enough */ + if (info->size_out >= 0 && send_cmd->size_out < info->size_out) + return -ENOMEM; + + /* Setting a few const fields here... */ + memcpy(out_cmd, c, sizeof(*c)); + *(s32 *)&out_cmd->info.size_in = send_cmd->size_in; + *(s32 *)&out_cmd->info.size_out = send_cmd->size_out; + + return 0; } static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg) @@ -479,6 +644,37 @@ static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg if (j == n_commands) break; } + + return 0; + } else if (cmd == CXL_MEM_SEND_COMMAND) { + struct cxl_send_command send, __user *u = (void __user *)arg; + struct cxl_mem_command c; + int rc; + + dev_dbg(dev, "Send IOCTL\n"); + + if (copy_from_user(&send, u, sizeof(send))) + return -EFAULT; + + rc = device_lock_interruptible(dev); + if (rc) + return rc; + + if (!get_live_device(dev)) { + device_unlock(dev); + return -ENXIO; + } + + rc = cxl_validate_cmd_from_user(cxlmd->cxlm, &send, &c); + if (!rc) + rc = handle_mailbox_cmd_from_user(cxlmd, &c, + send.in_payload, + send.out_payload, u); + + put_device(dev); + device_unlock(dev); + + return rc; } return -ENOTTY; @@ -837,7 +1033,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm) int rc; /* Retrieve initial device memory map */ - rc = cxl_mem_mbox_get(cxlm); + rc = cxl_mem_mbox_get(cxlm, false); if (rc) return rc; diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h index 847f825bbe18..cb4e2bee5228 100644 --- a/include/uapi/linux/cxl_mem.h +++ b/include/uapi/linux/cxl_mem.h @@ -26,6 +26,7 @@ extern "C" { */ #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands) +#define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command) #undef CMDS #define CMDS \ @@ -69,6 +70,7 @@ struct cxl_command_info { #define CXL_MEM_COMMAND_FLAG_NONE 0 #define CXL_MEM_COMMAND_FLAG_KERNEL BIT(0) #define CXL_MEM_COMMAND_FLAG_MUTEX BIT(1) +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(31, 2) __s32 size_in; __s32 size_out; @@ -110,6 +112,43 @@ struct cxl_mem_query_commands { struct cxl_command_info __user commands[]; /* out: supported commands */ }; +/** + * struct cxl_send_command - Send a command to a memory device. + * @id: The command to send to the memory device. This must be one of the + * commands returned by the query command. + * @flags: Flags for the command (input). + * @rsvd: Must be zero. + * @retval: Return value from the memory device (output). + * @size_in: Size of the payload to provide to the device (input). + * @size_out: Size of the payload received from the device (input/output). This + * field is filled in by userspace to let the driver know how much + * space was allocated for output. It is populated by the driver to + * let userspace know how large the output payload actually was. + * @in_payload: Pointer to memory for payload input (little endian order). + * @out_payload: Pointer to memory for payload output (little endian order). + * + * Mechanism for userspace to send a command to the hardware for processing. The + * driver will do basic validation on the command sizes, but the payload input + * and output are not introspected. Userspace is required to allocate large + * enough buffers for max(size_in, size_out). + */ +struct cxl_send_command { + __u32 id; + __u32 flags; + __u32 rsvd; + __u32 retval; + + struct { + __s32 size_in; + __u64 in_payload; + }; + + struct { + __s32 size_out; + __u64 out_payload; + }; +}; + #if defined(__cplusplus) } #endif
The send command allows userspace to issue mailbox commands directly to the hardware. The driver will verify basic properties of the command and possible inspect the input (or output) payload to determine whether or not the command is allowed (or might taint the kernel). The list of allowed commands and their properties can be determined by using the QUERY IOCTL for CXL memory devices. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++- include/uapi/linux/cxl_mem.h | 39 +++++++ 2 files changed, 239 insertions(+), 4 deletions(-)