| Message ID | 20221222-cxl-misc-v2-3-60403cc37257@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL: Miscellaneous fixes | expand |
Ira Weiny wrote: > It was pointed out that commands not supported by the device or excluded > by the kernel were being returned in cxl_query_cmd().[1] > > While libcxl correctly handles failing commands, it is more efficient to > not issue an invalid command in the first place. > > Exclude both kernel exclusive and disabled commands from the list of > commands returned by cxl_query_cmd(). > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for v2: > New patch > --- > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index b03fba212799..a1618b7f01e5 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > return 0; > } > > +/* Return 0 if the cmd id is available for userspace */ > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > +{ > + /* Check that the command is enabled for hardware */ > + if (!test_bit(id, cxlds->enabled_cmds)) > + return -ENOTTY; > + > + /* Check that the command is not claimed for exclusive kernel use */ > + if (test_bit(id, cxlds->exclusive_cmds)) > + return -EBUSY; > + > + return 0; > +} > + > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > const struct cxl_send_command *send_cmd, > struct cxl_dev_state *cxlds) > { > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > const struct cxl_command_info *info = &c->info; > + int rc; > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > return -EINVAL; > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > return -EINVAL; > > - /* Check that the command is enabled for hardware */ > - if (!test_bit(info->id, cxlds->enabled_cmds)) > - return -ENOTTY; > - > - /* Check that the command is not claimed for exclusive kernel use */ > - if (test_bit(info->id, cxlds->exclusive_cmds)) > - return -EBUSY; > + rc = cxl_cmd_id_user(info->id, cxlds); > + if (rc) > + return rc; > > /* Check the input buffer is the expected size */ > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > */ > cxl_for_each_cmd(cmd) { > const struct cxl_command_info *info = &cmd->info; > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + int rc; > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > - return -EFAULT; > + rc = cxl_cmd_id_user(info->id, cxlds); > + if (!rc) { > + if (copy_to_user(&q->commands[j++], info, > + sizeof(*info))) > + return -EFAULT; > + } I like where this is going, but I think it is still useful to return all the commands even if they are not enabled or currently exclusive, especially since that expectation has already shipped. I also think it is a bug that this command passes kernel internal flags like CXL_CMD_FLAG_FORCE_ENABLE. So let's declare new flags in include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h index c71021a2a9ed..1ba0e12fd410 100644 --- a/include/uapi/linux/cxl_mem.h +++ b/include/uapi/linux/cxl_mem.h @@ -87,8 +87,10 @@ struct cxl_command_info { __u32 id; __u32 flags; -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) - +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) __u32 size_in; __u32 size_out; }; diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 6ed8e3654939..24469668f1af 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -427,6 +427,7 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd, int cxl_query_cmd(struct cxl_memdev *cxlmd, struct cxl_mem_query_commands __user *q) { + struct cxl_dev_state *cxlds = cxlmd->cxlds; struct device *dev = &cxlmd->dev; struct cxl_mem_command *cmd; u32 n_commands; @@ -446,9 +447,18 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, * structures. */ cxl_for_each_cmd(cmd) { - const struct cxl_command_info *info = &cmd->info; + struct cxl_command_info info = cmd->info; + + /* wipe kernel internal flags */ + info.flags = 0; - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) + /* reflect exclusive and enabled */ + if (test_bit(info.id, cxlds->enabled_cmds)) + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; + if (test_bit(info.id, cxlds->exclusive_cmds)) + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; + + if (copy_to_user(&q->commands[j++], &info, sizeof(info))) return -EFAULT; if (j == n_commands)
On Fri, 27 Jan 2023 18:06:43 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Ira Weiny wrote: > > It was pointed out that commands not supported by the device or excluded > > by the kernel were being returned in cxl_query_cmd().[1] > > > > While libcxl correctly handles failing commands, it is more efficient to > > not issue an invalid command in the first place. > > > > Exclude both kernel exclusive and disabled commands from the list of > > commands returned by cxl_query_cmd(). > > > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > --- > > Changes for v2: > > New patch > > --- > > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index b03fba212799..a1618b7f01e5 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > > return 0; > > } > > > > +/* Return 0 if the cmd id is available for userspace */ > > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > > +{ > > + /* Check that the command is enabled for hardware */ > > + if (!test_bit(id, cxlds->enabled_cmds)) > > + return -ENOTTY; > > + > > + /* Check that the command is not claimed for exclusive kernel use */ > > + if (test_bit(id, cxlds->exclusive_cmds)) > > + return -EBUSY; > > + > > + return 0; > > +} > > + > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > const struct cxl_send_command *send_cmd, > > struct cxl_dev_state *cxlds) > > { > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > const struct cxl_command_info *info = &c->info; > > + int rc; > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > return -EINVAL; > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > return -EINVAL; > > > > - /* Check that the command is enabled for hardware */ > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > - return -ENOTTY; > > - > > - /* Check that the command is not claimed for exclusive kernel use */ > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > - return -EBUSY; > > + rc = cxl_cmd_id_user(info->id, cxlds); > > + if (rc) > > + return rc; > > > > /* Check the input buffer is the expected size */ > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > */ > > cxl_for_each_cmd(cmd) { > > const struct cxl_command_info *info = &cmd->info; > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > + int rc; > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > - return -EFAULT; > > + rc = cxl_cmd_id_user(info->id, cxlds); > > + if (!rc) { > > + if (copy_to_user(&q->commands[j++], info, > > + sizeof(*info))) > > + return -EFAULT; > > + } > > I like where this is going, but I think it is still useful to return all > the commands even if they are not enabled or currently exclusive, > especially since that expectation has already shipped. > > I also think it is a bug that this command passes kernel internal flags > like CXL_CMD_FLAG_FORCE_ENABLE. So let's declare new flags in > include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: > So this is OK in that we are just adding more info in the flags field so backwards compatibility is better maintained than simply hiding commands. > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > index c71021a2a9ed..1ba0e12fd410 100644 > --- a/include/uapi/linux/cxl_mem.h > +++ b/include/uapi/linux/cxl_mem.h > @@ -87,8 +87,10 @@ struct cxl_command_info { > __u32 id; > > __u32 flags; > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) > - > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) > +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) Good to add a comment on this flag. I've been staring at it an can't figure out what it was ever for... > +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) ENABLED isn't the clearest naming... Perhaps _AVAILABLE_TO_USER or something like that would be easier? > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) What does EXCLUSIVE mean to a userspace caller? EXCLUSIVE to whom? Other than bikeshedding this looks good to me. Thanks, Jonathan > __u32 size_in; > __u32 size_out; > }; > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 6ed8e3654939..24469668f1af 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -427,6 +427,7 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd, > int cxl_query_cmd(struct cxl_memdev *cxlmd, > struct cxl_mem_query_commands __user *q) > { > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct device *dev = &cxlmd->dev; > struct cxl_mem_command *cmd; > u32 n_commands; > @@ -446,9 +447,18 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > * structures. > */ > cxl_for_each_cmd(cmd) { > - const struct cxl_command_info *info = &cmd->info; > + struct cxl_command_info info = cmd->info; > + > + /* wipe kernel internal flags */ > + info.flags = 0; > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > + /* reflect exclusive and enabled */ > + if (test_bit(info.id, cxlds->enabled_cmds)) > + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; > + if (test_bit(info.id, cxlds->exclusive_cmds)) > + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; > + > + if (copy_to_user(&q->commands[j++], &info, sizeof(info))) > return -EFAULT; > > if (j == n_commands)
Jonathan Cameron wrote: > On Fri, 27 Jan 2023 18:06:43 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Ira Weiny wrote: > > > It was pointed out that commands not supported by the device or excluded > > > by the kernel were being returned in cxl_query_cmd().[1] > > > > > > While libcxl correctly handles failing commands, it is more efficient to > > > not issue an invalid command in the first place. > > > > > > Exclude both kernel exclusive and disabled commands from the list of > > > commands returned by cxl_query_cmd(). > > > > > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > --- > > > Changes for v2: > > > New patch > > > --- > > > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > > index b03fba212799..a1618b7f01e5 100644 > > > --- a/drivers/cxl/core/mbox.c > > > +++ b/drivers/cxl/core/mbox.c > > > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > > > return 0; > > > } > > > > > > +/* Return 0 if the cmd id is available for userspace */ > > > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > > > +{ > > > + /* Check that the command is enabled for hardware */ > > > + if (!test_bit(id, cxlds->enabled_cmds)) > > > + return -ENOTTY; > > > + > > > + /* Check that the command is not claimed for exclusive kernel use */ > > > + if (test_bit(id, cxlds->exclusive_cmds)) > > > + return -EBUSY; > > > + > > > + return 0; > > > +} > > > + > > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > const struct cxl_send_command *send_cmd, > > > struct cxl_dev_state *cxlds) > > > { > > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > > const struct cxl_command_info *info = &c->info; > > > + int rc; > > > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > > return -EINVAL; > > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > > return -EINVAL; > > > > > > - /* Check that the command is enabled for hardware */ > > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > > - return -ENOTTY; > > > - > > > - /* Check that the command is not claimed for exclusive kernel use */ > > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > > - return -EBUSY; > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > + if (rc) > > > + return rc; > > > > > > /* Check the input buffer is the expected size */ > > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > > */ > > > cxl_for_each_cmd(cmd) { > > > const struct cxl_command_info *info = &cmd->info; > > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > + int rc; > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > - return -EFAULT; > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > + if (!rc) { > > > + if (copy_to_user(&q->commands[j++], info, > > > + sizeof(*info))) > > > + return -EFAULT; > > > + } > > > > I like where this is going, but I think it is still useful to return all > > the commands even if they are not enabled or currently exclusive, > > especially since that expectation has already shipped. > > > > I also think it is a bug that this command passes kernel internal flags > > like CXL_CMD_FLAG_FORCE_ENABLE. So let's declare new flags in > > include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: > > > > So this is OK in that we are just adding more info in the flags > field so backwards compatibility is better maintained than simply hiding > commands. > > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > > index c71021a2a9ed..1ba0e12fd410 100644 > > --- a/include/uapi/linux/cxl_mem.h > > +++ b/include/uapi/linux/cxl_mem.h > > @@ -87,8 +87,10 @@ struct cxl_command_info { > > __u32 id; > > > > __u32 flags; > > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) > > - > > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) > > +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) > > Good to add a comment on this flag. I've been staring at it an can't > figure out what it was ever for... Oh, I just noticed that the kernel internal CXL_CMD_FLAG_FORCE_ENABLE was being blindly copied out to userspace where it has no meaning. So any new flags need to come after that one that is already burned. > > +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) > > ENABLED isn't the clearest naming... Perhaps _AVAILABLE_TO_USER > or something like that would be easier? Yes... but it is unavailable to the user when it is temporarily reserved by the kernel in the next flag indication. > > > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) > What does EXCLUSIVE mean to a userspace caller? EXCLUSIVE to whom? > > Other than bikeshedding this looks good to me. How about USER_ENABLED and KERNEL_RESERVED? With some docs: /* * Older kernels leaked an internal flag at this bit position, that flag * is not relevant to userspace. Newer kernels set it to zero. */ #define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) /* * The given command id is supported by the driver and is supported by a * related opcode on the device. */ #define CXL_MEM_COMMAND_FLAG_USER_ENABLED BIT(1) /* * Requests with the given command id will terminate with EBUSY as the * kernel actively owns management of the given resource. For example, * the label-storage-area can not be written while the kernel is * actively managing that space. */ #define CXL_MEM_COMMAND_FLAG_KERNEL_RESERVED BIT(2)
Dan Williams wrote: > Ira Weiny wrote: > > It was pointed out that commands not supported by the device or excluded > > by the kernel were being returned in cxl_query_cmd().[1] > > > > While libcxl correctly handles failing commands, it is more efficient to > > not issue an invalid command in the first place. > > > > Exclude both kernel exclusive and disabled commands from the list of > > commands returned by cxl_query_cmd(). > > > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > --- > > Changes for v2: > > New patch > > --- > > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index b03fba212799..a1618b7f01e5 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > > return 0; > > } > > > > +/* Return 0 if the cmd id is available for userspace */ > > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > > +{ > > + /* Check that the command is enabled for hardware */ > > + if (!test_bit(id, cxlds->enabled_cmds)) > > + return -ENOTTY; > > + > > + /* Check that the command is not claimed for exclusive kernel use */ > > + if (test_bit(id, cxlds->exclusive_cmds)) > > + return -EBUSY; > > + > > + return 0; > > +} > > + > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > const struct cxl_send_command *send_cmd, > > struct cxl_dev_state *cxlds) > > { > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > const struct cxl_command_info *info = &c->info; > > + int rc; > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > return -EINVAL; > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > return -EINVAL; > > > > - /* Check that the command is enabled for hardware */ > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > - return -ENOTTY; > > - > > - /* Check that the command is not claimed for exclusive kernel use */ > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > - return -EBUSY; > > + rc = cxl_cmd_id_user(info->id, cxlds); > > + if (rc) > > + return rc; > > > > /* Check the input buffer is the expected size */ > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > */ > > cxl_for_each_cmd(cmd) { > > const struct cxl_command_info *info = &cmd->info; > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > + int rc; > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > - return -EFAULT; > > + rc = cxl_cmd_id_user(info->id, cxlds); > > + if (!rc) { > > + if (copy_to_user(&q->commands[j++], info, > > + sizeof(*info))) > > + return -EFAULT; > > + } > > I like where this is going, but I think it is still useful to return all > the commands even if they are not enabled or currently exclusive, > especially since that expectation has already shipped. I recognize that there may be an expectation of functionality but userspace can't _do_ anything with the disabled/exclusive commands. So why not actually fix cxl_query_cmd()? Effectively keeping the current behavior will neither fix nor break existing user space. But more importantly the patch I've proposed makes existing user space more efficient without breaking it either. This is because returning these command only allows libcxl to issue those commands which then fail. The way I see it, there is no value in returning those commands at all. Unless we want to have user space provide additional debugging for the reason for the commands failing. > > I also think it is a bug that this command passes kernel internal flags > like CXL_CMD_FLAG_FORCE_ENABLE. AFAICS It does not return that flag. CXL_CMD_FLAG_FORCE_ENABLE is a flag in struct cxl_mem_command while struct cxl_command_info has separate flags. AFAICS only struct cxl_command_info is exposed to userspace. > So let's declare new flags in > include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > index c71021a2a9ed..1ba0e12fd410 100644 > --- a/include/uapi/linux/cxl_mem.h > +++ b/include/uapi/linux/cxl_mem.h > @@ -87,8 +87,10 @@ struct cxl_command_info { > __u32 id; > > __u32 flags; > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) > - > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) > +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) > +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) > __u32 size_in; > __u32 size_out; > }; > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 6ed8e3654939..24469668f1af 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -427,6 +427,7 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd, > int cxl_query_cmd(struct cxl_memdev *cxlmd, > struct cxl_mem_query_commands __user *q) > { > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct device *dev = &cxlmd->dev; > struct cxl_mem_command *cmd; > u32 n_commands; > @@ -446,9 +447,18 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > * structures. > */ > cxl_for_each_cmd(cmd) { > - const struct cxl_command_info *info = &cmd->info; > + struct cxl_command_info info = cmd->info; > + > + /* wipe kernel internal flags */ > + info.flags = 0; Unless I'm missing something this is not needed. > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > + /* reflect exclusive and enabled */ > + if (test_bit(info.id, cxlds->enabled_cmds)) > + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; > + if (test_bit(info.id, cxlds->exclusive_cmds)) > + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; Ok Just to make sure I'm following you. This is then expecting user space to check these flags to know if this command is available or not? This means additional user space changes to use these. Which is ok. But what is the value? Current user space will still be broken and future user space will be more complicated. Right now libcxl issues a query before each command to ensure the command is available. These flags can't be cached. The patch as I have proposed simply short circuits the already failing path in libcxl with minimal changes. Do we have a use case for user space to report the current disabled and exclusive commands? Ira
Ira Weiny wrote: > Dan Williams wrote: > > Ira Weiny wrote: > > > It was pointed out that commands not supported by the device or excluded > > > by the kernel were being returned in cxl_query_cmd().[1] > > > > > > While libcxl correctly handles failing commands, it is more efficient to > > > not issue an invalid command in the first place. > > > > > > Exclude both kernel exclusive and disabled commands from the list of > > > commands returned by cxl_query_cmd(). > > > > > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > --- > > > Changes for v2: > > > New patch > > > --- > > > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > > index b03fba212799..a1618b7f01e5 100644 > > > --- a/drivers/cxl/core/mbox.c > > > +++ b/drivers/cxl/core/mbox.c > > > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > > > return 0; > > > } > > > > > > +/* Return 0 if the cmd id is available for userspace */ > > > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > > > +{ > > > + /* Check that the command is enabled for hardware */ > > > + if (!test_bit(id, cxlds->enabled_cmds)) > > > + return -ENOTTY; > > > + > > > + /* Check that the command is not claimed for exclusive kernel use */ > > > + if (test_bit(id, cxlds->exclusive_cmds)) > > > + return -EBUSY; > > > + > > > + return 0; > > > +} > > > + > > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > const struct cxl_send_command *send_cmd, > > > struct cxl_dev_state *cxlds) > > > { > > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > > const struct cxl_command_info *info = &c->info; > > > + int rc; > > > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > > return -EINVAL; > > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > > return -EINVAL; > > > > > > - /* Check that the command is enabled for hardware */ > > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > > - return -ENOTTY; > > > - > > > - /* Check that the command is not claimed for exclusive kernel use */ > > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > > - return -EBUSY; > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > + if (rc) > > > + return rc; > > > > > > /* Check the input buffer is the expected size */ > > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > > */ > > > cxl_for_each_cmd(cmd) { > > > const struct cxl_command_info *info = &cmd->info; > > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > + int rc; > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > - return -EFAULT; > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > + if (!rc) { > > > + if (copy_to_user(&q->commands[j++], info, > > > + sizeof(*info))) > > > + return -EFAULT; > > > + } > > > > I like where this is going, but I think it is still useful to return all > > the commands even if they are not enabled or currently exclusive, > > especially since that expectation has already shipped. > > I recognize that there may be an expectation of functionality but > userspace can't _do_ anything with the disabled/exclusive commands. So > why not actually fix cxl_query_cmd()? Effectively keeping the current > behavior will neither fix nor break existing user space. But more > importantly the patch I've proposed makes existing user space more > efficient without breaking it either. > > This is because returning these command only allows libcxl to issue those > commands which then fail. > > The way I see it, there is no value in returning those commands at all. > Unless we want to have user space provide additional debugging for the > reason for the commands failing. There is value, similar to the motivation to print the status of enabled commands. It is useful to know what functionality is limited by the kernel and what functionality is limited by the device. > > I also think it is a bug that this command passes kernel internal flags > > like CXL_CMD_FLAG_FORCE_ENABLE. > > AFAICS It does not return that flag. CXL_CMD_FLAG_FORCE_ENABLE is a flag > in struct cxl_mem_command while struct cxl_command_info has separate > flags. > > AFAICS only struct cxl_command_info is exposed to userspace. Ah true, CXL_CMD() leaves info.flags unset. > > > So let's declare new flags in > > include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: > > > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > > index c71021a2a9ed..1ba0e12fd410 100644 > > --- a/include/uapi/linux/cxl_mem.h > > +++ b/include/uapi/linux/cxl_mem.h > > @@ -87,8 +87,10 @@ struct cxl_command_info { > > __u32 id; > > > > __u32 flags; > > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) > > - > > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) > > +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) > > +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) > > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) > > __u32 size_in; > > __u32 size_out; > > }; > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 6ed8e3654939..24469668f1af 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -427,6 +427,7 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd, > > int cxl_query_cmd(struct cxl_memdev *cxlmd, > > struct cxl_mem_query_commands __user *q) > > { > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > struct device *dev = &cxlmd->dev; > > struct cxl_mem_command *cmd; > > u32 n_commands; > > @@ -446,9 +447,18 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > * structures. > > */ > > cxl_for_each_cmd(cmd) { > > - const struct cxl_command_info *info = &cmd->info; > > + struct cxl_command_info info = cmd->info; > > + > > + /* wipe kernel internal flags */ > > + info.flags = 0; > > Unless I'm missing something this is not needed. Yes. > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > + /* reflect exclusive and enabled */ > > + if (test_bit(info.id, cxlds->enabled_cmds)) > > + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; > > + if (test_bit(info.id, cxlds->exclusive_cmds)) > > + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; > > Ok Just to make sure I'm following you. This is then expecting user space > to check these flags to know if this command is available or not? No, there is no expectation that userspace check these, especially since they were not mandated before, but they are optional useful information for future debug. > This means additional user space changes to use these. Which is ok. But > what is the value? Current user space will still be broken and future > user space will be more complicated. Value is as above, more user visible debug information which will be important as different levels of enabling make it out to different backport kernels. > Right now libcxl issues a query before each command to ensure the command > is available. These flags can't be cached. > > The patch as I have proposed simply short circuits the already failing > path in libcxl with minimal changes. > > Do we have a use case for user space to report the current disabled and > exclusive commands? Nothing beyond future debug flexibility.
On Fri, Jan 27, 2023 at 04:09:40PM -0800, Ira Weiny wrote: > It was pointed out that commands not supported by the device or excluded > by the kernel were being returned in cxl_query_cmd().[1] > > While libcxl correctly handles failing commands, it is more efficient to > not issue an invalid command in the first place. > > Exclude both kernel exclusive and disabled commands from the list of > commands returned by cxl_query_cmd(). Hi Ira, I have read everyone's responses here, and I'm not sure where to append mine, so I'll spit it out right here. This is great to clean up as we (or at least me) keep stumbling on it. 'Excluding kernel exclusive and disable commands from the list' may not tell user what they want to know. It seems you are after these 3 bits of info: 1) does my device support this cmd 2) does the driver support this cmd 3) is user allowed to run this cmd (ie..or is it kernel reserved). Can we crisply impart this status, all of it, in query cmd? Note that I'm thinking you deliver the whole truth, not piecemeal. ie. Don't tell me that my device doesn't support it, then I go off and try another device that supports it, only to find out that the driver doesn't support it, and....rat hole deepens, then I see driver support added, only to find out it's exclusive to kernel. ;) I'm assuming kernel exclusive includes raw command not allowed. Thanks for getting this all tidy'd up Ira, Alison > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes for v2: > New patch > --- > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index b03fba212799..a1618b7f01e5 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > return 0; > } > > +/* Return 0 if the cmd id is available for userspace */ > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > +{ > + /* Check that the command is enabled for hardware */ > + if (!test_bit(id, cxlds->enabled_cmds)) > + return -ENOTTY; > + > + /* Check that the command is not claimed for exclusive kernel use */ > + if (test_bit(id, cxlds->exclusive_cmds)) > + return -EBUSY; > + > + return 0; > +} > + > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > const struct cxl_send_command *send_cmd, > struct cxl_dev_state *cxlds) > { > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > const struct cxl_command_info *info = &c->info; > + int rc; > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > return -EINVAL; > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > return -EINVAL; > > - /* Check that the command is enabled for hardware */ > - if (!test_bit(info->id, cxlds->enabled_cmds)) > - return -ENOTTY; > - > - /* Check that the command is not claimed for exclusive kernel use */ > - if (test_bit(info->id, cxlds->exclusive_cmds)) > - return -EBUSY; > + rc = cxl_cmd_id_user(info->id, cxlds); > + if (rc) > + return rc; > > /* Check the input buffer is the expected size */ > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > */ > cxl_for_each_cmd(cmd) { > const struct cxl_command_info *info = &cmd->info; > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + int rc; > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > - return -EFAULT; > + rc = cxl_cmd_id_user(info->id, cxlds); > + if (!rc) { > + if (copy_to_user(&q->commands[j++], info, > + sizeof(*info))) > + return -EFAULT; > + } > > if (j == n_commands) > break; > > -- > 2.39.1
On Mon, 30 Jan 2023 11:42:57 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > On Fri, 27 Jan 2023 18:06:43 -0800 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > Ira Weiny wrote: > > > > It was pointed out that commands not supported by the device or excluded > > > > by the kernel were being returned in cxl_query_cmd().[1] > > > > > > > > While libcxl correctly handles failing commands, it is more efficient to > > > > not issue an invalid command in the first place. > > > > > > > > Exclude both kernel exclusive and disabled commands from the list of > > > > commands returned by cxl_query_cmd(). > > > > > > > > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ > > > > > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > > > --- > > > > Changes for v2: > > > > New patch > > > > --- > > > > drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- > > > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > > > index b03fba212799..a1618b7f01e5 100644 > > > > --- a/drivers/cxl/core/mbox.c > > > > +++ b/drivers/cxl/core/mbox.c > > > > @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, > > > > return 0; > > > > } > > > > > > > > +/* Return 0 if the cmd id is available for userspace */ > > > > +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) > > > > +{ > > > > + /* Check that the command is enabled for hardware */ > > > > + if (!test_bit(id, cxlds->enabled_cmds)) > > > > + return -ENOTTY; > > > > + > > > > + /* Check that the command is not claimed for exclusive kernel use */ > > > > + if (test_bit(id, cxlds->exclusive_cmds)) > > > > + return -EBUSY; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > > const struct cxl_send_command *send_cmd, > > > > struct cxl_dev_state *cxlds) > > > > { > > > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > > > const struct cxl_command_info *info = &c->info; > > > > + int rc; > > > > > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > > > return -EINVAL; > > > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > > > return -EINVAL; > > > > > > > > - /* Check that the command is enabled for hardware */ > > > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > > > - return -ENOTTY; > > > > - > > > > - /* Check that the command is not claimed for exclusive kernel use */ > > > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > > > - return -EBUSY; > > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > > + if (rc) > > > > + return rc; > > > > > > > > /* Check the input buffer is the expected size */ > > > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > > > */ > > > > cxl_for_each_cmd(cmd) { > > > > const struct cxl_command_info *info = &cmd->info; > > > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > > + int rc; > > > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > > - return -EFAULT; > > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > > + if (!rc) { > > > > + if (copy_to_user(&q->commands[j++], info, > > > > + sizeof(*info))) > > > > + return -EFAULT; > > > > + } > > > > > > I like where this is going, but I think it is still useful to return all > > > the commands even if they are not enabled or currently exclusive, > > > especially since that expectation has already shipped. > > > > > > I also think it is a bug that this command passes kernel internal flags > > > like CXL_CMD_FLAG_FORCE_ENABLE. So let's declare new flags in > > > include/uapi/linux/cxl_mem.h starting at BIT(1) and do something like: > > > > > > > So this is OK in that we are just adding more info in the flags > > field so backwards compatibility is better maintained than simply hiding > > commands. > > > > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > > > index c71021a2a9ed..1ba0e12fd410 100644 > > > --- a/include/uapi/linux/cxl_mem.h > > > +++ b/include/uapi/linux/cxl_mem.h > > > @@ -87,8 +87,10 @@ struct cxl_command_info { > > > __u32 id; > > > > > > __u32 flags; > > > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0) > > > - > > > +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(2, 1) > > > +#define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) > > > > Good to add a comment on this flag. I've been staring at it an can't > > figure out what it was ever for... > > Oh, I just noticed that the kernel internal CXL_CMD_FLAG_FORCE_ENABLE > was being blindly copied out to userspace where it has no meaning. So > any new flags need to come after that one that is already burned. > > > > +#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(1) > > > > ENABLED isn't the clearest naming... Perhaps _AVAILABLE_TO_USER > > or something like that would be easier? > > Yes... but it is unavailable to the user when it is temporarily reserved > by the kernel in the next flag indication. > > > > > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(2) > > What does EXCLUSIVE mean to a userspace caller? EXCLUSIVE to whom? > > > > Other than bikeshedding this looks good to me. > > How about USER_ENABLED and KERNEL_RESERVED? With some docs: > > /* > * Older kernels leaked an internal flag at this bit position, that flag > * is not relevant to userspace. Newer kernels set it to zero. > */ > #define CXL_MEM_COMMAND_FLAG_RESERVED BIT(0) > > /* > * The given command id is supported by the driver and is supported by a > * related opcode on the device. > */ > #define CXL_MEM_COMMAND_FLAG_USER_ENABLED BIT(1) > > /* > * Requests with the given command id will terminate with EBUSY as the > * kernel actively owns management of the given resource. For example, > * the label-storage-area can not be written while the kernel is > * actively managing that space. > */ > #define CXL_MEM_COMMAND_FLAG_KERNEL_RESERVED BIT(2) LGTM
Dan Williams wrote: > Ira Weiny wrote: > > Dan Williams wrote: > > > Ira Weiny wrote: [snip] > > > > + > > > > static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > > const struct cxl_send_command *send_cmd, > > > > struct cxl_dev_state *cxlds) > > > > { > > > > struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; > > > > const struct cxl_command_info *info = &c->info; > > > > + int rc; > > > > > > > > if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > > > return -EINVAL; > > > > @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > > > > if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > > > return -EINVAL; > > > > > > > > - /* Check that the command is enabled for hardware */ > > > > - if (!test_bit(info->id, cxlds->enabled_cmds)) > > > > - return -ENOTTY; > > > > - > > > > - /* Check that the command is not claimed for exclusive kernel use */ > > > > - if (test_bit(info->id, cxlds->exclusive_cmds)) > > > > - return -EBUSY; > > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > > + if (rc) > > > > + return rc; > > > > > > > > /* Check the input buffer is the expected size */ > > > > if ((info->size_in != CXL_VARIABLE_PAYLOAD) && > > > > @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > > > > */ > > > > cxl_for_each_cmd(cmd) { > > > > const struct cxl_command_info *info = &cmd->info; > > > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > > > + int rc; > > > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > > - return -EFAULT; > > > > + rc = cxl_cmd_id_user(info->id, cxlds); > > > > + if (!rc) { > > > > + if (copy_to_user(&q->commands[j++], info, > > > > + sizeof(*info))) > > > > + return -EFAULT; > > > > + } > > > > > > I like where this is going, but I think it is still useful to return all > > > the commands even if they are not enabled or currently exclusive, > > > especially since that expectation has already shipped. > > > > I recognize that there may be an expectation of functionality but > > userspace can't _do_ anything with the disabled/exclusive commands. So > > why not actually fix cxl_query_cmd()? Effectively keeping the current > > behavior will neither fix nor break existing user space. But more > > importantly the patch I've proposed makes existing user space more > > efficient without breaking it either. > > > > This is because returning these command only allows libcxl to issue those > > commands which then fail. > > > > The way I see it, there is no value in returning those commands at all. > > Unless we want to have user space provide additional debugging for the > > reason for the commands failing. > > There is value, similar to the motivation to print the status of enabled > commands. It is useful to know what functionality is limited by the > kernel and what functionality is limited by the device. Ok. Do you think we should then update libcxl to use these flags? Or are you ok with the try and fail behavior now? [snip] > > > > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > + /* reflect exclusive and enabled */ > > > + if (test_bit(info.id, cxlds->enabled_cmds)) > > > + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; > > > + if (test_bit(info.id, cxlds->exclusive_cmds)) > > > + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; > > > > Ok Just to make sure I'm following you. This is then expecting user space > > to check these flags to know if this command is available or not? > > No, there is no expectation that userspace check these, especially since > they were not mandated before, but they are optional useful information > for future debug. Ok this is obviously easy to incorporate. > > > This means additional user space changes to use these. Which is ok. But > > what is the value? Current user space will still be broken and future > > user space will be more complicated. > > Value is as above, more user visible debug information which will be > important as different levels of enabling make it out to different > backport kernels. > > > Right now libcxl issues a query before each command to ensure the command > > is available. These flags can't be cached. > > > > The patch as I have proposed simply short circuits the already failing > > path in libcxl with minimal changes. > > > > Do we have a use case for user space to report the current disabled and > > exclusive commands? > > Nothing beyond future debug flexibility. I'll respin with this change. Let me know on any user space change you might want. Another option would be to add a libcxl 'info' functionality which reports the above flags? Not quite sure how that would work exactly but I could put some thought into that if you want. Ira
Ira Weiny wrote: > > There is value, similar to the motivation to print the status of enabled > > commands. It is useful to know what functionality is limited by the > > kernel and what functionality is limited by the device. > > Ok. Do you think we should then update libcxl to use these flags? Or are > you ok with the try and fail behavior now? The try and fail behavior needs to stay if only to maintain backward compatibility. > > [snip] > > > > > > > > > > > > > > - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) > > > > + /* reflect exclusive and enabled */ > > > > + if (test_bit(info.id, cxlds->enabled_cmds)) > > > > + info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED; > > > > + if (test_bit(info.id, cxlds->exclusive_cmds)) > > > > + info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE; > > > > > > Ok Just to make sure I'm following you. This is then expecting user space > > > to check these flags to know if this command is available or not? > > > > No, there is no expectation that userspace check these, especially since > > they were not mandated before, but they are optional useful information > > for future debug. > > Ok this is obviously easy to incorporate. > > > > > > This means additional user space changes to use these. Which is ok. But > > > what is the value? Current user space will still be broken and future > > > user space will be more complicated. > > > > Value is as above, more user visible debug information which will be > > important as different levels of enabling make it out to different > > backport kernels. > > > > > Right now libcxl issues a query before each command to ensure the command > > > is available. These flags can't be cached. > > > > > > The patch as I have proposed simply short circuits the already failing > > > path in libcxl with minimal changes. > > > > > > Do we have a use case for user space to report the current disabled and > > > exclusive commands? > > > > Nothing beyond future debug flexibility. > > I'll respin with this change. Let me know on any user space change you > might want. Another option would be to add a libcxl 'info' functionality > which reports the above flags? Not quite sure how that would work exactly > but I could put some thought into that if you want. I think it can wait until there is a bigger need to enumerate device capabilities. All the current commands have sysfs and other sideband ways to enumerate their presence, but going forward I can see this being useful. I.e. legacy commands are enumerated by try and fail, but new commands (post this enabling) can reliably depend on the flags to be there.
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index b03fba212799..a1618b7f01e5 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -326,12 +326,27 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd, return 0; } +/* Return 0 if the cmd id is available for userspace */ +static int cxl_cmd_id_user(__u32 id, struct cxl_dev_state *cxlds) +{ + /* Check that the command is enabled for hardware */ + if (!test_bit(id, cxlds->enabled_cmds)) + return -ENOTTY; + + /* Check that the command is not claimed for exclusive kernel use */ + if (test_bit(id, cxlds->exclusive_cmds)) + return -EBUSY; + + return 0; +} + static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, const struct cxl_send_command *send_cmd, struct cxl_dev_state *cxlds) { struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id]; const struct cxl_command_info *info = &c->info; + int rc; if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) return -EINVAL; @@ -342,13 +357,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, if (send_cmd->in.rsvd || send_cmd->out.rsvd) return -EINVAL; - /* Check that the command is enabled for hardware */ - if (!test_bit(info->id, cxlds->enabled_cmds)) - return -ENOTTY; - - /* Check that the command is not claimed for exclusive kernel use */ - if (test_bit(info->id, cxlds->exclusive_cmds)) - return -EBUSY; + rc = cxl_cmd_id_user(info->id, cxlds); + if (rc) + return rc; /* Check the input buffer is the expected size */ if ((info->size_in != CXL_VARIABLE_PAYLOAD) && @@ -446,9 +457,15 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, */ cxl_for_each_cmd(cmd) { const struct cxl_command_info *info = &cmd->info; + struct cxl_dev_state *cxlds = cxlmd->cxlds; + int rc; - if (copy_to_user(&q->commands[j++], info, sizeof(*info))) - return -EFAULT; + rc = cxl_cmd_id_user(info->id, cxlds); + if (!rc) { + if (copy_to_user(&q->commands[j++], info, + sizeof(*info))) + return -EFAULT; + } if (j == n_commands) break;
It was pointed out that commands not supported by the device or excluded by the kernel were being returned in cxl_query_cmd().[1] While libcxl correctly handles failing commands, it is more efficient to not issue an invalid command in the first place. Exclude both kernel exclusive and disabled commands from the list of commands returned by cxl_query_cmd(). [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/ Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Changes for v2: New patch --- drivers/cxl/core/mbox.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)