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

[Alison Schofield <alison.schofield@intel.com>]

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