[PATCH v3 0/4] CXL: Miscellaneous fixes

[PATCH v3 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 v3:
- Pick up tags from v2
- Dan : Return flags to user space rather than masking commands
- Link to v2: https://lore.kernel.org/r/20221222-cxl-misc-v2-0-60403cc37257@intel.com

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

 drivers/cxl/core/mbox.c      |  9 +++++++--
 drivers/cxl/cxlmem.h         |  1 -
 include/uapi/linux/cxl_mem.h | 15 +++++++++++----
 3 files changed, 18 insertions(+), 7 deletions(-)
---
base-commit: 22b8077d0fcec86c6ed0e0fce9f7e7e5a4c2d56a
change-id: 20221222-cxl-misc-793ec2442455

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

[PATCH v3 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>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 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

[PATCH v3 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>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 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

[PATCH v3 3/4] cxl/uapi: Tag 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.  This can't be done
without additional information being returned from cxl_query_cmd().  In
addition, information about the availability of commands can be useful
for debugging.

Add flags to struct cxl_command_info which reflect if a command is
enabled and/or exclusive to the kernel.

[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 v3:
	Dan
		Pass enabled/exclusive flags to user space rather than
		masking those commands.
---
 drivers/cxl/core/mbox.c      | 9 +++++++--
 include/uapi/linux/cxl_mem.h | 4 +++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index b03fba212799..6ab4613f3cf9 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -445,9 +445,14 @@ 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;
 
-		if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
+		if (test_bit(info.id, cxlmd->cxlds->enabled_cmds))
+			info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
+		if (test_bit(info.id, cxlmd->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)
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 459a3f7f764b..4aee64b7cc1a 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -91,7 +91,9 @@ struct cxl_command_info {
 	__u32 id;
 
 	__u32 flags;
-#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0)
+#define CXL_MEM_COMMAND_FLAG_MASK		GENMASK(1, 0)
+#define CXL_MEM_COMMAND_FLAG_ENABLED		BIT(0)
+#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE		BIT(1)
 
 	__u32 size_in;
 	__u32 size_out;

-- 
2.39.1

[PATCH v3 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 returns excluded and device unsupported
command information.  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 v3:
	Adjust commit message for updated query command behavior.
---
 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 4aee64b7cc1a..b1d41e3be4e5 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

Re: [PATCH v3 3/4] cxl/uapi: Tag commands from cxl_query_cmd()

[Jonathan Cameron]

On Wed, 01 Feb 2023 17:23:12 -0800
Ira Weiny <ira.weiny@intel.com> 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.  This can't be done
> without additional information being returned from cxl_query_cmd().  In
> addition, information about the availability of commands can be useful
> for debugging.
> 
> Add flags to struct cxl_command_info which reflect if a command is
> enabled and/or exclusive to the kernel.
> 
> [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>

No mention of why we can use space in the existing defined flag mask?

I couldn't find a reason why we couldn't previously but Dan's proposal
was to reserve that first bit so I'm wondering on the reasoning one
way or the other!

Other than that detail, this looks like a good solution to me.

Jonathan


> 
> ---
> Changes for v3:
> 	Dan
> 		Pass enabled/exclusive flags to user space rather than
> 		masking those commands.
> ---
>  drivers/cxl/core/mbox.c      | 9 +++++++--
>  include/uapi/linux/cxl_mem.h | 4 +++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index b03fba212799..6ab4613f3cf9 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -445,9 +445,14 @@ 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;
>  
> -		if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> +		if (test_bit(info.id, cxlmd->cxlds->enabled_cmds))
> +			info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
> +		if (test_bit(info.id, cxlmd->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)
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 459a3f7f764b..4aee64b7cc1a 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -91,7 +91,9 @@ struct cxl_command_info {
>  	__u32 id;
>  
>  	__u32 flags;
> -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0)
> +#define CXL_MEM_COMMAND_FLAG_MASK		GENMASK(1, 0)
> +#define CXL_MEM_COMMAND_FLAG_ENABLED		BIT(0)
> +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE		BIT(1)
>  
>  	__u32 size_in;
>  	__u32 size_out;
> 

Re: [PATCH v3 3/4] cxl/uapi: Tag commands from cxl_query_cmd()

[Jonathan Cameron]

On Thu, 2 Feb 2023 09:58:21 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 01 Feb 2023 17:23:12 -0800
> Ira Weiny <ira.weiny@intel.com> 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.  This can't be done
> > without additional information being returned from cxl_query_cmd().  In
> > addition, information about the availability of commands can be useful
> > for debugging.
> > 
> > Add flags to struct cxl_command_info which reflect if a command is
> > enabled and/or exclusive to the kernel.
> > 
> > [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>  
> 
> No mention of why we can use space in the existing defined flag mask?
> 
> I couldn't find a reason why we couldn't previously but Dan's proposal
> was to reserve that first bit so I'm wondering on the reasoning one
> way or the other!
> 
> Other than that detail, this looks like a good solution to me.

On second thoughts. Some docs for the two flags explaining what they
mean would be good to have (as there isn't a reference to how to use
them in the Docs patch that follows this).

> 
> Jonathan
> 
> 
> > 
> > ---
> > Changes for v3:
> > 	Dan
> > 		Pass enabled/exclusive flags to user space rather than
> > 		masking those commands.
> > ---
> >  drivers/cxl/core/mbox.c      | 9 +++++++--
> >  include/uapi/linux/cxl_mem.h | 4 +++-
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index b03fba212799..6ab4613f3cf9 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -445,9 +445,14 @@ 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;
> >  
> > -		if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> > +		if (test_bit(info.id, cxlmd->cxlds->enabled_cmds))
> > +			info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
> > +		if (test_bit(info.id, cxlmd->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)
> > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> > index 459a3f7f764b..4aee64b7cc1a 100644
> > --- a/include/uapi/linux/cxl_mem.h
> > +++ b/include/uapi/linux/cxl_mem.h
> > @@ -91,7 +91,9 @@ struct cxl_command_info {
> >  	__u32 id;
> >  
> >  	__u32 flags;
> > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0)
> > +#define CXL_MEM_COMMAND_FLAG_MASK		GENMASK(1, 0)
> > +#define CXL_MEM_COMMAND_FLAG_ENABLED		BIT(0)
> > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE		BIT(1)
> >  
> >  	__u32 size_in;
> >  	__u32 size_out;
> >   
> 

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

[Jonathan Cameron]

On Wed, 01 Feb 2023 17:23:13 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> 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 returns excluded and device unsupported
> command information.  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>

Looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> Changes for v3:
> 	Adjust commit message for updated query command behavior.
> ---
>  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 4aee64b7cc1a..b1d41e3be4e5 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)
> 

Re: [PATCH v3 3/4] cxl/uapi: Tag commands from cxl_query_cmd()

[Ira Weiny]

Jonathan Cameron wrote:
> On Thu, 2 Feb 2023 09:58:21 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Wed, 01 Feb 2023 17:23:12 -0800
> > Ira Weiny <ira.weiny@intel.com> 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.  This can't be done
> > > without additional information being returned from cxl_query_cmd().  In
> > > addition, information about the availability of commands can be useful
> > > for debugging.
> > > 
> > > Add flags to struct cxl_command_info which reflect if a command is
> > > enabled and/or exclusive to the kernel.
> > > 
> > > [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>  
> > 
> > No mention of why we can use space in the existing defined flag mask?
> > 
> > I couldn't find a reason why we couldn't previously but Dan's proposal
> > was to reserve that first bit so I'm wondering on the reasoning one
> > way or the other!
> > 
> > Other than that detail, this looks like a good solution to me.

The reason was covered in the previous discussion.  I pointed out that the flag
Dan mentioned was an internal one which did not apply here at all.  So I did
not mention it.

	> > 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.

	-- https://lore.kernel.org/all/63d85857b1648_8e2c29465@dwillia2-mobl3.amr.corp.intel.com.notmuch/

> 
> On second thoughts. Some docs for the two flags explaining what they
> mean would be good to have (as there isn't a reference to how to use
> them in the Docs patch that follows this).

Good point.

I'll respin with kdocs for the following:

/*
 * 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)

Ira