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

[Ira Weiny <ira.weiny@intel.com>]

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