RE: [PATCH v2 3/4] cxl/uapi: Only return valid commands from cxl_query_cmd()

[Dan Williams <dan.j.williams@intel.com>]

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)