diff mbox series

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

Message ID 20221222-cxl-misc-v3-3-60d4f35e4b39@intel.com
State Superseded
Headers show
Series CXL: Miscellaneous fixes | expand

Commit Message

Ira Weiny Feb. 2, 2023, 1:23 a.m. UTC 
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(-)

Comments

Jonathan Cameron Feb. 2, 2023, 9:58 a.m. UTC | #1 
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;
>
Jonathan Cameron Feb. 2, 2023, 10:01 a.m. UTC | #2 
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;
> >   
>
Ira Weiny Feb. 2, 2023, 11:58 p.m. UTC | #3 
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
diff mbox series

Patch

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;