[PATCH v2 0/4] CXL: Miscellaneous fixes

[PATCH v2 0/4] CXL: Miscellaneous fixes

[Ira Weiny]

These are minor fixes I have seen along the way in the CXL code.

This includes fixing the query command to account for kernel exclusive and
enabled commands.

To: "Dan Williams" <dan.j.williams@intel.com>
Cc: "Jiang, Dave" <dave.jiang@intel.com>
Cc: "Alison Schofield" <alison.schofield@intel.com>
Cc: "Vishal Verma" <vishal.l.verma@intel.com>
Cc: "Ben Widawsky" <bwidawsk@kernel.org>
Cc: Robert Richter <rrichter@amd.com>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: linux-cxl@vger.kernel.org
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes in v2:
- Add checks for kernel exclusive and enabled commands to query before fixing
  the comment
- Link to v1: https://lore.kernel.org/r/20221222-cxl-misc-v1-0-9343bab16e72@intel.com

---
Ira Weiny (4):
      cxl/mem: Remove unused CXL_CMD_FLAG_NONE define
      cxl/uapi: Add warning on CXL command enum
      cxl/uapi: Only return valid commands from cxl_query_cmd()
      cxl/mem: Fix UAPI command comment

 drivers/cxl/core/mbox.c      | 35 ++++++++++++++++++++++++++---------
 drivers/cxl/cxlmem.h         |  1 -
 include/uapi/linux/cxl_mem.h | 11 ++++++++---
 3 files changed, 34 insertions(+), 13 deletions(-)
---
base-commit: 1fe4fd6f5cad346e598593af36caeadc4f5d4fa9
change-id: 20221222-cxl-misc-793ec2442455

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH v2 1/4] cxl/mem: Remove unused CXL_CMD_FLAG_NONE define

[Ira Weiny]

CXL_CMD_FLAG_NONE is not used, remove it.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v2:
	Pick up tag
---
 drivers/cxl/cxlmem.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ab138004f644..2d85776236dd 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -393,7 +393,6 @@ struct cxl_mem_command {
 	struct cxl_command_info info;
 	enum cxl_opcode opcode;
 	u32 flags;
-#define CXL_CMD_FLAG_NONE 0
 #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
 };
 

-- 
2.39.1

Re: [PATCH v2 1/4] cxl/mem: Remove unused CXL_CMD_FLAG_NONE define

[Dave Jiang]

On 1/27/23 5:09 PM, Ira Weiny wrote:
> CXL_CMD_FLAG_NONE is not used, remove it.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> 
> ---
> Changes for v2:
> 	Pick up tag
> ---
>   drivers/cxl/cxlmem.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ab138004f644..2d85776236dd 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -393,7 +393,6 @@ struct cxl_mem_command {
>   	struct cxl_command_info info;
>   	enum cxl_opcode opcode;
>   	u32 flags;
> -#define CXL_CMD_FLAG_NONE 0
>   #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
>   };
>   
> 

[PATCH v2 2/4] cxl/uapi: Add warning on CXL command enum

[Ira Weiny]

The CXL command enum is exported to user space and must maintain
backwards compatibility.

Add comment that new defines must be added to the end of the list.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v2:
	Pick up tag
---
 include/uapi/linux/cxl_mem.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c71021a2a9ed..459a3f7f764b 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -19,6 +19,10 @@
 #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)
 
+/*
+ * NOTE: New defines must be added to the end of the list to preserve
+ * compatibility because this enum is exported to user space.
+ */
 #define CXL_CMDS                                                          \
 	___C(INVALID, "Invalid Command"),                                 \
 	___C(IDENTIFY, "Identify Command"),                               \

-- 
2.39.1

Re: [PATCH v2 2/4] cxl/uapi: Add warning on CXL command enum

[Dave Jiang]

On 1/27/23 5:09 PM, Ira Weiny wrote:
> The CXL command enum is exported to user space and must maintain
> backwards compatibility.
> 
> Add comment that new defines must be added to the end of the list.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> 
> ---
> Changes for v2:
> 	Pick up tag
> ---
>   include/uapi/linux/cxl_mem.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index c71021a2a9ed..459a3f7f764b 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -19,6 +19,10 @@
>   #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)
>   
> +/*
> + * NOTE: New defines must be added to the end of the list to preserve
> + * compatibility because this enum is exported to user space.
> + */
>   #define CXL_CMDS                                                          \
>   	___C(INVALID, "Invalid Command"),                                 \
>   	___C(IDENTIFY, "Identify Command"),                               \
> 

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

[Ira Weiny]

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;
+		}
 
 		if (j == n_commands)
 			break;

-- 
2.39.1

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

[Dan Williams]

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)

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

[Jonathan Cameron]

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)

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

[Dan Williams]

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)

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

[Jonathan Cameron]

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

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

[Ira Weiny]

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

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

[Dan Williams]

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.

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

[Ira Weiny]

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

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

[Dan Williams]

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.

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

[Alison Schofield]

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

[PATCH v2 4/4] cxl/mem: Fix UAPI command comment

[Ira Weiny]

The command comment had grammatical errors.  In an attempt to fix those
it was noted that the comment and the query command were not in sync.

Now that the query command accounts for excluded and device unsupported
commands.  Update the kdoc and fix the grammatical errors.

[1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v2:
	Add patch to fix the query command and adjust the comment for
	that support.
---
 include/uapi/linux/cxl_mem.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 459a3f7f764b..c58101437ea7 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -11,9 +11,10 @@
 /**
  * DOC: UAPI
  *
- * Not all of all commands that the driver supports are always available for use
- * by userspace. Userspace must check the results from the QUERY command in
- * order to determine the live set of commands.
+ * Not all of the commands that the driver supports are available for use by
+ * userspace at all times.  Userspace can check the result of the QUERY command
+ * to determine the live set of commands.  Alternatively, it can issue the
+ * command and check for failure.
  */
 
 #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)

-- 
2.39.1