RE: [RFC PATCH v5 01/12] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command
From: Shiju Jose
Date: Fri Feb 09 2024 - 06:17:37 EST
Hi Davidlohr,
Thanks for the feedbacks.
>-----Original Message-----
>From: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>Sent: 08 February 2024 20:52
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-cxl@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-
>mm@xxxxxxxxx; Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>;
>dave.jiang@xxxxxxxxx; alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx;
>ira.weiny@xxxxxxxxx; dan.j.williams@xxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx;
>leo.duran@xxxxxxx; Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx;
>jiaqiyan@xxxxxxxxxx; tony.luck@xxxxxxxxx; Jon.Grimm@xxxxxxx;
>dave.hansen@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
>naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
>somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
>duenwen@xxxxxxxxxx; mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx;
>wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
>tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>;
>kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>;
>Linuxarm <linuxarm@xxxxxxxxxx>
>Subject: Re: [RFC PATCH v5 01/12] cxl/mbox: Add GET_SUPPORTED_FEATURES
>mailbox command
>
>On Thu, 11 Jan 2024, shiju.jose@xxxxxxxxxx wrote:
>
>>From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>
>>Add support for GET_SUPPORTED_FEATURES mailbox command.
>>
>>CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
>
>For the whole series, might as well make it 3.1 based. Are you aware of any
>major differences (I have not checked) between versions in either this or any of
>the two users that use feats?
Sure, I will make 3.1 based.
I checked, no differences found in feature commands b/w versions 3.0 and 31 other than
new users of the features commands are present in v3.1 such as memory sparing features and Advanced Programmable Corrected Volatile Memory Error Threshold Feature Discovery and Configuration.
>
>>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.
>>
>>Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>---
>> drivers/cxl/core/mbox.c | 23 ++++++++++++++++
>> drivers/cxl/cxlmem.h | 59 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 82 insertions(+)
>>
>>diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>>36270dcfb42e..6960ce935e73 100644
>>--- a/drivers/cxl/core/mbox.c
>>+++ b/drivers/cxl/core/mbox.c
>>@@ -1303,6 +1303,29 @@ 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,
>>+ struct
>cxl_mbox_get_supp_feats_in *pi,
>>+ void *feats_out)
>>+{
>>+ struct cxl_mbox_cmd mbox_cmd;
>>+ int rc;
>>+
>>+ mbox_cmd = (struct cxl_mbox_cmd) {
>>+ .opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>>+ .size_in = sizeof(*pi),
>>+ .payload_in = pi,
>>+ .size_out = le32_to_cpu(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)
>>+ return rc;
>>+
>>+ return 0;
>>+}
>>+EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, 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..d831dad748f5 100644
>>--- a/drivers/cxl/cxlmem.h
>>+++ b/drivers/cxl/cxlmem.h
>>@@ -506,6 +506,7 @@ 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_IDENTIFY = 0x4000,
>> CXL_MBOX_OP_GET_PARTITION_INFO = 0x4100,
>> CXL_MBOX_OP_SET_PARTITION_INFO = 0x4101,
>>@@ -740,6 +741,61 @@ 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)
>>+
>>+enum cxl_feat_attrib_value_persistence {
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_NONE = 0x0,
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_CXL_RESET = 0x1,
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_HOT_RESET = 0x2,
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_WARM_RESET = 0x3,
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_COLD_RESET = 0x4,
>
>Just leave the enums without explicit values - you are not changing the default
>counting.
Sure.
>
>>+ CXL_FEAT_ATTRIB_VALUE_PERSISTENCE_MAX
>>+};
>>+
>>+#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;
>
>Nit but rename to 'attr', it is more along the kernel coding style (just compare a
>git grep of attrb vs attr). Also applies to the whole series.
Sure. I will modify.
>
>Thanks,
>Davidlohr
>
>>+ 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 Poison List CXL 3.0 Spec 8.2.9.8.4.1 */ struct
>>cxl_mbox_poison_in {
>> __le64 offset;
>>@@ -867,6 +923,9 @@ 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);
>>+int cxl_get_supported_features(struct cxl_memdev_state *mds,
>>+ struct cxl_mbox_get_supp_feats_in *pi,
>>+ void *feats_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);
>>--
>>2.34.1
>>
Thank,
Shiju