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

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

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