RE: [RFC PATCH v7 03/12] cxl/mbox: Add SET_FEATURE mailbox command

[Shiju Jose <shiju.jose@huawei.com>]

Hi Fan,

>-----Original Message-----
>From: fan <nifan.cxl@gmail.com>
>Sent: 11 March 2024 21:20
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-cxl@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
>mm@kvack.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
>Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; david@redhat.com;
>Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>rientjes@google.com; jiaqiyan@google.com; tony.luck@intel.com;
>Jon.Grimm@amd.com; dave.hansen@linux.intel.com; rafael@kernel.org;
>lenb@kernel.org; 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;
>tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [RFC PATCH v7 03/12] cxl/mbox: Add SET_FEATURE mailbox
>command
>
>On Fri, Feb 23, 2024 at 10:37:14PM +0800, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add support for SET_FEATURE mailbox command.
>>
>> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
>> CXL devices supports features with changeable attributes.
>> The settings of a feature can be optionally modified using Set Feature
>> command.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/core/mbox.c | 67
>+++++++++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h    | 30 ++++++++++++++++++
>>  2 files changed, 97 insertions(+)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> c078e62ea194..d1660bd20bdb 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1366,6 +1366,73 @@ size_t cxl_get_feature(struct cxl_memdev_state
>> *mds,  }  EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL);
>>
>> +int cxl_set_feature(struct cxl_memdev_state *mds,
>> +		    const uuid_t feat_uuid, u8 feat_version,
>> +		    void *feat_data, size_t feat_data_size,
>> +		    u8 feat_flag)
>> +{
>> +	struct cxl_memdev_set_feat_pi {
>> +		struct cxl_mbox_set_feat_hdr hdr;
>> +		u8 feat_data[];
>> +	}  __packed;
>> +	size_t data_in_size, data_sent_size = 0;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	size_t hdr_size;
>> +	int rc = 0;
>> +
>> +	struct cxl_memdev_set_feat_pi *pi __free(kfree) =
>> +					kmalloc(mds->payload_size,
>GFP_KERNEL);
>> +	pi->hdr.uuid = feat_uuid;
>> +	pi->hdr.version = feat_version;
>> +	feat_flag &= ~CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK;
>> +	hdr_size = sizeof(pi->hdr);
>> +	/*
>> +	 * Check minimum mbox payload size is available for
>> +	 * the feature data transfer.
>> +	 */
>> +	if (hdr_size + 10 > mds->payload_size)
>
>Where does this magic number come from?

This represents minimum extra number of bytes to be available in the mail box
for storing the actual feature data to work with multipart feature data transfers.
This will be set as a definition in the next version and however not sure the
best value to be set.
>
>Fan
>
>> +		return -ENOMEM;
>> +
>> +	if ((hdr_size + feat_data_size) <= mds->payload_size) {
>> +		pi->hdr.flags = cpu_to_le32(feat_flag |
>> +
>CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER);
>> +		data_in_size = feat_data_size;
>> +	} else {
>> +		pi->hdr.flags = cpu_to_le32(feat_flag |
>> +
>CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER);
>> +		data_in_size = mds->payload_size - hdr_size;
>> +	}
>> +
>> +	do {
>> +		pi->hdr.offset = cpu_to_le16(data_sent_size);
>> +		memcpy(pi->feat_data, feat_data + data_sent_size,
>data_in_size);
>> +		mbox_cmd = (struct cxl_mbox_cmd) {
>> +			.opcode = CXL_MBOX_OP_SET_FEATURE,
>> +			.size_in = hdr_size + data_in_size,
>> +			.payload_in = pi,
>> +		};
>> +		rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		data_sent_size += data_in_size;
>> +		if (data_sent_size >= feat_data_size)
>> +			return 0;
>> +
>> +		if ((feat_data_size - data_sent_size) <= (mds->payload_size -
>hdr_size)) {
>> +			data_in_size = feat_data_size - data_sent_size;
>> +			pi->hdr.flags = cpu_to_le32(feat_flag |
>> +
>CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER);
>> +		} else {
>> +			pi->hdr.flags = cpu_to_le32(feat_flag |
>> +
>CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER);
>> +		}
>> +	} while (true);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_set_feature, 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
>> bcfefff062a6..a8d4104afa53 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -531,6 +531,7 @@ enum cxl_opcode {
>>  	CXL_MBOX_OP_GET_LOG		= 0x0401,
>>  	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
>>  	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
>> +	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
>>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
>> @@ -773,6 +774,31 @@ struct cxl_mbox_get_feat_in {
>>  	u8 selection;
>>  }  __packed;
>>
>> +/* Set Feature CXL 3.1 Spec 8.2.9.6.3 */
>> +/*
>> + * Set Feature input payload
>> + * CXL rev 3.1 section 8.2.9.6.3 Table 8-101  */
>> +/* Set Feature : Payload in flags */
>> +#define CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK	GENMASK(2, 0)
>> +enum cxl_set_feat_flag_data_transfer {
>> +	CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER,
>> +	CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER,
>> +	CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER,
>> +	CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER,
>> +	CXL_SET_FEAT_FLAG_ABORT_DATA_TRANSFER,
>> +	CXL_SET_FEAT_FLAG_DATA_TRANSFER_MAX
>> +};
>> +#define CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET	BIT(3)
>> +
>> +struct cxl_mbox_set_feat_hdr {
>> +	uuid_t uuid;
>> +	__le32 flags;
>> +	__le16 offset;
>> +	u8 version;
>> +	u8 rsvd[9];
>> +}  __packed;
>> +
>>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */  struct
>> cxl_mbox_poison_in {
>>  	__le64 offset;
>> @@ -912,6 +938,10 @@ size_t cxl_get_feature(struct cxl_memdev_state
>*mds,
>>  		       size_t feat_out_size,
>>  		       size_t feat_out_min_size,
>>  		       enum cxl_get_feat_selection selection);
>> +int cxl_set_feature(struct cxl_memdev_state *mds,
>> +		    const uuid_t feat_uuid, u8 feat_version,
>> +		    void *feat_data, size_t feat_data_size,
>> +		    u8 feat_flag);
>>  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);
>> --
>> 2.34.1
>>

Thanks,
Shiju