Re: [RFC PATCH v2 15/20] fwctl/cxl: Add support for fwctl RPC command to enable CXL feature commands

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 15 Nov 2024 14:25:48 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
> to a device. The cxl fwctl driver will start by supporting the CXL
> feature commands: Get Supported Features, Get Feature, and Set Feature.
> 
> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
> it indicates the security scope of the call. The Get Supported Features
> and Get Feature calls can be executed with the scope of
> FWCTL_RPC_DEBUG_READ_ONLY. The Set Feature call is gated by the effects
> of the feature reported by Get Supported Features call for the specific
> feature.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A few comments inline

Jonathan


>  
> -int cxl_send_cmd(struct cxl_mailbox *cxl_mbox, struct cxl_send_command __user *s)
> +static int cxl_send_cmd(struct cxl_mailbox *cxl_mbox, struct cxl_send_command *send,
> +			struct cxl_mbox_cmd *mbox_cmd)
> +{
> +	int rc;
> +
> +	rc = cxl_validate_cmd_from_user(mbox_cmd, cxl_mbox, send);
> +	if (rc)
> +		return rc;
> +
> +	rc = handle_mailbox_cmd_from_user(cxl_mbox, mbox_cmd, send->out.payload,
> +					  &send->out.size, &send->retval);
> +	if (rc)
> +		return rc;

return handle_mailbox_cmd_from_user()

> +
> +	return 0;
> +}
> +
> +/**
> + * handle_mailbox_cmd_from_fwctl() - Dispatch a mailbox command for userspace.
> + * @cxl_mbox: The mailbox context for the operation.
> + * @mbox_cmd: The validated mailbox command.
> + *
> + * Return:
> + *  * %0	- Mailbox transaction succeeded. This implies the mailbox
> + *		  protocol completed successfully not that the operation itself
> + *		  was successful.
> + *  * %-ENOMEM  - Couldn't allocate a bounce buffer.
> + *  * %-EINTR	- Mailbox acquisition interrupted.
> + *  * %-EXXX	- Transaction level failures.
> + *
> + * Dispatches a mailbox command on behalf of a userspace request.
> + * The output payload is copied to userspace by fwctl.
> + *
> + * See cxl_send_cmd().
> + */
> +static int handle_mailbox_cmd_from_fwctl(struct cxl_mailbox *cxl_mbox,
> +					 struct cxl_mbox_cmd *mbox_cmd)
> +{
> +	struct device *dev = cxl_mbox->host;
Not sure I'd bother for one use.

> +	struct fwctl_rpc_cxl_out *orig_out;
> +	int rc;
> +
> +	/*
> +	 * Save the payload_out pointer and move it to where hardware output
> +	 * can be copied to.
> +	 */
> +	orig_out = mbox_cmd->payload_out;
> +	mbox_cmd->payload_out = (void *)orig_out + sizeof(*orig_out);
Messing around with void * seems awkward.
Isn't this just
	mbox_cmd->payload_out = orig_out + 1;

or (I prefer the above)
	mbox_cmd->payload_out += sizeof(*orig_out);

> +
> +	dev_dbg(dev,
> +		"Submitting %s command for user\n"
> +		"\topcode: %x\n"
> +		"\tsize: %zx\n",
> +		cxl_mem_opcode_to_name(mbox_cmd->opcode),
> +		mbox_cmd->opcode, mbox_cmd->size_in);
> +
> +	rc = cxl_mbox->mbox_send(cxl_mbox, mbox_cmd);
> +	if (rc)
> +		return rc;
> +
> +	orig_out->retval = mbox_cmd->return_code;
> +	mbox_cmd->payload_out = (void *)orig_out;

No need to cast a pointer to void *

> +
> +	return 0;
> +}
> +
> +int cxl_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
> +		      struct fwctl_rpc_cxl *rpc_in,
> +		      struct cxl_mbox_cmd *mbox_cmd, size_t *out_len)
> +{
> +	struct cxl_send_command send_cmd = {
> +		.id = rpc_in->id,
> +		.flags = rpc_in->flags,
> +		.in.size = rpc_in->op_size,
> +		.in.payload = rpc_in->in_payload,
> +		.out.size = *out_len,
> +	};
> +	struct cxl_mem_command *cmd;
> +	int rc;
> +
> +	cmd = cxl_mem_find_command_by_id(rpc_in->id);
> +	if (!cmd)
> +		return -EINVAL;
> +	send_cmd.raw.opcode = cmd->opcode;
> +
> +	rc = cxl_validate_cmd_from_fwctl(mbox_cmd, cxl_mbox, &send_cmd);

I'd not really expect to see fwctl specific code in a function called
simply cxl_mbox_send_cmd().  Perhaps a rename is appropriate?

> +	if (rc)
> +		return rc;
> +
> +	rc = handle_mailbox_cmd_from_fwctl(cxl_mbox, mbox_cmd);
> +	if (rc)
> +		return rc;
> +
> +	guard(rwsem_read)(&cxl_memdev_rwsem);
> +	rc = cxl_mbox->mbox_send(cxl_mbox, mbox_cmd);
> +	if (rc)
> +		return rc;
> +
> +	*out_len = mbox_cmd->size_out;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);

> diff --git a/drivers/fwctl/cxl/cxl.c b/drivers/fwctl/cxl/cxl.c
> index ce8960a9beaa..164f6774a2c1 100644
> --- a/drivers/fwctl/cxl/cxl.c
> +++ b/drivers/fwctl/cxl/cxl.c
> @@ -77,11 +77,106 @@ static void *cxlctl_hw_info(struct fwctl_uctx *uctx, int commands, size_t *out_l
>  	return_ptr(out);
>  }
>  
> +static bool cxlctl_validate_set_features(struct cxl_mailbox *cxl_mbox,
> +					 const struct fwctl_rpc_cxl *rpc_in,
> +					 enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_feat_entry *feat;
> +	bool found = false;
> +	uuid_t uuid;
> +	u16 effects, mask;

Pick an order. Reverse xmas tree maybe.


> +
> +static bool cxlctl_validate_hw_cmds(struct cxl_mailbox *cxl_mbox,
> +				    const struct fwctl_rpc_cxl *rpc_in,
> +				    enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_mem_command *cmd;
> +
> +	/*
> +	 * Only supporting feature commands for now.
> +	 */
> +	if (!cxl_mbox->num_features)
> +		return false;
> +
> +	cmd = cxl_get_mem_command_for_fwctl(cxl_mbox, rpc_in->id);
> +	if (!cmd)
> +		return false;
> +
> +	switch (cmd->opcode) {
> +	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
> +		if (scope >= FWCTL_RPC_CONFIGURATION)
> +			return true;
> +		return false;

		return scope >= FWCTL_RFC_CONFIGURATION;

> +	case CXL_MBOX_OP_GET_FEATURE:
> +		if (scope >= FWCTL_RPC_DEBUG_READ_ONLY)
> +			return true;
> +		return false;
		return scope >= FWCTL_RPC_DEBUG_READ_ONLY;

> +	case CXL_MBOX_OP_SET_FEATURE:
> +		return cxlctl_validate_set_features(cxl_mbox, rpc_in, scope);
> +	default:
> +		return false;
> +	}
> +}
> +

>  static const struct fwctl_ops cxlctl_ops = {
> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> index e753d5d1d708..3e5e9c9362f5 100644
> --- a/include/cxl/mailbox.h
> +++ b/include/cxl/mailbox.h
> @@ -193,5 +193,11 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host);
>  int cxl_mailbox_user_commands_supported(struct cxl_mailbox *cxl_mbox);
>  int cxl_mailbox_user_commands_info_get(struct cxl_mailbox *cxl_mbox, int nr_cmds,
>  				       void *outbuf, size_t *out_len);
> +struct cxl_mem_command *cxl_get_mem_command(u32 id);
> +struct cxl_mem_command *
> +cxl_get_mem_command_for_fwctl(struct cxl_mailbox *cxl_mbox, u32 id);
> +int cxl_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
> +		      struct fwctl_rpc_cxl *rpc_in,
> +		      struct cxl_mbox_cmd *mbox_cmd, size_t *out_len);
>  
>  #endif
> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
> index a32c4c752db6..99804fc72f28 100644
> --- a/include/uapi/fwctl/cxl.h
> +++ b/include/uapi/fwctl/cxl.h


> +/**
> + * struct fwctl_rpc_cxl_out - ioctl9FWCTL_RPC) output for CXL
(FWCTL_RPC)  9/(

> + * @size: Size of the output payload
> + * @retval: Return value from device
> + * @payload: Return data from device
> + */
> +struct fwctl_rpc_cxl_out {
> +	__u32 size;
> +	__u32 retval;
> +	__u8 payload[];
Can we add counted_by to uapi headers?

Seems there are some existing examples so I guess that is fine.

> +};
> +
>  #endif
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 9dd37849c450..4e7c8c03cfe8 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -234,4 +234,16 @@ struct cxl_send_command {
>  	} out;
>  };
>  
> +/*
> + * CXL spec r3.1 Table 8-101 Set Feature Input Payload
> + */
> +struct cxl_set_feature_input {
> +	__u8 uuid[16];
> +	__u32 flags;
> +	__u16 offset;
> +	__u8 version;
> +	__u8 reserved[9];

This first bit matches with cxl_mbox_set_feat_hdr.
Good avoid the duplication.


> +	__u8 data[];
> +} __packed;
> +
>  #endif