diff mbox series

[v11,04/14] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command

Message ID 20240816164238.1902-5-shiju.jose@huawei.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series EDAC: Scrub: introduce generic EDAC RAS control feature driver + CXL/ACPI-RAS2 drivers | expand

Commit Message

Shiju Jose Aug. 16, 2024, 4:42 p.m. UTC 
From: Shiju Jose <shiju.jose@huawei.com>

Add support for GET_SUPPORTED_FEATURES mailbox command.

CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
CXL devices support features with changeable attributes.

CXL spec 3.1 section 8.2.9.6.1 describes Get Supported features command.
Get Supported Features retrieves the list of supported device specific
features. The settings of a feature can be retrieved using Get Feature
and optionally modified using Set Feature.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/core/mbox.c | 68 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h    | 63 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+)

Comments

kernel test robot Aug. 18, 2024, 7:23 p.m. UTC | #1 
Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ras/edac-for-next]
[also build test WARNING on rafael-pm/linux-next rafael-pm/bleeding-edge cxl/next linus/master v6.11-rc3 next-20240816]
[cannot apply to cxl/pending]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/shiju-jose-huawei-com/EDAC-Add-support-for-EDAC-device-feature-s-control/20240817-004442
base:   https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git edac-for-next
patch link:    https://lore.kernel.org/r/20240816164238.1902-5-shiju.jose%40huawei.com
patch subject: [PATCH v11 04/14] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command
config: alpha-randconfig-r131-20240818 (https://download.01.org/0day-ci/archive/20240819/202408190510.dFXq1LrX-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce: (https://download.01.org/0day-ci/archive/20240819/202408190510.dFXq1LrX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408190510.dFXq1LrX-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/cxl/core/mbox.c:1377:26: sparse: sparse: incorrect type in assignment (different base types) @@     expected int nentries @@     got restricted __le16 [usertype] nr_entries @@
   drivers/cxl/core/mbox.c:1377:26: sparse:     expected int nentries
   drivers/cxl/core/mbox.c:1377:26: sparse:     got restricted __le16 [usertype] nr_entries

vim +1377 drivers/cxl/core/mbox.c

  1353	
  1354	int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const uuid_t *feat_uuid,
  1355					    struct cxl_mbox_supp_feat_entry *feat_entry_out)
  1356	{
  1357		struct cxl_mbox_supp_feat_entry *feat_entry;
  1358		int feat_index, feats_out_size;
  1359		int nentries, count;
  1360		int ret;
  1361	
  1362		feat_index = 0;
  1363		feats_out_size = sizeof(struct cxl_mbox_get_supp_feats_out) +
  1364					sizeof(struct cxl_mbox_supp_feat_entry);
  1365		struct cxl_mbox_get_supp_feats_out *feats_out __free(kfree) =
  1366						kmalloc(feats_out_size, GFP_KERNEL);
  1367		if (!feats_out)
  1368			return -ENOMEM;
  1369	
  1370		while (true) {
  1371			memset(feats_out, 0, feats_out_size);
  1372			ret = cxl_get_supported_features(mds, feats_out_size,
  1373							 feat_index, feats_out);
  1374			if (ret)
  1375				return ret;
  1376	
> 1377			nentries = feats_out->nr_entries;
  1378			if (!nentries)
  1379				return -EOPNOTSUPP;
  1380	
  1381			/* Check CXL memdev supports the feature */
  1382			feat_entry = feats_out->feat_entries;
  1383			for (count = 0; count < nentries; count++, feat_entry++) {
  1384				if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
  1385					memcpy(feat_entry_out, feat_entry,
  1386					       sizeof(*feat_entry_out));
  1387					return 0;
  1388				}
  1389			}
  1390			feat_index += nentries;
  1391		}
  1392	}
  1393	EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
  1394
Dave Jiang Aug. 20, 2024, 10:46 p.m. UTC | #2 
On 8/16/24 9:42 AM, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for GET_SUPPORTED_FEATURES mailbox command.
> 
> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
> CXL devices support features with changeable attributes.
> 
> CXL spec 3.1 section 8.2.9.6.1 describes Get Supported features command.
> Get Supported Features retrieves the list of supported device specific
> features. The settings of a feature can be retrieved using Get Feature
> and optionally modified using Set Feature.
> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/core/mbox.c | 68 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    | 63 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..760fa3e1075f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1324,6 +1324,74 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>  
> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
> +			       u32 count, u16 start_index,
> +			       struct cxl_mbox_get_supp_feats_out *feats_out)
> +{
> +	struct cxl_mbox_get_supp_feats_in pi;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int rc;
> +
> +	pi.count = cpu_to_le32(count);
> +	pi.start_index = cpu_to_le16(start_index);
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
> +		.size_in = sizeof(pi),
> +		.payload_in = &pi,
> +		.size_out = count,
> +		.payload_out = feats_out,
> +		.min_out = sizeof(*feats_out),
> +	};
> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
> +
> +int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const uuid_t *feat_uuid,
> +				    struct cxl_mbox_supp_feat_entry *feat_entry_out)

Hi Shiju,
thoughts on storing all the supported features meta info from GET_SUPPORTED_FEATURES command in the cxl core driver during device enumeration and the CXL EDAC code can just query the core instead of reading from the device? Just looking forward to supporting other read/set features operations in the future and avoiding going through this entire read and find routine for each feature.

DJ 

> +{
> +	struct cxl_mbox_supp_feat_entry *feat_entry;
> +	int feat_index, feats_out_size;
> +	int nentries, count;
> +	int ret;
> +
> +	feat_index = 0;
> +	feats_out_size = sizeof(struct cxl_mbox_get_supp_feats_out) +
> +				sizeof(struct cxl_mbox_supp_feat_entry);
> +	struct cxl_mbox_get_supp_feats_out *feats_out __free(kfree) =
> +					kmalloc(feats_out_size, GFP_KERNEL);
> +	if (!feats_out)
> +		return -ENOMEM;
> +
> +	while (true) {
> +		memset(feats_out, 0, feats_out_size);
> +		ret = cxl_get_supported_features(mds, feats_out_size,
> +						 feat_index, feats_out);
> +		if (ret)
> +			return ret;
> +
> +		nentries = feats_out->nr_entries;
> +		if (!nentries)
> +			return -EOPNOTSUPP;
> +
> +		/* Check CXL memdev supports the feature */
> +		feat_entry = feats_out->feat_entries;
> +		for (count = 0; count < nentries; count++, feat_entry++) {
> +			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
> +				memcpy(feat_entry_out, feat_entry,
> +				       sizeof(*feat_entry_out));
> +				return 0;
> +			}
> +		}
> +		feat_index += nentries;
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
> +
>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		       struct cxl_region *cxlr)
>  {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index af8169ccdbc0..9939c771f642 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -531,6 +531,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,
> @@ -700,6 +701,63 @@ struct cxl_mbox_set_timestamp_in {
>  
>  } __packed;
>  
> +/*
> + * Get Supported Features CXL 3.1 Spec 8.2.9.6.1
> + */
> +
> +/*
> + * Get Supported Features input payload
> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-95
> + */
> +struct cxl_mbox_get_supp_feats_in {
> +	__le32 count;
> +	__le16 start_index;
> +	u8 rsvd[2];
> +} __packed;
> +
> +/*
> + * Get Supported Features Supported Feature Entry
> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-97
> + */
> +/* 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_mbox_supp_feat_entry {
> +	uuid_t uuid;
> +	__le16 index;
> +	__le16 get_size;
> +	__le16 set_size;
> +	__le32 attr_flags;
> +	u8 get_version;
> +	u8 set_version;
> +	__le16 set_effects;
> +	u8 rsvd[18];
> +}  __packed;
> +
> +/*
> + * Get Supported Features output payload
> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-96
> + */
> +struct cxl_mbox_get_supp_feats_out {
> +	__le16 nr_entries;
> +	__le16 nr_supported;
> +	u8 rsvd[4];
> +	struct cxl_mbox_supp_feat_entry feat_entries[];
> +} __packed;
> +
>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */
>  struct cxl_mbox_poison_in {
>  	__le64 offset;
> @@ -831,6 +889,11 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  			    enum cxl_event_type event_type,
>  			    const uuid_t *uuid, union cxl_event *evt);
>  int cxl_set_timestamp(struct cxl_memdev_state *mds);
> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
> +			       u32 count, u16 start_index,
> +			       struct cxl_mbox_get_supp_feats_out *feats_out);
> +int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const uuid_t *feat_uuid,
> +				    struct cxl_mbox_supp_feat_entry *feat_entry_out);
>  int cxl_poison_state_init(struct cxl_memdev_state *mds);
>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		       struct cxl_region *cxlr);
Shiju Jose Aug. 21, 2024, 4:11 p.m. UTC | #3 
Hi Dave,

>-----Original Message-----
>From: Dave Jiang <dave.jiang@intel.com>
>Sent: 20 August 2024 23:46
>To: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>cxl@vger.kernel.org; linux-acpi@vger.kernel.org; linux-mm@kvack.org; linux-
>kernel@vger.kernel.org
>Cc: bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
>mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
>Cameron <jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com;
>Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com;
>dave.hansen@linux.intel.com; naoya.horiguchi@nec.com;
>james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
>erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
>mike.malvestuto@intel.com; gthelen@google.com;
>wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
>wbs@os.amperecomputing.com; nifan.cxl@gmail.com; jgroves@micron.com;
>vsalve@micron.com; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
><prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [PATCH v11 04/14] cxl/mbox: Add GET_SUPPORTED_FEATURES
>mailbox command
>
>
>
>On 8/16/24 9:42 AM, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add support for GET_SUPPORTED_FEATURES mailbox command.
>>
>> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
>> CXL devices support features with changeable attributes.
>>
>> CXL spec 3.1 section 8.2.9.6.1 describes Get Supported features command.
>> Get Supported Features retrieves the list of supported device specific
>> features. The settings of a feature can be retrieved using Get Feature
>> and optionally modified using Set Feature.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/core/mbox.c | 68
>+++++++++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h    | 63 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 131 insertions(+)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> 2626f3fff201..760fa3e1075f 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1324,6 +1324,74 @@ int cxl_set_timestamp(struct cxl_memdev_state
>> *mds)  }  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>>
>> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
>> +			       u32 count, u16 start_index,
>> +			       struct cxl_mbox_get_supp_feats_out *feats_out) {
>> +	struct cxl_mbox_get_supp_feats_in pi;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	int rc;
>> +
>> +	pi.count = cpu_to_le32(count);
>> +	pi.start_index = cpu_to_le16(start_index);
>> +
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +		.size_in = sizeof(pi),
>> +		.payload_in = &pi,
>> +		.size_out = count,
>> +		.payload_out = feats_out,
>> +		.min_out = sizeof(*feats_out),
>> +	};
>> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
>> +
>> +int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const
>uuid_t *feat_uuid,
>> +				    struct cxl_mbox_supp_feat_entry
>*feat_entry_out)
>
>Hi Shiju,
>thoughts on storing all the supported features meta info from
>GET_SUPPORTED_FEATURES command in the cxl core driver during device
>enumeration and the CXL EDAC code can just query the core instead of reading
>from the device? Just looking forward to supporting other read/set features
>operations in the future and avoiding going through this entire read and find
>routine for each feature.
Thanks for the suggestion.
I saw you have implementation for storing all the supported features meta info from GET_SUPPORTED_FEATURES command  here. 
https://patchwork.kernel.org/project/cxl/patch/20240718213446.1750135-5-dave.jiang@intel.com/
I am fine basing on it if this patch is ready to go?
>
>DJ

Thanks,
Shiju
>
>> +{
>> +	struct cxl_mbox_supp_feat_entry *feat_entry;
>> +	int feat_index, feats_out_size;
>> +	int nentries, count;
>> +	int ret;
>> +
>> +	feat_index = 0;
>> +	feats_out_size = sizeof(struct cxl_mbox_get_supp_feats_out) +
>> +				sizeof(struct cxl_mbox_supp_feat_entry);
>> +	struct cxl_mbox_get_supp_feats_out *feats_out __free(kfree) =
>> +					kmalloc(feats_out_size, GFP_KERNEL);
>> +	if (!feats_out)
>> +		return -ENOMEM;
>> +
>> +	while (true) {
>> +		memset(feats_out, 0, feats_out_size);
>> +		ret = cxl_get_supported_features(mds, feats_out_size,
>> +						 feat_index, feats_out);
>> +		if (ret)
>> +			return ret;
>> +
>> +		nentries = feats_out->nr_entries;
>> +		if (!nentries)
>> +			return -EOPNOTSUPP;
>> +
>> +		/* Check CXL memdev supports the feature */
>> +		feat_entry = feats_out->feat_entries;
>> +		for (count = 0; count < nentries; count++, feat_entry++) {
>> +			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
>> +				memcpy(feat_entry_out, feat_entry,
>> +				       sizeof(*feat_entry_out));
>> +				return 0;
>> +			}
>> +		}
>> +		feat_index += nentries;
>> +	}
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
>> +
>>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>  		       struct cxl_region *cxlr)
>>  {
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>> af8169ccdbc0..9939c771f642 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -531,6 +531,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,
>> @@ -700,6 +701,63 @@ struct cxl_mbox_set_timestamp_in {
>>
>>  } __packed;
>>
>> +/*
>> + * Get Supported Features CXL 3.1 Spec 8.2.9.6.1  */
>> +
>> +/*
>> + * Get Supported Features input payload
>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-95  */ struct
>> +cxl_mbox_get_supp_feats_in {
>> +	__le32 count;
>> +	__le16 start_index;
>> +	u8 rsvd[2];
>> +} __packed;
>> +
>> +/*
>> + * Get Supported Features Supported Feature Entry
>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-97  */
>> +/* 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_mbox_supp_feat_entry {
>> +	uuid_t uuid;
>> +	__le16 index;
>> +	__le16 get_size;
>> +	__le16 set_size;
>> +	__le32 attr_flags;
>> +	u8 get_version;
>> +	u8 set_version;
>> +	__le16 set_effects;
>> +	u8 rsvd[18];
>> +}  __packed;
>> +
>> +/*
>> + * Get Supported Features output payload
>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-96  */ struct
>> +cxl_mbox_get_supp_feats_out {
>> +	__le16 nr_entries;
>> +	__le16 nr_supported;
>> +	u8 rsvd[4];
>> +	struct cxl_mbox_supp_feat_entry feat_entries[]; } __packed;
>> +
>>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */  struct
>> cxl_mbox_poison_in {
>>  	__le64 offset;
>> @@ -831,6 +889,11 @@ void cxl_event_trace_record(const struct
>cxl_memdev *cxlmd,
>>  			    enum cxl_event_type event_type,
>>  			    const uuid_t *uuid, union cxl_event *evt);  int
>> cxl_set_timestamp(struct cxl_memdev_state *mds);
>> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
>> +			       u32 count, u16 start_index,
>> +			       struct cxl_mbox_get_supp_feats_out *feats_out);
>int
>> +cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const
>uuid_t *feat_uuid,
>> +				    struct cxl_mbox_supp_feat_entry
>*feat_entry_out);
>>  int cxl_poison_state_init(struct cxl_memdev_state *mds);  int
>> cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>  		       struct cxl_region *cxlr);
Dave Jiang Aug. 21, 2024, 4:50 p.m. UTC | #4 
On 8/21/24 9:11 AM, Shiju Jose wrote:
> Hi Dave,
> 
>> -----Original Message-----
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: 20 August 2024 23:46
>> To: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>> cxl@vger.kernel.org; linux-acpi@vger.kernel.org; linux-mm@kvack.org; linux-
>> kernel@vger.kernel.org
>> Cc: bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
>> mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
>> Cameron <jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>> vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com;
>> Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>> rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com;
>> dave.hansen@linux.intel.com; naoya.horiguchi@nec.com;
>> james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
>> erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
>> mike.malvestuto@intel.com; gthelen@google.com;
>> wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
>> wbs@os.amperecomputing.com; nifan.cxl@gmail.com; jgroves@micron.com;
>> vsalve@micron.com; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
>> <prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>;
>> kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>> Linuxarm <linuxarm@huawei.com>
>> Subject: Re: [PATCH v11 04/14] cxl/mbox: Add GET_SUPPORTED_FEATURES
>> mailbox command
>>
>>
>>
>> On 8/16/24 9:42 AM, shiju.jose@huawei.com wrote:
>>> From: Shiju Jose <shiju.jose@huawei.com>
>>>
>>> Add support for GET_SUPPORTED_FEATURES mailbox command.
>>>
>>> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
>>> CXL devices support features with changeable attributes.
>>>
>>> CXL spec 3.1 section 8.2.9.6.1 describes Get Supported features command.
>>> Get Supported Features retrieves the list of supported device specific
>>> features. The settings of a feature can be retrieved using Get Feature
>>> and optionally modified using Set Feature.
>>>
>>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>>> ---
>>>  drivers/cxl/core/mbox.c | 68
>> +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/cxl/cxlmem.h    | 63 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 131 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>>> 2626f3fff201..760fa3e1075f 100644
>>> --- a/drivers/cxl/core/mbox.c
>>> +++ b/drivers/cxl/core/mbox.c
>>> @@ -1324,6 +1324,74 @@ int cxl_set_timestamp(struct cxl_memdev_state
>>> *mds)  }  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>>>
>>> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
>>> +			       u32 count, u16 start_index,
>>> +			       struct cxl_mbox_get_supp_feats_out *feats_out) {
>>> +	struct cxl_mbox_get_supp_feats_in pi;
>>> +	struct cxl_mbox_cmd mbox_cmd;
>>> +	int rc;
>>> +
>>> +	pi.count = cpu_to_le32(count);
>>> +	pi.start_index = cpu_to_le16(start_index);
>>> +
>>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>>> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>>> +		.size_in = sizeof(pi),
>>> +		.payload_in = &pi,
>>> +		.size_out = count,
>>> +		.payload_out = feats_out,
>>> +		.min_out = sizeof(*feats_out),
>>> +	};
>>> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>>> +	if (rc < 0)
>>> +		return rc;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
>>> +
>>> +int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const
>> uuid_t *feat_uuid,
>>> +				    struct cxl_mbox_supp_feat_entry
>> *feat_entry_out)
>>
>> Hi Shiju,
>> thoughts on storing all the supported features meta info from
>> GET_SUPPORTED_FEATURES command in the cxl core driver during device
>> enumeration and the CXL EDAC code can just query the core instead of reading
>>from the device? Just looking forward to supporting other read/set features
>> operations in the future and avoiding going through this entire read and find
>> routine for each feature.
> Thanks for the suggestion.
> I saw you have implementation for storing all the supported features meta info from GET_SUPPORTED_FEATURES command  here. 
> https://patchwork.kernel.org/project/cxl/patch/20240718213446.1750135-5-dave.jiang@intel.com/
> I am fine basing on it if this patch is ready to go?

If you are fine with it then please feel free to incorporate the changes with your code and we'll go through the reviews. Thanks!

>>
>> DJ
> 
> Thanks,
> Shiju
>>
>>> +{
>>> +	struct cxl_mbox_supp_feat_entry *feat_entry;
>>> +	int feat_index, feats_out_size;
>>> +	int nentries, count;
>>> +	int ret;
>>> +
>>> +	feat_index = 0;
>>> +	feats_out_size = sizeof(struct cxl_mbox_get_supp_feats_out) +
>>> +				sizeof(struct cxl_mbox_supp_feat_entry);
>>> +	struct cxl_mbox_get_supp_feats_out *feats_out __free(kfree) =
>>> +					kmalloc(feats_out_size, GFP_KERNEL);
>>> +	if (!feats_out)
>>> +		return -ENOMEM;
>>> +
>>> +	while (true) {
>>> +		memset(feats_out, 0, feats_out_size);
>>> +		ret = cxl_get_supported_features(mds, feats_out_size,
>>> +						 feat_index, feats_out);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		nentries = feats_out->nr_entries;
>>> +		if (!nentries)
>>> +			return -EOPNOTSUPP;
>>> +
>>> +		/* Check CXL memdev supports the feature */
>>> +		feat_entry = feats_out->feat_entries;
>>> +		for (count = 0; count < nentries; count++, feat_entry++) {
>>> +			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
>>> +				memcpy(feat_entry_out, feat_entry,
>>> +				       sizeof(*feat_entry_out));
>>> +				return 0;
>>> +			}
>>> +		}
>>> +		feat_index += nentries;
>>> +	}
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
>>> +
>>>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>>  		       struct cxl_region *cxlr)
>>>  {
>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>>> af8169ccdbc0..9939c771f642 100644
>>> --- a/drivers/cxl/cxlmem.h
>>> +++ b/drivers/cxl/cxlmem.h
>>> @@ -531,6 +531,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,
>>> @@ -700,6 +701,63 @@ struct cxl_mbox_set_timestamp_in {
>>>
>>>  } __packed;
>>>
>>> +/*
>>> + * Get Supported Features CXL 3.1 Spec 8.2.9.6.1  */
>>> +
>>> +/*
>>> + * Get Supported Features input payload
>>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-95  */ struct
>>> +cxl_mbox_get_supp_feats_in {
>>> +	__le32 count;
>>> +	__le16 start_index;
>>> +	u8 rsvd[2];
>>> +} __packed;
>>> +
>>> +/*
>>> + * Get Supported Features Supported Feature Entry
>>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-97  */
>>> +/* 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_mbox_supp_feat_entry {
>>> +	uuid_t uuid;
>>> +	__le16 index;
>>> +	__le16 get_size;
>>> +	__le16 set_size;
>>> +	__le32 attr_flags;
>>> +	u8 get_version;
>>> +	u8 set_version;
>>> +	__le16 set_effects;
>>> +	u8 rsvd[18];
>>> +}  __packed;
>>> +
>>> +/*
>>> + * Get Supported Features output payload
>>> + * CXL rev 3.1 section 8.2.9.6.1 Table 8-96  */ struct
>>> +cxl_mbox_get_supp_feats_out {
>>> +	__le16 nr_entries;
>>> +	__le16 nr_supported;
>>> +	u8 rsvd[4];
>>> +	struct cxl_mbox_supp_feat_entry feat_entries[]; } __packed;
>>> +
>>>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */  struct
>>> cxl_mbox_poison_in {
>>>  	__le64 offset;
>>> @@ -831,6 +889,11 @@ void cxl_event_trace_record(const struct
>> cxl_memdev *cxlmd,
>>>  			    enum cxl_event_type event_type,
>>>  			    const uuid_t *uuid, union cxl_event *evt);  int
>>> cxl_set_timestamp(struct cxl_memdev_state *mds);
>>> +int cxl_get_supported_features(struct cxl_memdev_state *mds,
>>> +			       u32 count, u16 start_index,
>>> +			       struct cxl_mbox_get_supp_feats_out *feats_out);
>> int
>>> +cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const
>> uuid_t *feat_uuid,
>>> +				    struct cxl_mbox_supp_feat_entry
>> *feat_entry_out);
>>>  int cxl_poison_state_init(struct cxl_memdev_state *mds);  int
>>> cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>>  		       struct cxl_region *cxlr);
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2626f3fff201..760fa3e1075f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1324,6 +1324,74 @@  int cxl_set_timestamp(struct cxl_memdev_state *mds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
 
+int cxl_get_supported_features(struct cxl_memdev_state *mds,
+			       u32 count, u16 start_index,
+			       struct cxl_mbox_get_supp_feats_out *feats_out)
+{
+	struct cxl_mbox_get_supp_feats_in pi;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc;
+
+	pi.count = cpu_to_le32(count);
+	pi.start_index = cpu_to_le16(start_index);
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
+		.size_in = sizeof(pi),
+		.payload_in = &pi,
+		.size_out = count,
+		.payload_out = feats_out,
+		.min_out = sizeof(*feats_out),
+	};
+	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
+
+int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const uuid_t *feat_uuid,
+				    struct cxl_mbox_supp_feat_entry *feat_entry_out)
+{
+	struct cxl_mbox_supp_feat_entry *feat_entry;
+	int feat_index, feats_out_size;
+	int nentries, count;
+	int ret;
+
+	feat_index = 0;
+	feats_out_size = sizeof(struct cxl_mbox_get_supp_feats_out) +
+				sizeof(struct cxl_mbox_supp_feat_entry);
+	struct cxl_mbox_get_supp_feats_out *feats_out __free(kfree) =
+					kmalloc(feats_out_size, GFP_KERNEL);
+	if (!feats_out)
+		return -ENOMEM;
+
+	while (true) {
+		memset(feats_out, 0, feats_out_size);
+		ret = cxl_get_supported_features(mds, feats_out_size,
+						 feat_index, feats_out);
+		if (ret)
+			return ret;
+
+		nentries = feats_out->nr_entries;
+		if (!nentries)
+			return -EOPNOTSUPP;
+
+		/* Check CXL memdev supports the feature */
+		feat_entry = feats_out->feat_entries;
+		for (count = 0; count < nentries; count++, feat_entry++) {
+			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
+				memcpy(feat_entry_out, feat_entry,
+				       sizeof(*feat_entry_out));
+				return 0;
+			}
+		}
+		feat_index += nentries;
+	}
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, CXL);
+
 int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		       struct cxl_region *cxlr)
 {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index af8169ccdbc0..9939c771f642 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -531,6 +531,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,
@@ -700,6 +701,63 @@  struct cxl_mbox_set_timestamp_in {
 
 } __packed;
 
+/*
+ * Get Supported Features CXL 3.1 Spec 8.2.9.6.1
+ */
+
+/*
+ * Get Supported Features input payload
+ * CXL rev 3.1 section 8.2.9.6.1 Table 8-95
+ */
+struct cxl_mbox_get_supp_feats_in {
+	__le32 count;
+	__le16 start_index;
+	u8 rsvd[2];
+} __packed;
+
+/*
+ * Get Supported Features Supported Feature Entry
+ * CXL rev 3.1 section 8.2.9.6.1 Table 8-97
+ */
+/* 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_mbox_supp_feat_entry {
+	uuid_t uuid;
+	__le16 index;
+	__le16 get_size;
+	__le16 set_size;
+	__le32 attr_flags;
+	u8 get_version;
+	u8 set_version;
+	__le16 set_effects;
+	u8 rsvd[18];
+}  __packed;
+
+/*
+ * Get Supported Features output payload
+ * CXL rev 3.1 section 8.2.9.6.1 Table 8-96
+ */
+struct cxl_mbox_get_supp_feats_out {
+	__le16 nr_entries;
+	__le16 nr_supported;
+	u8 rsvd[4];
+	struct cxl_mbox_supp_feat_entry feat_entries[];
+} __packed;
+
 /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */
 struct cxl_mbox_poison_in {
 	__le64 offset;
@@ -831,6 +889,11 @@  void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 			    enum cxl_event_type event_type,
 			    const uuid_t *uuid, union cxl_event *evt);
 int cxl_set_timestamp(struct cxl_memdev_state *mds);
+int cxl_get_supported_features(struct cxl_memdev_state *mds,
+			       u32 count, u16 start_index,
+			       struct cxl_mbox_get_supp_feats_out *feats_out);
+int cxl_get_supported_feature_entry(struct cxl_memdev_state *mds, const uuid_t *feat_uuid,
+				    struct cxl_mbox_supp_feat_entry *feat_entry_out);
 int cxl_poison_state_init(struct cxl_memdev_state *mds);
 int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		       struct cxl_region *cxlr);