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

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

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.

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

> +	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?


> +	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?

> +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