Re: [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

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

On Mon, Dec 05, 2022 at 08:22:39PM -0800, Dan Williams wrote:
> cxl_internal_send_cmd() skips output size validation for variable output
> commands which is not ideal. Most of the time internal usages want to
> fail if the output size does not match what was requested. For other
> commands where the caller cannot predict the size there is usually a
> a header that conveys how much vaild data is in the payload. For those
> cases add @min_out as a parameter to specify what the minimum response
> payload needs to be for the caller to parse the rest of the payload.
> 
> In this patch only Get Supported Logs has that behavior, but going
> forward records retrieval commands like Get Poison List and Get Event
> Records can use @min_out to retrieve a variable amount of records.
> 
> Critically, this validation scheme skips the needs to interrogate the

					       need

> cxl_mem_commands array which in turn frees up the implementation to
> support internal command enabling without also enabling external / user
> commands.
> 

Minor comment below.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/mbox.c |   23 ++++++++++++++---------
>  drivers/cxl/cxlmem.h    |    2 ++
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ed451ca60ce5..c36a3589377a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -166,9 +166,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>  int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  			  struct cxl_mbox_cmd *mbox_cmd)
>  {
> -	const struct cxl_mem_command *cmd =
> -		cxl_mem_find_command(mbox_cmd->opcode);
> -	size_t out_size;
> +	size_t out_size, min_out;
>  	int rc;
>  
>  	if (mbox_cmd->size_in > cxlds->payload_size ||
> @@ -176,6 +174,7 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  		return -E2BIG;
>  
>  	out_size = mbox_cmd->size_out;
> +	min_out = mbox_cmd->min_out;
>  	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>  	if (rc)
>  		return rc;
> @@ -183,14 +182,18 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
>  		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>  
> +	if (!out_size)

	if (out_size == 0)
	???

> +		return 0;
> +
>  	/*
> -	 * Variable sized commands can't be validated and so it's up to the
> -	 * caller to do that if they wish.
> +	 * Variable sized output needs to at least satisfy the caller's
> +	 * minimum if not the fully requested size.
>  	 */
> -	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd->size_out != out_size)
> -			return -EIO;
> -	}
> +	if (min_out == 0)

I prefer this logic but NIT is that they are both used.

Ira

> +		min_out = out_size;
> +
> +	if (mbox_cmd->size_out < min_out)
> +		return -EIO;
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
> @@ -635,6 +638,8 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
>  		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
>  		.size_out = cxlds->payload_size,
>  		.payload_out = ret,
> +		/* At least the record number field must be valid */
> +		.min_out = 2,
>  	};
>  	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0) {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c447577f5ad5..ab138004f644 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -101,6 +101,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   *            outputs commands this is always expected to be deterministic. For
>   *            variable sized output commands, it tells the exact number of bytes
>   *            written.
> + * @min_out: (input) internal command output payload size validation
>   * @return_code: (output) Error code returned from hardware.
>   *
>   * This is the primary mechanism used to send commands to the hardware.
> @@ -115,6 +116,7 @@ struct cxl_mbox_cmd {
>  	void *payload_out;
>  	size_t size_in;
>  	size_t size_out;
> +	size_t min_out;
>  	u16 return_code;
>  };
>  
>