Re: [PATCH v2 11/16] cxl: Add support for fwctl RPC command to enable CXL feature commands

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

On Fri, 31 Jan 2025 17:42:04 -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_CONFIGRATION. The Set Feature call is gated by the effects
> of the feature reported by Get Supported Features call for the specific
> feature.
> 
> Only "get supported features" is supported in this patch. Additional
> commands will be added in follow on patches. "Get supported features"
> will filter the features that are exclusive to the kernel and only
> report out features that are not kernel only.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Return -EINVAL for get_support_feature_info() instead of -ENOENT. (Dan)
> - Set set_feat_size to 0 for kernel exclusive features.
> - Just don't bother checking the valid (9) bit. (Dan)
> - Reject if the reserved bits are set. (Dan)

A few minor things inline.  Main ones are things that should
be in earlier patches to reduce noise in this one.

Jonathan


> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> index b7984c98645c..93c6174ded20 100644
> --- a/drivers/cxl/fwctl.c
> +++ b/drivers/cxl/fwctl.c
> @@ -35,11 +35,209 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  	return info;
>  }

> +
> +static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
> +					   const struct fwctl_rpc_cxl *rpc_in,
> +					   size_t *out_len)
> +{
> +	struct cxl_mbox_get_sup_feats_out *feat_out;
> +	struct cxl_mbox_get_sup_feats_in feat_in;
> +	struct cxl_feat_entry *pos;
> +	int requested, copied;
> +	size_t out_size;
> +	u32 count;
> +	u16 start;
> +
> +	if (rpc_in->op_size != sizeof(feat_in))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload),
> +			   rpc_in->op_size))
> +		return ERR_PTR(-EFAULT);
> +
> +	count = le32_to_cpu(feat_in.count);
> +	start = le16_to_cpu(feat_in.start_idx);
> +	requested = count / sizeof(*pos);
> +
> +	/*
> +	 * Make sure that the total requested number of entries is not greater
> +	 * than the total number of supported features allowed for userspace.
> +	 */
> +	if (start >= cxlfs->num_user_features)
> +		return ERR_PTR(-EINVAL);
> +
> +	requested = min_t(int, requested, cxlfs->num_user_features - start);
> +
> +	out_size = sizeof(struct fwctl_rpc_cxl_out) + sizeof(*feat_out) +
> +		   requested * sizeof(*pos);
Pity we don't have a pointer to rpc_out until next line, otherwise
could do
	out_size = struct_size(rpc_out, payload,
			       struct_size(feat_out, ents, requested));

Maybe still worth
	out_size = sizeof(struct fwctl_rpc_cxl_out) +
		struct_size(feat_out, ents, requested);
Whilst pos is the right type, it isn't anything in particular in
some code paths so this just seems odd.
 
> +
> +	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
> +		kvzalloc(out_size, GFP_KERNEL);
> +	if (!rpc_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rpc_out->size = sizeof(*feat_out) + requested * sizeof(*pos);

	struct_size for this one.

> +	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;
> +	if (requested == 0) {
> +		feat_out->num_entries = cpu_to_le16(requested);
> +		feat_out->supported_feats =
> +			cpu_to_le16(cxlfs->num_user_features);
> +		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +		*out_len = out_size;
> +		return no_free_ptr(rpc_out);
> +	}
> +
> +	pos = &feat_out->ents[0];
> +
> +	copied = 0;
> +	for (int i = 0; i < cxlfs->num_features; i++, pos++) {
	for (int i = 0, copied = 0, pos = &feat_out->ents[0],
	     i < cxlfs->num_features; i++, pos++)

perhaps, or maybe keep copied outside given it's not adjusted
in the loops control.
 
> +		memcpy(pos, &cxlfs->entries[i], sizeof(*pos));
> +		/*
> +		 * If the feature is exclusive, set the set_feat_size to 0 to
> +		 * indicate that the feature is not changeable.
> +		 */
> +		if (is_cxl_feature_exclusive(pos))
> +			pos->set_feat_size = 0;
> +
> +		copied++;
Could move copied alongside pos++ in the loop definition and
this check to the start of the loop.

I don't mind on these changes.  That loop will get very
fiddly.


> +		if (copied == requested)
> +			break;
> +	}
> +
> +	feat_out->num_entries = cpu_to_le16(requested);
> +	feat_out->supported_feats = cpu_to_le16(cxlfs->num_features);
> +	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +	*out_len = out_size;
> +
> +	return no_free_ptr(rpc_out);
> +}


> +
>  static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> -			   void *rpc_in, size_t in_len, size_t *out_len)
> +			   void *in, size_t in_len, size_t *out_len)

Push renames back to where the stub was introduced.
Reduces noise as I doubt anyone minds them!

>  {
> -	/* Place holder */
> -	return ERR_PTR(-EOPNOTSUPP);
> +	struct fwctl_device *fwctl = uctx->fwctl;
> +	struct cxl_features_state *cxlfs = to_cxl_features_state(fwctl);
> +	const struct fwctl_rpc_cxl *rpc_in = in;
> +	u16 opcode;
> +
> +	opcode = cxl_get_feature_command_opcode(rpc_in->command_id);
> +	if (opcode == 0xffff)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!cxlctl_validate_hw_command(cxlfs, rpc_in, scope, opcode))
> +		return ERR_PTR(-EINVAL);
> +
> +	return cxlctl_handle_commands(cxlfs, rpc_in, out_len, opcode);
>  }
>  
>  static const struct fwctl_ops cxlctl_ops = {
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index eb13d7ee64f0..f43aa761a7ee 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -42,14 +42,6 @@
>  
>  struct cxl_mailbox;
>  
> -/* Index IDs for CXL mailbox Feature commands */
> -enum feature_cmds {
> -	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
> -	CXL_FEATURE_ID_GET_FEATURE,
> -	CXL_FEATURE_ID_SET_FEATURE,
> -	CXL_FEATURE_ID_MAX
> -};
> -
>  /* Feature commands capability supported by a device */
>  enum cxl_features_capability {
>  	CXL_FEATURES_NONE = 0,
> @@ -83,5 +75,6 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  int cxl_set_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  		    u8 feat_version, void *feat_data, size_t feat_data_size,
>  		    u32 feat_flag, u16 offset, u16 *return_code);
> +bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);

Why not put it here directly in patch 7?
Isn't it defined in two places after this patch?


>  
>  #endif