Re: [RFC PATCH v2 16/20] fwctl/cxl: Add support to filter exclusive features

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

On 11/22/24 8:05 AM, Jonathan Cameron wrote:
> On Fri, 15 Nov 2024 14:25:49 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add support to allow filtering of CXL features that are exclusive to the
>> kernel and make them not visible to user space. The "get supported
>> features" mailbox command returned to the userspace is emulated and
>> will skip the features that is marked exclusive for the kernel. The
>> exclusion allows certain the feature setting of certain commands such
>> as claimed by RAS to be exclusive to the kernel.
> 
> Well behaved software is only going to issue features that were returned
> by get_supported_features.  However we need to block badly behaved software
> too.  So a check is needed in the set_feature() / get_feature() paths.
> 
> A few queries inline but basically LGTM other than that.
> 
> Jonathan
> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/mbox.c | 115 ++++++++++++++++++++++++++++++++++++----
>>  drivers/fwctl/cxl/cxl.c |   7 +--
>>  include/cxl/features.h  |  52 ++++++++++++++++++
>>  include/cxl/mailbox.h   |   8 ++-
>>  4 files changed, 168 insertions(+), 14 deletions(-)
>>  create mode 100644 include/cxl/features.h
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index e7c5c709ac79..12ace2951f7c 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -5,6 +5,7 @@
>>  #include <linux/ktime.h>
>>  #include <linux/mutex.h>
>>  #include <linux/unaligned.h>
>> +#include <cxl/features.h>
>>  #include <cxl/mailbox.h>
>>  #include <cxlpci.h>
>>  #include <cxlmem.h>
>> @@ -897,6 +898,78 @@ static int handle_mailbox_cmd_from_fwctl(struct cxl_mailbox *cxl_mbox,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Emulate the 'get supported features' mailbox command and only copy out the
>> + * features that are not marked as exclusive for kernel.
>> + */
>> +static void cxl_mbox_get_supported_features_filtered(struct cxl_mailbox *cxl_mbox,
>> +						     struct cxl_mbox_cmd *mbox_cmd)
>> +{
>> +	struct cxl_mbox_get_sup_feats_in *feat_in = mbox_cmd->payload_in;
>> +	struct cxl_mbox_get_sup_feats_out *feat_out = mbox_cmd->payload_out;
>> +	const int feat_out_size = sizeof(*feat_out);
> Worth while?  I'd just use sizeof(*feat_out) inline.
> Compiler will sort out out and it is self documenting.

ok

> 
>> +	struct cxl_feat_entry *pos;
>> +	const int feat_ent_size = sizeof(*pos);
> Same for this.

ok

> 
>> +	int out_count, ents, u;
>> +	u32 count;
>> +	u16 start;
>> +
>> +	count = le32_to_cpu(feat_in->count);
>> +	start = le16_to_cpu(feat_in->start_idx);
>> +	ents = count / sizeof(struct cxl_feat_entry);
>> +	ents -= start;
> 
> You've lost me on this maths. ents is what?

So I'm trying to validate the number of entries I need to copy out when 'start' index gets thrown in the fray. So it calculates the number of entries the output buffer is capable off, and then minus the start index. Although now I think about it, I should compare that to the (total driver supported features - start). Having to deal with 'start' with this "emulation" is a pain.  

> 
> 
>> +	if (ents < 0) {
>> +		mbox_cmd->size_out = 0;
>> +		mbox_cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
>> +		return;
>> +	}
>> +
>> +	if (mbox_cmd->size_out < feat_out_size) {
>> +		mbox_cmd->size_out = 0;
>> +		mbox_cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
>> +		return;
>> +	}
>> +
>> +	feat_out->supported_feats =
>> +		cpu_to_le16(cxl_mbox->num_user_feats);
>> +	if (ents == 0) {
>> +		feat_out->num_entries = cpu_to_le16(0);
>> +		mbox_cmd->size_out = feat_out_size;
>> +		mbox_cmd->return_code = CXL_MBOX_CMD_RC_SUCCESS;
>> +		return;
>> +	}
>> +
>> +	pos = &feat_out->ents[0];
>> +	out_count = feat_out_size;
>> +	for (int i = 0, u = 0; i < cxl_mbox->num_features; i++) {
>> +		struct cxl_feature *feat = &cxl_mbox->entries[i];
>> +
>> +		if (feat->exclusive)
>> +			continue;
>> +
>> +		if (u < start) {
>> +			u++;
>> +			continue;
>> +		}
>> +
>> +		memcpy(pos, &feat->entry, feat_ent_size);
>> +		out_count += feat_ent_size;
>> +		pos++;
>> +		u++;
>> +
>> +		if (u == count)
>> +			break;
>> +
>> +		/* Make sure it does not go over total output buffer size */
>> +		if (out_count + feat_ent_size >= mbox_cmd->size_out)
>> +			break;
>> +	}
>> +
>> +	feat_out->num_entries = cpu_to_le16(u);
>> +	mbox_cmd->size_out = out_count;
>> +	mbox_cmd->return_code = CXL_MBOX_CMD_RC_SUCCESS;
>> +}
> 
> 
>> diff --git a/include/cxl/features.h b/include/cxl/features.h
>> new file mode 100644
>> index 000000000000..6074a3f79d70
>> --- /dev/null
>> +++ b/include/cxl/features.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright(c) 2024 Intel Corporation. */
>> +#ifndef __CXL_FEATS_H_
>> +#define __CXL_FEATS_H_
>> +
>> +#include <linux/uuid.h>
>> +
>> +#define CXL_FEAT_PATROL_SCRUB_UUID						\
>> +	UUID_INIT(0x96dad7d6, 0xfde8, 0x482b, 0xa7, 0x33, 0x75, 0x77, 0x4e,	\
>> +		  0x06, 0xdb, 0x8a)
> 
> We should figure out ordering for this one to avoid introducing the UUIDs in
> Shiju's set then pulling them here.   Shiju, maybe just use the UUID
> part of this file as a patch in your series?
> 
> The exclusive array belongs only with this patch.
> 
> I guess it doesn't really matter if we end up with repeated UUID definitions
> and cleanup after though.
> 
>> +
>> +#define CXL_FEAT_ECS_UUID							\
>> +	UUID_INIT(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba, 0xb9, 0x69, 0x1e,	\
>> +		  0x89, 0x33, 0x86)
>> +
>> +#define CXL_FEAT_SPPR_UUID							\
>> +	UUID_INIT(0x892ba475, 0xfad8, 0x474e, 0x9d, 0x3e, 0x69, 0x2c, 0x91,	\
>> +		  0x75, 0x68, 0xbb)
>> +
>> +#define CXL_FEAT_HPPR_UUID							\
>> +	UUID_INIT(0x80ea4521, 0x786f, 0x4127, 0xaf, 0xb1, 0xec, 0x74, 0x59,	\
>> +		  0xfb, 0x0e, 0x24)
>> +
>> +#define CXL_FEAT_CACHELINE_SPARING_UUID						\
>> +	UUID_INIT(0x96C33386, 0x91dd, 0x44c7, 0x9e, 0xcb, 0xfd, 0xaf, 0x65,	\
>> +		  0x03, 0xba, 0xc4)
>> +
>> +#define CXL_FEAT_ROW_SPARING_UUID						\
>> +	UUID_INIT(0x450ebf67, 0xb135, 0x4f97, 0xa4, 0x98, 0xc2, 0xd5, 0x7f,	\
>> +		  0x27, 0x9b, 0xed)
>> +
>> +#define CXL_FEAT_BANK_SPARING_UUID						\
>> +	UUID_INIT(0x78b79636, 0x90ac, 0x4b64, 0xa4, 0xef, 0xfa, 0xac, 0x5d,	\
>> +		  0x18, 0xa8, 0x63)
>> +
>> +#define CXL_FEAT_RANK_SPARING_UUID						\
>> +	UUID_INIT(0x34dbaff5, 0x0552, 0x4281, 0x8f, 0x76, 0xda, 0x0b, 0x5e,	\
>> +		  0x7a, 0x76, 0xa7)
>> +
>> +#define CXL_FEAT_UUID_MAX	8
> 
> Why not size automatically and use ARRAY_SIZE() to figure out size?

will do

> 
>> +static uuid_t cxl_exclusive_feats[CXL_FEAT_UUID_MAX] = {
>> +	CXL_FEAT_PATROL_SCRUB_UUID,
>> +	CXL_FEAT_ECS_UUID,
>> +	CXL_FEAT_SPPR_UUID,
>> +	CXL_FEAT_HPPR_UUID,
>> +	CXL_FEAT_CACHELINE_SPARING_UUID,
>> +	CXL_FEAT_ROW_SPARING_UUID,
>> +	CXL_FEAT_BANK_SPARING_UUID,
>> +	CXL_FEAT_RANK_SPARING_UUID,
>> +};
>> +
>> +#endif
>