Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <1ad8c9d4-0436-48be-ac55-67a09c747dae@intel.com>
Date:   Wed, 15 Nov 2023 11:20:59 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [RFC PATCH 1/6] cxl/mbox: Add GET_SUPPORTED_FEATURES, GET_FEATURE
 and SET_FEATURE mailbox commands
To:     <shiju.jose@huawei.com>, <linux-cxl@vger.kernel.org>
CC:     <jonathan.cameron@huawei.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>, <linux-acpi@vger.kernel.org>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        <rafael@kernel.org>, <lenb@kernel.org>, <naoya.horiguchi@nec.com>,
        <james.morse@arm.com>, <david@redhat.com>, <jthoughton@google.com>,
        <somasundaram.a@hpe.com>, <erdemaktas@google.com>,
        <"pgonda@pgonda"@google.com>, <duenwen@google.com>,
        <mike.malvestuto@intel.com>, <gthelen@google.com>,
        <tanxiaofei@huawei.com>, <prime.zeng@hisilicon.com>,
        <kangkang.shen@futurewei.com>, <wanghuiqiang@huawei.com>,
        <linuxarm@huawei.com>
References: <20231114125648.1146-1-shiju.jose@huawei.com>
 <20231114125648.1146-2-shiju.jose@huawei.com>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20231114125648.1146-2-shiju.jose@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ODNNSDRpY2U3bEh5ZUpEVnFSOGxUaXlwTlpVVGJSdkdrRFVHZGkxeHlsdklN?=
 =?utf-8?B?aWhZTnhTV3h3TkovQ0YzYUJlcnhzeUkwaFpvVDZabEJwbHNhYkZ5N0hKcTNl?=
 =?utf-8?B?ZnFCSmVkWFlDaUpKRWRCRFFxcmFvcHZSejVQeTNpSjJ1Z1o4Wk1oUFZJUWVH?=
 =?utf-8?B?dEZlM0YzSXMzdmprYlQ2MjNaNUdnM2tVNzBDQVZuNTBoUGVwOXBzc0d6VHg0?=
 =?utf-8?B?bkJQRmIwUVRYbDQzNldnWTBEaU5tZGdocWowblk0U0VOUGZMT05PWk5sRllv?=
 =?utf-8?B?a3dzK3l0amduYVAvOGM5aFFQNHdwZTNCMDVEUDBZYVlVTWRqSW9UWm01K1Jm?=
 =?utf-8?B?clp6YjJuOGZPcWowV3hHeHpOL3BqK0k4MnRRdkZMUEVLZTdjSUcrSW1BeGFI?=
 =?utf-8?B?bTNxWG1iZVZMNFFLU3VnZ0FySE1rOGVNcFhldW12YUU2Sk1ENklha1JINzho?=
 =?utf-8?B?REpUd0J1YTcyNkF5dUN6TnhQakRCNjNISjlWd3NhZThHNms2QSt3WGc0cm42?=
 =?utf-8?B?YTVZajgxMlduUU0vRXdoSjdHbU1jODlMQUZ1c0xUSW8zSHdySUNpcHhHMFRV?=
 =?utf-8?B?aVoyYmZQK1ZGZWpFa3pKbEszem5WUWlQUUNxUG9sZ29CSG1XRVdNd2FKZnR6?=
 =?utf-8?B?bkIweUUvQjhlZEhLMWpUcFd6d2ZPQ1hvalE5aHFTMFAvckh4NjNxL2VHUlp1?=
 =?utf-8?B?SHA2bUxCMmp6Z3orRFNpRWRMdXQ0U0tEaFplcG9NVkppMmR3QmtBNzg3b2JG?=
 =?utf-8?B?QTV6Y1JtQUlWWFRvNVdZNG5raW11S0pJRVRPUm5sTjBxaXJWTWs0bXdPKzY2?=
 =?utf-8?B?RmVESHFjRjVGT1J1TXJNV1B5RVp1VWRyYmNDT0VyejlMQ1dzOTZ5TEtIRHZN?=
 =?utf-8?B?b2wxM2hiUUxZT2h0OVo1ZEZlSDczOWd0QUxoVTJlTzI4Vm8xZ01MSG5YUGk3?=
 =?utf-8?B?VUsyRXVheGR6Z0cxTGJmdGc3WC8ySm9YUHdxU3JYV05YWlRsaFpyM3FVRVBW?=
 =?utf-8?B?Vm5mYVA4NjJGWE95ZGUyL0NyQWEyTlV3L3BiaWtuemk4VHh4RzkycXBiTDJL?=
 =?utf-8?B?ZkJLVlFjM1RMRmZIYkw4M3BsaXE4MDZVbGw3bU12RkNDZUFrcWZDclEyZ0Vp?=
 =?utf-8?B?c3dlOGVuYVVRU3BVWHBrdG5rZWF3RkY4c1lqRTVZdmtjV1RFeU5LWjdvZjhu?=
 =?utf-8?B?UnpVL3Z3bjdXSkVibXd6YkRLelcyNkVBMjlZbkJId2ZTek1DQUd6b2hXRU9y?=
 =?utf-8?B?U21uWU9jMzc3TzdyeXMyUW5xYk52WmorSkdxdndJeWtMWTdzL21zT29IOGFl?=
 =?utf-8?B?cnEwclhZWVd2MXd2VDRDbkI0U2JWR0F2ZnpsdTZTUnJobzRqY3NQT2RIcnJw?=
 =?utf-8?B?TDJyUDdSWE1JcFl0UnlKZEdWZ3hrQU5WSGNCalBlMkg3MExtRjh6aWcrR0U4?=
 =?utf-8?B?LzEwQXYxWStBeHNUalJRd0xIQ1ltKzl5UG1aVXB1dzdXZ3hCeEFJRnZQMFJZ?=
 =?utf-8?B?NnJhWXhaOEFsaysyVEVrZkpkR3ArM1ZJajcwMENJREp5dTF0TEE2M3FGNmNK?=
 =?utf-8?B?NG4rSmZtbzJjUSsxRHpTemlqQTFkdUVZU2FnVU1wTTFwc0ZKUFo3TmFka0xl?=
 =?utf-8?B?Z0hjS1grMXV1dm1kZ0EyZEpocHN0d2xVWDZYd1ZBa0pLd2xvVWhVejJhKzhU?=
 =?utf-8?B?MUNncTg5VTVpQWwzeURSV01vSitzSWxNQTB6cjhiNTNDdTkyNHVRK0w5eEtD?=
 =?utf-8?B?Q3RHVGc2RnVEdCszSTFYNVFFTEllVEZMQU1aUmFvNERIOWZVUEVkcjBaYW9M?=
 =?utf-8?B?ZVpQMkFIeHFWNjRONGtRVnovVG00MDVRbTJ1M0ZZSWMxRlVQZEQwOGxCbWMv?=
 =?utf-8?B?Wm9lWHRNbHVia0lRWEFFNG51QnZoOEV1N2p0Wk80ZUlJZlhuWUZtN09HRTdi?=
 =?utf-8?B?YkhOTnUvZ0k1WFVzR0N0ZDZpNzRHNzdQazVscGFkdTFDbzhkTTl4MnV6U094?=
 =?utf-8?B?ek03K2ptNkkwcmpQNnY5cnNWSlRxRzVPaWs2c0tUaEZuTnMxZHMzcEVnR2ZR?=
 =?utf-8?B?SFBacVVsUHdRNitPc2doUVZYUkFxWEx0Ty9ZRnl2M2pHQkljTUdHSzJNc2sw?=
 =?utf-8?Q?s7fcC2mMO2FgepqkMsFDwxNZI?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c036ad9-e44a-4178-43e0-08dbe607a161
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2023 18:21:05.1257
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jMveYlZTW5OmQ5aGRWb0XscVdpaf4SS4dScFjFiOVeDRdq2Io9d6z1fT94E6HdzEfkUtx1+akAxO+xr3W9NE7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4925
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.0 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Wed, 15 Nov 2023 10:21:31 -0800 (PST)


On 11/14/23 05:56, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox
> commands.
> 
> CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
> CXL devices supports features with changeable attributes.
> 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.

Maybe split this patch out into 3 for each of the individual commands you are enabling. That'll make git bisect easier for debugging.

A few nits but the rest LGTM.

> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/core/mbox.c      | 74 ++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h         | 95 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/cxl_mem.h |  3 ++
>  3 files changed, 172 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 36270dcfb42e..2a6d8ab927bd 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -63,6 +63,9 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_SHUTDOWN_STATE, 0, 0x1, 0),
>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_CMD(GET_FEATURE, 0x15, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0),
>  };
>  
>  /*
> @@ -1303,6 +1306,77 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>  
> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
> +						struct cxl_mbox_get_supp_feats_in *pi)
> +{
> +	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	struct cxl_mbox_get_supp_feats_out *feats_out;

reverse xmas tree declarations

> +
> +	feats_out = kvmalloc(pi->count, GFP_KERNEL);
> +	if (!feats_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
> +		.size_in = sizeof(*pi),
> +		.payload_in = pi,
> +		.size_out = pi->count,
> +		.payload_out = feats_out,
> +		.min_out = sizeof(struct cxl_mbox_get_supp_feats_out),
> +	};
> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> +	if (rc < 0) {
> +		kvfree(feats_out);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return feats_out;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
> +
> +void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi)
> +{
> +	int rc;
> +	void *feat_out;
> +	struct cxl_mbox_cmd mbox_cmd;

reverse xmas tree declarations


> +
> +	feat_out = kvmalloc(pi->count, GFP_KERNEL);
> +	if (!feat_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_FEATURE,
> +		.size_in = sizeof(*pi),
> +		.payload_in = pi,
> +		.size_out = pi->count,
> +		.payload_out = feat_out,
> +		.min_out = pi->count,
> +	};
> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> +	if (rc < 0) {
> +		kvfree(feat_out);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return feat_out;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL);
> +
> +int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size)
> +{
> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_FEATURE,
> +		.size_in = size,
> +		.payload_in = feat_in,
> +	};
> +
> +	return cxl_internal_send_cmd(mds, &mbox_cmd);
> +}
> +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 a2fcbca253f3..fdac686560d4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -506,6 +506,9 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
>  	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,
> @@ -740,6 +743,94 @@ struct cxl_mbox_set_timestamp_in {
>  
>  } __packed;
>  
> +/* Get Supported Features CXL 3.0 Spec 8.2.9.6.1 */
> +/*
> + * Get Supported Features input payload
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-75
> + */
> +struct cxl_mbox_get_supp_feats_in {
> +	__le32 count;
> +	__le16 start_index;
> +	u16 reserved;
> +} __packed;
> +
> +/*
> + * Get Supported Features Supported Feature Entry
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-77
> + */
> +/* 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_DEEPEST_RESET_PERSISTENCE_NONE	0x0
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_CXL_RESET	0x1
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_HOT_RESET	0x2
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_WARM_RESET	0x3
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_COLD_RESET	0x4

I think you can declare the flags as enums?

> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_ACROSS_FW_UPDATE_MASK	BIT(4)
> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_DEFAULT_SEL_SUPPORT_MASK	BIT(5)
> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_SAVED_SEL_SUPPORT_MASK	BIT(6)
> +
> +struct cxl_mbox_supp_feat_entry {
> +	uuid_t uuid;
> +	__le16 feat_index;
> +	__le16 get_feat_size;
> +	__le16 set_feat_size;
> +	__le32 attrb_flags;
> +	u8 get_feat_version;
> +	u8 set_feat_version;
> +	__le16 set_feat_effects;
> +	u8 rsvd[18];
> +}  __packed;
> +
> +/*
> + * Get Supported Features output payload
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-76
> + */
> +struct cxl_mbox_get_supp_feats_out {
> +	__le16 entries;
> +	__le16 nsuppfeats_dev;
> +	u32 reserved;
> +	struct cxl_mbox_supp_feat_entry feat_entries[];
> +} __packed;
> +
> +/* Get Feature CXL 3.0 Spec 8.2.9.6.2 */
> +/*
> + * Get Feature input payload
> + * CXL rev 3.0 section 8.2.9.6.2; Table 8-79
> + */
> +/* Get Feature : Payload in selection */
> +#define CXL_GET_FEAT_CURRENT_VALUE	0x00
> +#define CXL_GET_FEAT_DEFAULT_VALUE	0x01
> +#define CXL_GET_FEAT_SAVED_VALUE	0x02
> +
> +struct cxl_mbox_get_feat_in {
> +	uuid_t uuid;
> +	__le16 offset;
> +	__le16 count;
> +	u8 selection;
> +}  __packed;
> +
> +/* Set Feature CXL 3.0 Spec 8.2.9.6.3 */
> +/*
> + * Set Feature input payload
> + * CXL rev 3.0 section 8.2.9.6.3; Table 8-81
> + */
> +/* Set Feature : Payload in flags */
> +#define CXL_SET_FEAT_FLAG_ACTION_MASK	GENMASK(2, 0)
> +#define CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER	0x0
> +#define CXL_SET_FEAT_FLAG_ACTION_INITIATE_DATA_TRANSFER	0x1
> +#define CXL_SET_FEAT_FLAG_ACTION_CONTINUE_DATA_TRANSFER	0x2
> +#define CXL_SET_FEAT_FLAG_ACTION_FINISH_DATA_TRANSFER	0x3
> +#define CXL_SET_FEAT_FLAG_ACTION_ABORT_DATA_TRANSFER	0x4
> +#define CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET	BIT(3)
> +struct cxl_mbox_set_feat_in {
> +	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;
> @@ -867,6 +958,10 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
>  void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
>  int cxl_set_timestamp(struct cxl_memdev_state *mds);
> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
> +						struct cxl_mbox_get_supp_feats_in *pi);
> +void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi);
> +int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size);
>  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 --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 14bc6e742148..8c89d323cc41 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -46,6 +46,9 @@
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
>  	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
> +	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),           \
> +	___C(GET_FEATURE, "Get Feature"),                                 \
> +	___C(SET_FEATURE, "Set Feature"),                                 \
>  	___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a