| Message ID | 20201209002418.1976362-12-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL 2.0 Support | expand |
On Tue, Dec 8, 2020 at 4:24 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > The CXL memory device send interface will have a number of supported > commands. The raw command is not such a command. Raw commands allow > userspace to send a specified opcode to the underlying hardware and > bypass all driver checks on the command. This is useful for a couple of > usecases, mainly: > 1. Undocumented vendor specific hardware commands > 2. Prototyping new hardware commands not yet supported by the driver > > While this all sounds very powerful it comes with a couple of caveats: > 1. Bug reports using raw commands will not get the same level of > attention as bug reports using supported commands (via taint). > 2. Supported commands will be rejected by the RAW command. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++++ > include/uapi/linux/cxl_mem.h | 14 ++++++++++++-- > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 0bf03afc0c80..a2cea7ac7cc6 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -115,6 +115,7 @@ struct cxl_mem_command { > > static struct cxl_mem_command mem_commands[] = { > CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0), > + CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0), Why is the taint indication in the ABI? It seems like it only needs to be documented. > }; > > static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm) > @@ -326,6 +327,20 @@ static int cxl_mem_count_commands(void) > return n; > }; > > +static struct cxl_mem_command *cxl_mem_find_command(u16 opcode) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(mem_commands); i++) { > + struct cxl_mem_command *c = &mem_commands[i]; > + > + if (c->opcode == opcode) > + return c; > + } > + > + return NULL; > +}; > + > /** > * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > * @cxlmd: The CXL memory device to communicate with. > @@ -421,6 +436,23 @@ static int cxl_validate_cmd_from_user(struct cxl_send_command __user *user_cmd, > c = &mem_commands[cmd.id]; > info = &c->info; > > + /* Checks are bypassed for raw commands but along comes the taint! */ > + if (cmd.id == CXL_MEM_COMMAND_ID_RAW) { > + struct cxl_mem_command temp = > + CXL_CMD(RAW, NONE, cmd.size_in, cmd.size_out, "Raw", > + true, cmd.raw.opcode); Oh, I thought CXL_CMD() was only used to populate the mem_commands array. Feels out of place to use it here when all it is doing is updating the size_{in,out} and opcode fields. Mainly I'm interested in CXL_CMD() enforcing that the command-id is the mem_commands index. > + > + if (cmd.raw.rsvd) > + return -EINVAL; > + > + if (cxl_mem_find_command(cmd.raw.opcode)) > + return -EPERM; > + > + add_taint(TAINT_WARN, LOCKDEP_STILL_OK); TAINT_WARN seems the wrong value, especially since no WARN has occurred. I feel that this is more in the spirit of TAINT_PROPRIETARY_MODULE, TAINT_OVERRIDDEN_ACPI_TABLE, and TAINT_OOT_MODULE. How about a new TAINT_RAW_PASSTHROUGH? I could use this for the acpi/nfit driver as well to disclaim responsibility for system errors that can result from not using the nominal kernel-provided commands.
On 20-12-09 14:38:49, Dan Williams wrote: > On Tue, Dec 8, 2020 at 4:24 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > The CXL memory device send interface will have a number of supported > > commands. The raw command is not such a command. Raw commands allow > > userspace to send a specified opcode to the underlying hardware and > > bypass all driver checks on the command. This is useful for a couple of > > usecases, mainly: > > 1. Undocumented vendor specific hardware commands > > 2. Prototyping new hardware commands not yet supported by the driver > > > > While this all sounds very powerful it comes with a couple of caveats: > > 1. Bug reports using raw commands will not get the same level of > > attention as bug reports using supported commands (via taint). > > 2. Supported commands will be rejected by the RAW command. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++++ > > include/uapi/linux/cxl_mem.h | 14 ++++++++++++-- > > 2 files changed, 44 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index 0bf03afc0c80..a2cea7ac7cc6 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -115,6 +115,7 @@ struct cxl_mem_command { > > > > static struct cxl_mem_command mem_commands[] = { > > CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0), > > + CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0), > > Why is the taint indication in the ABI? It seems like it only needs to > be documented. > It's removed per the previous patch discussion. > > }; > > > > static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm) > > @@ -326,6 +327,20 @@ static int cxl_mem_count_commands(void) > > return n; > > }; > > > > +static struct cxl_mem_command *cxl_mem_find_command(u16 opcode) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(mem_commands); i++) { > > + struct cxl_mem_command *c = &mem_commands[i]; > > + > > + if (c->opcode == opcode) > > + return c; > > + } > > + > > + return NULL; > > +}; > > + > > /** > > * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. > > * @cxlmd: The CXL memory device to communicate with. > > @@ -421,6 +436,23 @@ static int cxl_validate_cmd_from_user(struct cxl_send_command __user *user_cmd, > > c = &mem_commands[cmd.id]; > > info = &c->info; > > > > + /* Checks are bypassed for raw commands but along comes the taint! */ > > + if (cmd.id == CXL_MEM_COMMAND_ID_RAW) { > > + struct cxl_mem_command temp = > > + CXL_CMD(RAW, NONE, cmd.size_in, cmd.size_out, "Raw", > > + true, cmd.raw.opcode); > > Oh, I thought CXL_CMD() was only used to populate the mem_commands > array. Feels out of place to use it here when all it is doing is > updating the size_{in,out} and opcode fields. Mainly I'm interested in > CXL_CMD() enforcing that the command-id is the mem_commands index. > Agreed and removed. > > + > > + if (cmd.raw.rsvd) > > + return -EINVAL; > > + > > + if (cxl_mem_find_command(cmd.raw.opcode)) > > + return -EPERM; > > + > > + add_taint(TAINT_WARN, LOCKDEP_STILL_OK); > > TAINT_WARN seems the wrong value, especially since no WARN has > occurred. I feel that this is more in the spirit of > TAINT_PROPRIETARY_MODULE, TAINT_OVERRIDDEN_ACPI_TABLE, and > TAINT_OOT_MODULE. How about a new TAINT_RAW_PASSTHROUGH? I could use > this for the acpi/nfit driver as well to disclaim responsibility for > system errors that can result from not using the nominal > kernel-provided commands. I like it.
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 0bf03afc0c80..a2cea7ac7cc6 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -115,6 +115,7 @@ struct cxl_mem_command { static struct cxl_mem_command mem_commands[] = { CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0), + CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0), }; static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm) @@ -326,6 +327,20 @@ static int cxl_mem_count_commands(void) return n; }; +static struct cxl_mem_command *cxl_mem_find_command(u16 opcode) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(mem_commands); i++) { + struct cxl_mem_command *c = &mem_commands[i]; + + if (c->opcode == opcode) + return c; + } + + return NULL; +}; + /** * handle_mailbox_cmd_from_user() - Dispatch a mailbox command. * @cxlmd: The CXL memory device to communicate with. @@ -421,6 +436,23 @@ static int cxl_validate_cmd_from_user(struct cxl_send_command __user *user_cmd, c = &mem_commands[cmd.id]; info = &c->info; + /* Checks are bypassed for raw commands but along comes the taint! */ + if (cmd.id == CXL_MEM_COMMAND_ID_RAW) { + struct cxl_mem_command temp = + CXL_CMD(RAW, NONE, cmd.size_in, cmd.size_out, "Raw", + true, cmd.raw.opcode); + + if (cmd.raw.rsvd) + return -EINVAL; + + if (cxl_mem_find_command(cmd.raw.opcode)) + return -EPERM; + + add_taint(TAINT_WARN, LOCKDEP_STILL_OK); + memcpy(out_cmd, &temp, sizeof(temp)); + return 0; + } + if (cmd.flags & CXL_MEM_COMMAND_FLAG_MASK) return -EINVAL; diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h index 189d86a13637..f2fbb0dcda06 100644 --- a/include/uapi/linux/cxl_mem.h +++ b/include/uapi/linux/cxl_mem.h @@ -49,7 +49,8 @@ extern "C" { struct cxl_command_info { __u32 id; #define CXL_MEM_COMMAND_ID_INVALID 0 -#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_INVALID + 1) +#define CXL_MEM_COMMAND_ID_RAW 1 +#define CXL_MEM_COMMAND_ID_MAX (CXL_MEM_COMMAND_ID_RAW + 1) __u32 flags; #define CXL_MEM_COMMAND_FLAG_NONE 0 @@ -103,6 +104,9 @@ struct cxl_mem_query_commands { * @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 + * @raw: Special fields for raw commands + * @raw.opcode: Opcode passed to hardware when using the RAW command. + * @raw.rsvd: Reserved for future use. * @rsvd: Reserved for future use. * @retval: Return value from the memory device (output). * @size_in: Size of the payload to provide to the device (input). @@ -120,7 +124,13 @@ struct cxl_mem_query_commands { struct cxl_send_command { __u32 id; __u32 flags; - __u32 rsvd; + union { + struct { + __u16 opcode; + __u16 rsvd; + } raw; + __u32 rsvd; + }; __u32 retval; struct {
The CXL memory device send interface will have a number of supported commands. The raw command is not such a command. Raw commands allow userspace to send a specified opcode to the underlying hardware and bypass all driver checks on the command. This is useful for a couple of usecases, mainly: 1. Undocumented vendor specific hardware commands 2. Prototyping new hardware commands not yet supported by the driver While this all sounds very powerful it comes with a couple of caveats: 1. Bug reports using raw commands will not get the same level of attention as bug reports using supported commands (via taint). 2. Supported commands will be rejected by the RAW command. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++++ include/uapi/linux/cxl_mem.h | 14 ++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)