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

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang 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>
> ---
> v3:
> - Cleanup for loop initializers. (Jonathan)
> - Move is_cxl_feature_exclusive() prototype. (Jonathan)
> - Move cxlctl_fwc_rpc() input param name change. (Jonathan)
> - Use struct_size() for calculation. (Jonathan)
> ---
>  drivers/cxl/core/mbox.c     |   9 ++
>  drivers/cxl/cxl.h           |   1 +
>  drivers/cxl/fwctl.c         | 200 +++++++++++++++++++++++++++++++++++-
>  include/cxl/features.h      |   8 --
>  include/uapi/cxl/features.h |   1 +
>  include/uapi/fwctl/cxl.h    |  37 +++++++
>  6 files changed, 246 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9fe552e3d465..d38a5fc1384f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -92,6 +92,15 @@ int cxl_get_feature_command_id(u16 opcode)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_id, "CXL");
>  
> +u16 cxl_get_feature_command_opcode(int feature_id)
> +{
> +	if (feature_id >= ARRAY_SIZE(cxl_feature_commands))
> +		return 0xffff;
> +
> +	return cxl_feature_commands[feature_id];
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_opcode, "CXL");

This can move to drivers/cxl/core/features.c, and it looks like
@feature_id and @cxl_feature_commands are misnamed.

This should be named in terms of fwctl command id to CXL opcode, right?


> +
>  /*
>   * Commands that RAW doesn't permit. The rationale for each:
>   *
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2d6f7c87e5e8..451b2c7e995b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -912,6 +912,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>  bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>  
>  int cxl_get_feature_command_id(u16 opcode);
> +u16 cxl_get_feature_command_opcode(int feature_id);
>  
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> index 85dee01d89d1..39c01ef4dbf2 100644
> --- a/drivers/cxl/fwctl.c
> +++ b/drivers/cxl/fwctl.c
> @@ -35,11 +35,207 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  	return info;
>  }
>  
> +static struct cxl_feat_entry *
> +get_support_feature_info(struct cxl_features_state *cxlfs,
> +			 const struct fwctl_rpc_cxl *rpc_in)
> +{
> +	struct cxl_feat_entry *feat;
> +	uuid_t uuid;
> +
> +	if (rpc_in->op_size < sizeof(uuid))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (copy_from_user(&uuid, u64_to_user_ptr(rpc_in->in_payload),
> +			   sizeof(uuid)))
> +		return ERR_PTR(-EFAULT);
> +
> +	for (int i = 0; i < cxlfs->num_features; i++) {
> +		feat = &cxlfs->entries[i];
> +		if (uuid_equal(&uuid, &feat->uuid))
> +			return feat;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +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;
> +	size_t out_size;
> +	int requested;
> +	u32 count;
> +	u16 start;
> +	int i;
> +
> +	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) +
> +		struct_size(feat_out, ents, requested);
> +
> +	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
> +		kvzalloc(out_size, GFP_KERNEL);
> +	if (!rpc_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rpc_out->size = struct_size(feat_out, ents, requested);
> +	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);
> +	}
> +
> +	for (i = 0, pos = &feat_out->ents[0];
> +	     i < cxlfs->num_features; i++, pos++) {
> +		if (i == requested)
> +			break;
> +
> +		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;
> +	}
> +
> +	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 bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
> +					 const struct fwctl_rpc_cxl *rpc_in,
> +					 enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_feat_entry *feat;
> +	u16 effects, mask;
> +	u32 flags;
> +
> +	feat = get_support_feature_info(cxlfs, rpc_in);
> +	if (IS_ERR(feat))
> +		return false;
> +
> +	/* Ensure that the attribute is changeable */
> +	flags = le32_to_cpu(feat->flags);
> +	if (!(flags & CXL_FEATURE_F_CHANGEABLE))
> +		return false;
> +
> +	effects = le16_to_cpu(feat->effects);
> +
> +	/*
> +	 * Reserved bits are set, rejecting since the effects is not
> +	 * comprehended by the driver.
> +	 */
> +	if (effects & CXL_CMD_EFFECTS_RESERVED) {
> +		dev_warn_once(&cxlfs->cxlmd->dev,
> +			      "Reserved bits set in the Feature effects field!\n");
> +		return false;
> +	}
> +
> +	/* Currently no user background command support */
> +	if (effects & CXL_CMD_BACKGROUND)
> +		return false;
> +
> +	/* Effects cause immediate change, highest security scope is needed */
> +	mask = CXL_CMD_CONFIG_CHANGE_IMMEDIATE |
> +	       CXL_CMD_DATA_CHANGE_IMMEDIATE |
> +	       CXL_CMD_POLICY_CHANGE_IMMEDIATE |
> +	       CXL_CMD_LOG_CHANGE_IMMEDIATE;
> +	if (effects & mask && scope >= FWCTL_RPC_DEBUG_WRITE_FULL)
> +		return true;
> +
> +	/* These effects supported for all WRITE scope */
> +	if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET ||
> +	     effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET ||
> +	     effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET) &&
> +	    scope >= FWCTL_RPC_DEBUG_WRITE)
> +		return true;

Jonathan and I came to the realization that bit[0,9:11] do not matter
because if they are not set there is no effect, and if they are set the
write is still allowed because the effect is post-reset.

The safety check that can be added is that if a Set has no immediate nor
post-reset effect then it is probably a device bug. In that case err on
the side of caution. I.e. all Set Feature must have at least one
immediate or reset effect.