Re: [PATCH v4 10/15] cxl: Add support for fwctl RPC command to enable CXL feature commands

[Dave Jiang <dave.jiang@intel.com>]

On 2/8/25 12:09 AM, Li Ming wrote:
> On 2/8/2025 7:37 AM, 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. The flag
>> field of the Feature details will be cleared of the "Changeable"
>> field and the "set feat size" will be set to 0 to indicate that
>> the feature is not changeable.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v4:
>> - Use opcode directly instead of command ids. (Dan)
>> - Add check for no immediate effects and no reset effects. (Dan, Jonathan)
>> - Add check for Set Features to have CXL_FEATURES_RW cap.
>> - Add clearing of the "Changeable" bit in the flags for exclusive features.
>> ---
>>  drivers/cxl/core/features.c | 221 +++++++++++++++++++++++++++++++++++-
>>  include/uapi/cxl/features.h |   1 +
>>  include/uapi/fwctl/cxl.h    |  37 ++++++
>>  3 files changed, 257 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
>> index b2836b951360..2682067dd2d7 100644
>> --- a/drivers/cxl/core/features.c
>> +++ b/drivers/cxl/core/features.c
>> @@ -365,11 +365,228 @@ 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->entries->num_features; i++) {
>> +		feat = &cxlfs->entries->ent[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->entries->num_features)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	requested = min_t(int, requested, cxlfs->entries->num_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->entries->num_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->entries->num_features; i++, pos++) {
> 
> My understanding is that 'i' should start from the value of the 'start' got from 'feat_in.start_idx'.
> 
> Please correct me if I'm wrong.

You are correct. It needs to be
for (i = start, pos = &feat_out->ents[0];
     i < cxlfs->entries->num_features; i++; pos++) {
	if (i - start == requested)
		break;
	...
}

Thanks for catching that.

DJ


> 
> 
> Ming
> 
>> +		if (i == requested)
>> +			break;
>> +
>> +		memcpy(pos, &cxlfs->entries->ent[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)) {
>> +			u32 flags;
>> +
>> +			pos->set_feat_size = 0;
>> +			flags = le32_to_cpu(pos->flags);
>> +			flags &= ~CXL_FEATURE_F_CHANGEABLE;
>> +			pos->flags = cpu_to_le32(flags);
>> +		}
>> +	}
>> +
>> +	feat_out->num_entries = cpu_to_le16(requested);
>> +	feat_out->supported_feats = cpu_to_le16(cxlfs->entries->num_features);
>> +	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
>> +	*out_len = out_size;
>> +
>> +	return no_free_ptr(rpc_out);
>> +}
>> +
> [...]