Message-ID: <fb98da17362d4b48b7d6e7a1547b6443@huawei.com>
Date: Wed, 25 Sep 2024 11:18:34 +0000
From: Shiju Jose <shiju.jose@...wei.com>
To: Dave Jiang <dave.jiang@...el.com>, "linux-edac@...r.kernel.org"
	<linux-edac@...r.kernel.org>, "linux-cxl@...r.kernel.org"
	<linux-cxl@...r.kernel.org>, "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "bp@...en8.de" <bp@...en8.de>, "tony.luck@...el.com"
	<tony.luck@...el.com>, "rafael@...nel.org" <rafael@...nel.org>,
	"lenb@...nel.org" <lenb@...nel.org>, "mchehab@...nel.org"
	<mchehab@...nel.org>, "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"dave@...olabs.net" <dave@...olabs.net>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, "alison.schofield@...el.com"
	<alison.schofield@...el.com>, "vishal.l.verma@...el.com"
	<vishal.l.verma@...el.com>, "ira.weiny@...el.com" <ira.weiny@...el.com>,
	"david@...hat.com" <david@...hat.com>, "Vilas.Sridharan@....com"
	<Vilas.Sridharan@....com>, "leo.duran@....com" <leo.duran@....com>,
	"Yazen.Ghannam@....com" <Yazen.Ghannam@....com>, "rientjes@...gle.com"
	<rientjes@...gle.com>, "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
	"Jon.Grimm@....com" <Jon.Grimm@....com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "naoya.horiguchi@....com"
	<naoya.horiguchi@....com>, "james.morse@....com" <james.morse@....com>,
	"jthoughton@...gle.com" <jthoughton@...gle.com>, "somasundaram.a@....com"
	<somasundaram.a@....com>, "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
	"pgonda@...gle.com" <pgonda@...gle.com>, "duenwen@...gle.com"
	<duenwen@...gle.com>, "mike.malvestuto@...el.com"
	<mike.malvestuto@...el.com>, "gthelen@...gle.com" <gthelen@...gle.com>,
	"wschwartz@...erecomputing.com" <wschwartz@...erecomputing.com>,
	"dferguson@...erecomputing.com" <dferguson@...erecomputing.com>,
	"wbs@...amperecomputing.com" <wbs@...amperecomputing.com>,
	"nifan.cxl@...il.com" <nifan.cxl@...il.com>, "jgroves@...ron.com"
	<jgroves@...ron.com>, "vsalve@...ron.com" <vsalve@...ron.com>, tanxiaofei
	<tanxiaofei@...wei.com>, "Zengtao (B)" <prime.zeng@...ilicon.com>, "Roberto
 Sassu" <roberto.sassu@...wei.com>, "kangkang.shen@...urewei.com"
	<kangkang.shen@...urewei.com>, wanghuiqiang <wanghuiqiang@...wei.com>,
	Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH v12 07/17] cxl: Add Get Supported Features command for
 kernel usage



>-----Original Message-----
>From: Dave Jiang <dave.jiang@...el.com>
>Sent: 24 September 2024 00:34
>To: Shiju Jose <shiju.jose@...wei.com>; linux-edac@...r.kernel.org; linux-
>cxl@...r.kernel.org; linux-acpi@...r.kernel.org; linux-mm@...ck.org; linux-
>kernel@...r.kernel.org
>Cc: bp@...en8.de; tony.luck@...el.com; rafael@...nel.org; lenb@...nel.org;
>mchehab@...nel.org; dan.j.williams@...el.com; dave@...olabs.net; Jonathan
>Cameron <jonathan.cameron@...wei.com>; alison.schofield@...el.com;
>vishal.l.verma@...el.com; ira.weiny@...el.com; david@...hat.com;
>Vilas.Sridharan@....com; leo.duran@....com; Yazen.Ghannam@....com;
>rientjes@...gle.com; jiaqiyan@...gle.com; Jon.Grimm@....com;
>dave.hansen@...ux.intel.com; naoya.horiguchi@....com;
>james.morse@....com; jthoughton@...gle.com; somasundaram.a@....com;
>erdemaktas@...gle.com; pgonda@...gle.com; duenwen@...gle.com;
>mike.malvestuto@...el.com; gthelen@...gle.com;
>wschwartz@...erecomputing.com; dferguson@...erecomputing.com;
>wbs@...amperecomputing.com; nifan.cxl@...il.com; jgroves@...ron.com;
>vsalve@...ron.com; tanxiaofei <tanxiaofei@...wei.com>; Zengtao (B)
><prime.zeng@...ilicon.com>; Roberto Sassu <roberto.sassu@...wei.com>;
>kangkang.shen@...urewei.com; wanghuiqiang <wanghuiqiang@...wei.com>;
>Linuxarm <linuxarm@...wei.com>
>Subject: Re: [PATCH v12 07/17] cxl: Add Get Supported Features command for
>kernel usage
>
>
>
>On 9/11/24 2:04 AM, shiju.jose@...wei.com wrote:
>> From: Dave Jiang <dave.jiang@...el.com>
>>
>> CXL spec r3.1 8.2.9.6.1 Get Supported Features (Opcode 0500h) The
>> command retrieve the list of supported device-specific features
>> (identified by UUID) and general information about each Feature.
>>
>> The driver will retrieve the feature entries in order to make checks
>> and provide information for the Get Feature and Set Feature command.
>> One of the main piece of information retrieved are the effects a Set
>> Feature command would have for a particular feature.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@...el.com>
>> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
>> ---
>>  drivers/cxl/core/mbox.c      | 175 +++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h         |  51 ++++++++++
>>  drivers/cxl/pci.c            |   4 +
>>  include/uapi/linux/cxl_mem.h |   1 +
>>  4 files changed, 231 insertions(+)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> fa1ee495a4e3..fe965ec5802f 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -67,6 +67,7 @@ static struct cxl_mem_command
>cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD,
>0),
>>  };
>>
>>  /*
>> @@ -790,6 +791,180 @@ static const uuid_t log_uuid[] = {
>>  	[VENDOR_DEBUG_UUID] = DEFINE_CXL_VENDOR_DEBUG_UUID,  };
>>
>> +static void cxl_free_features(void *features) {
>> +	kvfree(features);
>> +}
>> +
>> +static int cxl_get_supported_features_count(struct cxl_dev_state
>> +*cxlds) {
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	struct cxl_mbox_get_sup_feats_out mbox_out;
>> +	struct cxl_mbox_get_sup_feats_in mbox_in;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	int rc;
>> +
>> +	memset(&mbox_in, 0, sizeof(mbox_in));
>> +	mbox_in.count = sizeof(mbox_out);
>> +	memset(&mbox_out, 0, sizeof(mbox_out));
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +		.size_in = sizeof(mbox_in),
>> +		.payload_in = &mbox_in,
>> +		.size_out = sizeof(mbox_out),
>> +		.payload_out = &mbox_out,
>> +		.min_out = sizeof(mbox_out),
>> +	};
>> +	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	cxlds->num_features = le16_to_cpu(mbox_out.supported_feats);
>> +	if (!cxlds->num_features)
>> +		return -ENOENT;
>> +
>> +	return 0;
>> +}
>> +
>> +int cxl_get_supported_features(struct cxl_dev_state *cxlds) {
>> +	int remain_feats, max_size, max_feats, start, rc;
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	int feat_size = sizeof(struct cxl_feat_entry);
>> +	struct cxl_mbox_get_sup_feats_out *mbox_out;
>> +	struct cxl_mbox_get_sup_feats_in mbox_in;
>> +	int hdr_size = sizeof(*mbox_out);
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	struct cxl_mem_command *cmd;
>> +	void *ptr;
>> +
>> +	/* Get supported features is optional, need to check */
>> +	cmd =
>cxl_mem_find_command(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
>> +	if (!cmd)
>> +		return -EOPNOTSUPP;
>> +	if (!test_bit(cmd->info.id, cxl_mbox->enabled_cmds))
>> +		return -EOPNOTSUPP;
>> +
>> +	rc = cxl_get_supported_features_count(cxlds);
>> +	if (rc)
>> +		return rc;
>> +
>> +	struct cxl_feat_entry *entries __free(kvfree) =
>> +		kvmalloc(cxlds->num_features * feat_size, GFP_KERNEL);
>> +
>> +	if (!entries)
>> +		return -ENOMEM;
>> +
>> +	cxlds->entries = no_free_ptr(entries);
>> +	rc = devm_add_action_or_reset(cxl_mbox->host, cxl_free_features,
>> +				      cxlds->entries);
>> +	if (rc)
>> +		return rc;
>> +
>> +	max_size = cxl_mbox->payload_size - hdr_size;
>> +	/* max feat entries that can fit in mailbox max payload size */
>> +	max_feats = max_size / feat_size;
>> +	ptr = &cxlds->entries[0];
>> +
>> +	mbox_out = kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
>> +	if (!mbox_out)
>> +		return -ENOMEM;
>> +
>> +	start = 0;
>> +	remain_feats = cxlds->num_features;
>> +	do {
>> +		int retrieved, alloc_size, copy_feats;
>> +
>> +		if (remain_feats > max_feats) {
>> +			alloc_size = sizeof(*mbox_out) + max_feats * feat_size;
>> +			remain_feats = remain_feats - max_feats;
>> +			copy_feats = max_feats;
>> +		} else {
>> +			alloc_size = sizeof(*mbox_out) + remain_feats *
>feat_size;
>> +			copy_feats = remain_feats;
>> +			remain_feats = 0;
>> +		}
>> +
>> +		memset(&mbox_in, 0, sizeof(mbox_in));
>> +		mbox_in.count = alloc_size;
>> +		mbox_in.start_idx = start;
>> +		memset(mbox_out, 0, alloc_size);
>> +		mbox_cmd = (struct cxl_mbox_cmd) {
>> +			.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +			.size_in = sizeof(mbox_in),
>> +			.payload_in = &mbox_in,
>> +			.size_out = alloc_size,
>> +			.payload_out = mbox_out,
>> +			.min_out = hdr_size,
>> +		};
>> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +		if (rc < 0) {
>> +			kfree(mbox_out);
>> +			return rc;
>> +		}
>> +		if (mbox_cmd.size_out <= hdr_size) {
>> +			rc = -ENXIO;
>> +			goto err;
>> +		}
>> +
>> +		/*
>> +		 * Make sure retrieved out buffer is multiple of feature
>> +		 * entries.
>> +		 */
>> +		retrieved = mbox_cmd.size_out - hdr_size;
>> +		if (retrieved % feat_size) {
>> +			rc = -ENXIO;
>> +			goto err;
>> +		}
>> +
>> +		/*
>> +		 * If the reported output entries * defined entry size !=
>> +		 * retrieved output bytes, then the output package is incorrect.
>> +		 */
>> +		if (mbox_out->num_entries * feat_size != retrieved) {
>> +			rc = -ENXIO;
>> +			goto err;
>> +		}
>> +
>> +		memcpy(ptr, mbox_out->ents, retrieved);
>> +		ptr += retrieved;
>> +		/*
>> +		 * If the number of output entries is less than expected, add the
>> +		 * remaining entries to the next batch.
>> +		 */
>> +		remain_feats += copy_feats - mbox_out->num_entries;
>> +		start += mbox_out->num_entries;
>> +	} while (remain_feats);
>> +
>> +	kfree(mbox_out);
>> +	return 0;
>> +
>> +err:
>> +	kfree(mbox_out);
>> +	cxlds->num_features = 0;
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
>> +
>> +int cxl_get_supported_feature_entry(struct cxl_dev_state *cxlds, const uuid_t
>*feat_uuid,
>> +				    struct cxl_feat_entry *feat_entry_out) {
>> +	struct cxl_feat_entry *feat_entry;
>> +	int count;
>> +
>> +	/* Check CXL dev supports the feature */
>> +	feat_entry = &cxlds->entries[0];
>> +	for (count = 0; count < cxlds->num_features; count++, feat_entry++) {
>> +		if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
>> +			memcpy(feat_entry_out, feat_entry,
>sizeof(*feat_entry_out));
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
>> +
>>  /**
>>   * cxl_enumerate_cmds() - Enumerate commands for a device.
>>   * @mds: The driver data for the operation diff --git
>> a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>> d7c6ffe2a884..5d149e64c247 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -389,6 +389,8 @@ struct cxl_dpa_perf {
>>   * @ram_res: Active Volatile memory capacity configuration
>>   * @serial: PCIe Device Serial Number
>>   * @type: Generic Memory Class device or Vendor Specific Memory
>> device
>> + * @features: number of supported features
>> + * @entries: list of supported feature entries.
>>   */
>>  struct cxl_dev_state {
>>  	struct device *dev;
>> @@ -404,6 +406,8 @@ struct cxl_dev_state {
>>  	u64 serial;
>>  	enum cxl_devtype type;
>>  	struct cxl_mailbox cxl_mbox;
>> +	int num_features;
>> +	struct cxl_feat_entry *entries;
>
>Hi Shiju,
>Going back and refactoring the fwctl code, I think this needs to stay with
>cxl_mailbox. Otherwise it makes it really hard to verify the feature effects for
>userspace. Preferably I don't think we want to expose 'struct cxl_dev_state' to
>fwctl if we don't need to.
>
>DJ

Hi Dave,
Ok. Revert to stay with struct cxl_mailbox and tested. Changes will be in the next version.

Thanks,
Shiju
>
>>  };
>>
>>  /**
>> @@ -482,6 +486,7 @@ enum cxl_opcode {
>>  	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
>>  	CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
>>  	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
>> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
>>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
>> @@ -765,6 +770,48 @@ enum {
>>  	CXL_PMEM_SEC_PASS_USER,
>>  };
>>
>> +/* Get Supported Features (0x500h) CXL r3.1 8.2.9.6.1 */ struct
>> +cxl_mbox_get_sup_feats_in {
>> +	__le32 count;
>> +	__le16 start_idx;
>> +	u8 reserved[2];
>> +} __packed;
>> +
>> +/* Supported Feature Entry : Payload out attribute flags */
>> +#define CXL_FEAT_ENTRY_FLAG_CHANGABLE	BIT(0)
>> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_MASK
>	GENMASK(3, 1)
>> +#define CXL_FEAT_ENTRY_FLAG_PERSIST_ACROSS_FIRMWARE_UPDATE
>	BIT(4)
>> +#define CXL_FEAT_ENTRY_FLAG_SUPPORT_DEFAULT_SELECTION	BIT(5)
>> +#define CXL_FEAT_ENTRY_FLAG_SUPPORT_SAVED_SELECTION	BIT(6)
>> +
>> +enum cxl_feat_attr_value_persistence {
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_NONE,
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_CXL_RESET,
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_HOT_RESET,
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_WARM_RESET,
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_COLD_RESET,
>> +	CXL_FEAT_ATTR_VALUE_PERSISTENCE_MAX
>> +};
>> +
>> +struct cxl_feat_entry {
>> +	uuid_t uuid;
>> +	__le16 id;
>> +	__le16 get_feat_size;
>> +	__le16 set_feat_size;
>> +	__le32 attr_flags;
>> +	u8 get_feat_ver;
>> +	u8 set_feat_ver;
>> +	__le16 set_effects;
>> +	u8 reserved[18];
>> +} __packed;
>> +
>> +struct cxl_mbox_get_sup_feats_out {
>> +	__le16 num_entries;
>> +	__le16 supported_feats;
>> +	u8 reserved[4];
>> +	struct cxl_feat_entry ents[]
>> +__counted_by(le32_to_cpu(supported_feats));
>> +} __packed;
>> +
>>  int cxl_internal_send_cmd(struct cxl_mailbox *cxl_mbox,
>>  			  struct cxl_mbox_cmd *cmd);
>>  int cxl_dev_state_identify(struct cxl_memdev_state *mds); @@ -824,4
>> +871,8 @@ struct cxl_hdm {  struct seq_file;  struct dentry
>> *cxl_debugfs_create_dir(const char *dir);  void cxl_dpa_debug(struct
>> seq_file *file, struct cxl_dev_state *cxlds);
>> +
>> +int cxl_get_supported_features(struct cxl_dev_state *cxlds); int
>> +cxl_get_supported_feature_entry(struct cxl_dev_state *cxlds, const uuid_t
>*feat_uuid,
>> +				    struct cxl_feat_entry *feat_entry_out);
>>  #endif /* __CXL_MEM_H__ */
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
>> 3c73de475bf3..cec88e3a1754 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -872,6 +872,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const
>struct pci_device_id *id)
>>  	if (rc)
>>  		return rc;
>>
>> +	rc = cxl_get_supported_features(cxlds);
>> +	if (rc)
>> +		dev_dbg(&pdev->dev, "No features enumerated.\n");
>> +
>>  	rc = cxl_set_timestamp(mds);
>>  	if (rc)
>>  		return rc;
>> diff --git a/include/uapi/linux/cxl_mem.h
>> b/include/uapi/linux/cxl_mem.h index c6c0fe27495d..bd2535962f70 100644
>> --- a/include/uapi/linux/cxl_mem.h
>> +++ b/include/uapi/linux/cxl_mem.h
>> @@ -50,6 +50,7 @@
>>  	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
>>  	___C(CLEAR_LOG, "Clear Log"),					  \
>>  	___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),	  \
>> +	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),
>	  \
>>  	___C(MAX, "invalid / last command")
>>
>>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
>

Powered by blists - more mailing lists