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:
> > Dan Williams wrote:
> > > Ira Weiny wrote:

[snip]

> > > > +
> > > >  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.
> 
> There is value, similar to the motivation to print the status of enabled
> commands. It is useful to know what functionality is limited by the
> kernel and what functionality is limited by the device.

Ok.  Do you think we should then update libcxl to use these flags?  Or are
you ok with the try and fail behavior now?

[snip]

> 
> > 
> > >  
> > > -               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?
> 
> No, there is no expectation that userspace check these, especially since
> they were not mandated before, but they are optional useful information
> for future debug.

Ok this is obviously easy to incorporate.

> 
> > 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.
> 
> Value is as above, more user visible debug information which will be
> important as different levels of enabling make it out to different
> backport kernels.
> 
> > 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?
> 
> Nothing beyond future debug flexibility.

I'll respin with this change.  Let me know on any user space change you
might want.  Another option would be to add a libcxl 'info' functionality
which reports the above flags?  Not quite sure how that would work exactly
but I could put some thought into that if you want.

Ira