[ndctl patch RFC 0/2] add support for IDENTIFY command

[ndctl patch RFC 0/2] add support for IDENTIFY command

[Jehoon Park]

From: jehoon park <jehoon.park@samsung.com>

This patchset supports CXL IDENTIFY mailbox command and corresponding
cxl tool interface command.

CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
information about CXL memory device. The information consist of device's
firmware version, capacity, LSA size, event log size, poison list size,
inject poison limit, poison handling capabilities and QoS telemetry
capabilities. Firmware version, capacity and LSA size are already supported
and used for partition commands or sysfs attributes while others are not.
Since patches about event log [1] and poison [2] are discussed recently,
support for those information will be helpful.

Example:
# cxl identify mem0
FW Revision                              : BWFW VERSION 00
Total Capacity                           : 1.00 GB
Volatile Only Capacity                   : 1.00 GB
Persistent Only Capacity                 : 0 B
Partition Alignment                      : 0 B
Informational Event Log Size             : 0
Warning Event Log Size                   : 0
Failure Event Log Size                   : 0
Fatal Event Log Size                     : 0
LSA Size                                 : 0 B
Poison List Maximum Media Error Records  : 256
Inject Poison Limit                      : 0
Poison Handling Capabilities
Injects Persistent Poison                : Not Supported
Scans for Poison                         : Not Supported
QoS Telemetry Capabilities
Egress Port Congestion                   : Not Supported
Temporary Throughput Reduction           : Not Supported
cxl memdev: cmd_identify: identified 1 mem

This patch is RFC because some of the information are already provided by
"list -m <memdev>" command and sysfs attributes.
I think a separate cxl tool interface command for identify is useful to provide
device's information clearly. In case of nvme-cli, there are separate
interface commands for identifying controller and namespace: id-ctrl and id-ns.

[1] https://lore.kernel.org/linux-cxl/20221216-cxl-ev-log-v7-0-2316a5c8f7d8@intel.com/
[2] https://lore.kernel.org/linux-cxl/cover.1676685180.git.alison.schofield@intel.com/

jehoon park (2):
  libcxl: add accessors for IDENTIFY command
  cxl: add identify command to cxl tool

 Documentation/cxl/cxl-identify.txt |  57 ++++++++++++
 Documentation/cxl/meson.build      |   1 +
 cxl/builtin.h                      |   1 +
 cxl/cxl.c                          |   1 +
 cxl/lib/libcxl.c                   |  73 +++++++++++++++
 cxl/lib/libcxl.sym                 |  11 +++
 cxl/lib/private.h                  |  11 +++
 cxl/libcxl.h                       |  16 ++++
 cxl/memdev.c                       | 141 +++++++++++++++++++++++++++++
 9 files changed, 312 insertions(+)
 create mode 100644 Documentation/cxl/cxl-identify.txt


base-commit: b830c4af984e72e5849c0705669aad2ffa19db13
-- 
2.17.1

[ndctl patch RFC 1/2] libcxl: add accessors for IDENTIFY command

[Jehoon Park]

From: jehoon park <jehoon.park@samsung.com>

Add accessors to retrieve event log size, poison list size,
inject poison limit, poison handling capabilities and QoS
telemetry capabilities from output payload of IDENTIFY command.

Signed-off-by: jehoon park <jehoon.park@samsung.com>
---
 cxl/lib/libcxl.c   | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym | 11 +++++++
 cxl/lib/private.h  | 11 +++++++
 cxl/libcxl.h       | 16 ++++++++++
 4 files changed, 111 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 59e5bdb..12d8e63 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -3775,6 +3775,79 @@ cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd)
 	return cxl_capacity_to_bytes(c->persistent_capacity);
 }
 
+CXL_EXPORT int
+cxl_cmd_identify_get_event_log_size(struct cxl_cmd *cmd,
+				    enum cxl_identify_event event)
+{
+	struct cxl_cmd_identify *id =
+		(struct cxl_cmd_identify *)cmd->send_cmd->out.payload;
+	int rc = cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_IDENTIFY);
+	if (rc)
+		return rc;
+
+	switch (event) {
+	case CXL_IDENTIFY_INFO:
+		return le16_to_cpu(id->info_event_log_size);
+	case CXL_IDENTIFY_WARN:
+		return le16_to_cpu(id->warning_event_log_size);
+	case CXL_IDENTIFY_FAIL:
+		return le16_to_cpu(id->failure_event_log_size);
+	case CXL_IDENTIFY_FATAL:
+		return le16_to_cpu(id->fatal_event_log_size);
+	default:
+		return -EINVAL;
+	}
+}
+
+CXL_EXPORT int cxl_cmd_identify_get_poison_list_max(struct cxl_cmd *cmd)
+{
+	unsigned int max_records = 0;
+	struct cxl_cmd_identify *id =
+		(struct cxl_cmd_identify *)cmd->send_cmd->out.payload;
+	int rc = cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_IDENTIFY);
+	if (rc)
+		return rc;
+
+	for (int i = 0; i < 3; i++)
+		max_records += id->poison_list_max_mer[i] << (8 * i);
+
+	return max_records;
+}
+
+CXL_EXPORT int cxl_cmd_identify_get_inject_poison_limit(struct cxl_cmd *cmd)
+{
+	cmd_get_field_u16(cmd, identify, IDENTIFY, inject_poison_limit);
+}
+
+CXL_EXPORT int cxl_cmd_identify_injects_persistent_poison(struct cxl_cmd *cmd)
+{
+	cmd_get_field_u8_mask(
+		cmd, identify, IDENTIFY, poison_caps,
+		CXL_CMD_IDENTIFY_POISON_HANDLING_CAPABILITIES_INJECTS_PERSISTENT_POISON);
+}
+
+CXL_EXPORT int cxl_cmd_identify_scans_for_poison(struct cxl_cmd *cmd)
+{
+	cmd_get_field_u8_mask(
+		cmd, identify, IDENTIFY, poison_caps,
+		CXL_CMD_IDENTIFY_POISON_HANDLING_CAPABILITIES_SCANS_FOR_POISON);
+}
+
+CXL_EXPORT int cxl_cmd_identify_egress_port_congestion(struct cxl_cmd *cmd)
+{
+	cmd_get_field_u8_mask(
+		cmd, identify, IDENTIFY, qos_telemetry_caps,
+		CXL_CMD_IDENTIFY_QOS_TELEMETRY_CAPABILITIES_EGRESS_PORT_CONGESTION);
+}
+
+CXL_EXPORT int
+cxl_cmd_identify_temporary_throughput_reduction(struct cxl_cmd *cmd)
+{
+	cmd_get_field_u8_mask(
+		cmd, identify, IDENTIFY, qos_telemetry_caps,
+		CXL_CMD_IDENTIFY_QOS_TELEMETRY_CAPABILITIES_TEMPORARY_THROUGHPUT_REDUCTION);
+}
+
 CXL_EXPORT struct cxl_cmd *cxl_cmd_new_raw(struct cxl_memdev *memdev,
 		int opcode)
 {
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 1c6177c..2bd209e 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -249,3 +249,14 @@ global:
 	cxl_decoder_create_ram_region;
 	cxl_region_get_daxctl_region;
 } LIBCXL_4;
+
+LIBCXL_6 {
+global:
+	cxl_cmd_identify_get_event_log_size;
+	cxl_cmd_identify_get_poison_list_max;
+	cxl_cmd_identify_get_inject_poison_limit;
+	cxl_cmd_identify_injects_persistent_poison;
+	cxl_cmd_identify_scans_for_poison;
+	cxl_cmd_identify_egress_port_congestion;
+	cxl_cmd_identify_temporary_throughput_reduction;
+} LIBCXL_5;
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index d648992..aab1552 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -211,6 +211,17 @@ struct cxl_cmd_identify {
 	u8 qos_telemetry_caps;
 } __attribute__((packed));
 
+/* CXL 3.0 8.2.9.8.1.1 Identify Memory Device Poison Handling Capabilities */
+#define CXL_CMD_IDENTIFY_POISON_HANDLING_CAPABILITIES_INJECTS_PERSISTENT_POISON \
+	BIT(0)
+#define CXL_CMD_IDENTIFY_POISON_HANDLING_CAPABILITIES_SCANS_FOR_POISON BIT(1)
+
+/* CXL 3.0 8.2.9.8.1.1 Identify Memory Device QoS Telemetry Capabilities */
+#define CXL_CMD_IDENTIFY_QOS_TELEMETRY_CAPABILITIES_EGRESS_PORT_CONGESTION \
+	BIT(0)
+#define CXL_CMD_IDENTIFY_QOS_TELEMETRY_CAPABILITIES_TEMPORARY_THROUGHPUT_REDUCTION \
+	BIT(1)
+
 struct cxl_cmd_get_lsa_in {
 	le32 offset;
 	le32 length;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 54d9f10..1856bee 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -334,6 +334,22 @@ unsigned long long cxl_cmd_identify_get_volatile_only_size(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_identify_get_partition_align(struct cxl_cmd *cmd);
 unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd);
+
+enum cxl_identify_event {
+	CXL_IDENTIFY_INFO,
+	CXL_IDENTIFY_WARN,
+	CXL_IDENTIFY_FAIL,
+	CXL_IDENTIFY_FATAL,
+};
+
+int cxl_cmd_identify_get_event_log_size(struct cxl_cmd *cmd,
+					enum cxl_identify_event event);
+int cxl_cmd_identify_get_poison_list_max(struct cxl_cmd *cmd);
+int cxl_cmd_identify_get_inject_poison_limit(struct cxl_cmd *cmd);
+int cxl_cmd_identify_injects_persistent_poison(struct cxl_cmd *cmd);
+int cxl_cmd_identify_scans_for_poison(struct cxl_cmd *cmd);
+int cxl_cmd_identify_egress_port_congestion(struct cxl_cmd *cmd);
+int cxl_cmd_identify_temporary_throughput_reduction(struct cxl_cmd *cmd);
 struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
 int cxl_cmd_health_info_get_maintenance_needed(struct cxl_cmd *cmd);
 int cxl_cmd_health_info_get_performance_degraded(struct cxl_cmd *cmd);
-- 
2.17.1

[ndctl patch RFC 2/2] cxl: add identify command to cxl tool

[Jehoon Park]

From: jehoon park <jehoon.park@samsung.com>

Add identify command to cxl-cli. This new command displays
basic information about CXL memory device(s).
Add man page documentation for identify cli command.

Signed-off-by: jehoon park <jehoon.park@samsung.com>
---
 Documentation/cxl/cxl-identify.txt |  57 ++++++++++++
 Documentation/cxl/meson.build      |   1 +
 cxl/builtin.h                      |   1 +
 cxl/cxl.c                          |   1 +
 cxl/memdev.c                       | 141 +++++++++++++++++++++++++++++
 5 files changed, 201 insertions(+)
 create mode 100644 Documentation/cxl/cxl-identify.txt

diff --git a/Documentation/cxl/cxl-identify.txt b/Documentation/cxl/cxl-identify.txt
new file mode 100644
index 0000000..ee25895
--- /dev/null
+++ b/Documentation/cxl/cxl-identify.txt
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-identify(1)
+===============
+
+NAME
+----
+cxl-identify - display basic information about a CXL memdev
+
+SYNOPSIS
+--------
+[verse]
+'cxl identify <mem0> [<mem1>..<memN>] [<options>]'
+
+DESCRIPTION
+-----------
+This command sends an identify command to a CXL memory device, and displays
+the result. Provided information: CXL memory device's capacity
+(total, volatile and persistent), partition alignment, event log size,
+LSA size, poison list size, inject poison limit, poison handling capabilities
+and QoS telemetry capabilities.
+
+EXAMPLE
+-------
+----
+# cxl identify mem0
+FW Revision                              : BWFW VERSION 00
+Total Capacity                           : 1.00 GB
+Volatile Only Capacity                   : 1.00 GB
+Persistent Only Capacity                 : 0 B
+Partition Alignment                      : 0 B
+Informational Event Log Size             : 0
+Warning Event Log Size                   : 0
+Failure Event Log Size                   : 0
+Fatal Event Log Size                     : 0
+LSA Size                                 : 0 B
+Poison List Maximum Media Error Records  : 256
+Inject Poison Limit                      : 0
+Poison Handling Capabilities
+Injects Persistent Poison                : Not Supported
+Scans for Poison                         : Not Supported
+QoS Telemetry Capabilities
+Egress Port Congestion                   : Not Supported
+Temporary Throughput Reduction           : Not Supported
+cxl memdev: cmd_identify: identified 1 mem
+----
+
+OPTIONS
+-------
+<memory device(s)>::
+include::memdev-option.txt[]
+
+include::verbose-option.txt[]
+
+SEE ALSO
+--------
+CXL-3.0 8.2.9.8.1
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index a6d77ab..d60d42d 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -46,6 +46,7 @@ cxl_manpages = [
   'cxl-enable-region.txt',
   'cxl-destroy-region.txt',
   'cxl-monitor.txt',
+  'cxl-identify.txt',
 ]
 
 foreach man : cxl_manpages
diff --git a/cxl/builtin.h b/cxl/builtin.h
index 9baa43b..2276415 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -32,4 +32,5 @@ static inline int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
 	return EXIT_FAILURE;
 }
 #endif
+int cmd_identify(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index 3be7026..be536ea 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -77,6 +77,7 @@ static struct cmd_struct commands[] = {
 	{ "disable-region", .c_fn = cmd_disable_region },
 	{ "destroy-region", .c_fn = cmd_destroy_region },
 	{ "monitor", .c_fn = cmd_monitor },
+	{ "identify", .c_fn = cmd_identify },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 0b3ad02..638107c 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -135,6 +135,136 @@ static const struct option free_dpa_options[] = {
 	OPT_END(),
 };
 
+static const struct option identify_options[] = {
+	BASE_OPTIONS(),
+	OPT_END(),
+};
+
+static void bytes_to_str(unsigned long long bytes, char *str, int len)
+{
+	/* convert bytes to human friendly formats (B, KB, MB, GB, TB) */
+
+	if (bytes == ULLONG_MAX)
+		snprintf(str, len, "Invalid");
+	else if (bytes < SZ_1K)
+		snprintf(str, len, "%lld B", bytes);
+	else if (bytes < SZ_1M)
+		snprintf(str, len, "%.2lf KB", (double)bytes / SZ_1K);
+	else if (bytes < SZ_1G)
+		snprintf(str, len, "%.2lf MB", (double)bytes / SZ_1M);
+	else if (bytes < SZ_1T)
+		snprintf(str, len, "%.2lf GB", (double)bytes / SZ_1G);
+	else
+		snprintf(str, len, "%.2lf TB", (double)bytes / SZ_1T);
+}
+
+static int action_identify(struct cxl_memdev *memdev,
+			   struct action_context *actx)
+{
+	const char *devname = cxl_memdev_get_devname(memdev);
+	struct cxl_cmd *cmd;
+	int rc;
+	char fw_rev[0x10], total_cap[10], volatile_only[10],
+		persistent_only[10], alignment[10], lsa_size[10];
+	int info_log_size, warn_log_size, fail_log_size, fatal_log_size,
+		poison_list_max, inject_poison_limit, inject_persistent_poison,
+		scan_poison, egress_port_congestion, temp_throughput_reduction;
+
+	cmd = cxl_cmd_new_identify(memdev);
+	if (!cmd)
+		return -ENOMEM;
+
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0) {
+		log_err(&ml, "cmd submission failed: %s\n", strerror(-rc));
+		return rc;
+	}
+
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0) {
+		log_err(&ml, "%s: mbox status: %d\n", __func__, rc);
+		return -ENXIO;
+	}
+
+	rc = cxl_cmd_identify_get_fw_rev(cmd, fw_rev, 0x10);
+	if (rc != 0) {
+		log_err(&ml, "%s: can't get firmware revision\n", devname);
+	}
+
+	bytes_to_str(cxl_cmd_identify_get_total_size(cmd), total_cap,
+		     sizeof(total_cap));
+	bytes_to_str(cxl_cmd_identify_get_volatile_only_size(cmd),
+		     volatile_only, sizeof(volatile_only));
+	bytes_to_str(cxl_cmd_identify_get_persistent_only_size(cmd),
+		     persistent_only, sizeof(persistent_only));
+	bytes_to_str(cxl_cmd_identify_get_partition_align(cmd), alignment,
+		     sizeof(alignment));
+	info_log_size =
+		cxl_cmd_identify_get_event_log_size(cmd, CXL_IDENTIFY_INFO);
+	warn_log_size =
+		cxl_cmd_identify_get_event_log_size(cmd, CXL_IDENTIFY_WARN);
+	fail_log_size =
+		cxl_cmd_identify_get_event_log_size(cmd, CXL_IDENTIFY_FAIL);
+	fatal_log_size =
+		cxl_cmd_identify_get_event_log_size(cmd, CXL_IDENTIFY_FATAL);
+	bytes_to_str(cxl_cmd_identify_get_label_size(cmd), lsa_size,
+		     sizeof(lsa_size));
+	poison_list_max = cxl_cmd_identify_get_poison_list_max(cmd);
+	inject_poison_limit = cxl_cmd_identify_get_inject_poison_limit(cmd);
+	inject_persistent_poison =
+		cxl_cmd_identify_injects_persistent_poison(cmd);
+	scan_poison = cxl_cmd_identify_scans_for_poison(cmd);
+	egress_port_congestion = cxl_cmd_identify_egress_port_congestion(cmd);
+	temp_throughput_reduction =
+		cxl_cmd_identify_temporary_throughput_reduction(cmd);
+
+	printf("CXL Identify Memory Device \"%s\"\n", devname);
+	printf("FW Revision                              : %s\n", fw_rev);
+	printf("Total Capacity                           : %s\n", total_cap);
+	printf("Volatile Only Capacity                   : %s\n",
+	       volatile_only);
+	printf("Persistent Only Capacity                 : %s\n",
+	       persistent_only);
+	printf("Partition Alignment                      : %s\n", alignment);
+	printf("Informational Event Log Size             : %d\n",
+	       info_log_size);
+	printf("Warning Event Log Size                   : %d\n",
+	       warn_log_size);
+	printf("Failure Event Log Size                   : %d\n",
+	       fail_log_size);
+	printf("Fatal Event Log Size                     : %d\n",
+	       fatal_log_size);
+	printf("LSA Size                                 : %s\n", lsa_size);
+	printf("Poison List Maximum Media Error Records  : %d\n",
+	       poison_list_max);
+	printf("Inject Poison Limit                      : %d\n",
+	       inject_poison_limit);
+	printf("Poison Handling Capabilities\n");
+	printf("Injects Persistent Poison                : ");
+	if (inject_persistent_poison)
+		printf("Supported\n");
+	else
+		printf("Not Supported\n");
+	printf("Scans for Poison                         : ");
+	if (scan_poison)
+		printf("Supported\n");
+	else
+		printf("Not Supported\n");
+	printf("QoS Telemetry Capabilities\n");
+	printf("Egress Port Congestion                   : ");
+	if (egress_port_congestion)
+		printf("Supported\n");
+	else
+		printf("Not Supported\n");
+	printf("Temporary Throughput Reduction           : ");
+	if (temp_throughput_reduction)
+		printf("Supported\n");
+	else
+		printf("Not Supported\n");
+
+	return 0;
+}
+
 enum reserve_dpa_mode {
 	DPA_ALLOC,
 	DPA_FREE,
@@ -893,3 +1023,14 @@ int cmd_free_dpa(int argc, const char **argv, struct cxl_ctx *ctx)
 
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_identify(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	int count = memdev_action(
+		argc, argv, ctx, action_identify, identify_options,
+		"cxl identify <mem0> [<mem1>..<memn>] [<options>]");
+	log_info(&ml, "identified %d mem%s\n", count >= 0 ? count : 0,
+		 count > 1 ? "s" : "");
+
+	return count >= 0 ? 0 : EXIT_FAILURE;
+}
-- 
2.17.1

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Alison Schofield]

On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> From: jehoon park <jehoon.park@samsung.com>
> 
> This patchset supports CXL IDENTIFY mailbox command and corresponding
> cxl tool interface command.
> 
> CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> information about CXL memory device. The information consist of device's
> firmware version, capacity, LSA size, event log size, poison list size,
> inject poison limit, poison handling capabilities and QoS telemetry
> capabilities. Firmware version, capacity and LSA size are already supported
> and used for partition commands or sysfs attributes while others are not.
> Since patches about event log [1] and poison [2] are discussed recently,
> support for those information will be helpful.

Hi Jehoon,

Does this need to be a separate command? Identify fields can be included
in cxl list options. For example, the -I option to cxl list, issues the
identify command and includes the partition related entries in that json
output.

There are other identify fields that need to be picked up, like the
poison related fields.  They need to be added to the cxl list 
options. We may want to include some when we list the poison, and
some as an option in the memdev listing.

Is there some reasoning behind separating this out? If not, can we look
to add the missing fields to the various cxl-list options and add
new cxl-list options where needed?

Alison

> 
> Example:

Users expect json formatted output here.


> # cxl identify mem0
> FW Revision                              : BWFW VERSION 00
> Total Capacity                           : 1.00 GB
> Volatile Only Capacity                   : 1.00 GB
> Persistent Only Capacity                 : 0 B
> Partition Alignment                      : 0 B
> Informational Event Log Size             : 0
> Warning Event Log Size                   : 0
> Failure Event Log Size                   : 0
> Fatal Event Log Size                     : 0
> LSA Size                                 : 0 B
> Poison List Maximum Media Error Records  : 256
> Inject Poison Limit                      : 0
> Poison Handling Capabilities
> Injects Persistent Poison                : Not Supported
> Scans for Poison                         : Not Supported
> QoS Telemetry Capabilities
> Egress Port Congestion                   : Not Supported
> Temporary Throughput Reduction           : Not Supported
> cxl memdev: cmd_identify: identified 1 mem
> 
> This patch is RFC because some of the information are already provided by
> "list -m <memdev>" command and sysfs attributes.
> I think a separate cxl tool interface command for identify is useful to provide
> device's information clearly. In case of nvme-cli, there are separate
> interface commands for identifying controller and namespace: id-ctrl and id-ns.
> 
> [1] https://lore.kernel.org/linux-cxl/20221216-cxl-ev-log-v7-0-2316a5c8f7d8@intel.com/
> [2] https://lore.kernel.org/linux-cxl/cover.1676685180.git.alison.schofield@intel.com/
> 
> jehoon park (2):
>   libcxl: add accessors for IDENTIFY command
>   cxl: add identify command to cxl tool
> 
>  Documentation/cxl/cxl-identify.txt |  57 ++++++++++++
>  Documentation/cxl/meson.build      |   1 +
>  cxl/builtin.h                      |   1 +
>  cxl/cxl.c                          |   1 +
>  cxl/lib/libcxl.c                   |  73 +++++++++++++++
>  cxl/lib/libcxl.sym                 |  11 +++
>  cxl/lib/private.h                  |  11 +++
>  cxl/libcxl.h                       |  16 ++++
>  cxl/memdev.c                       | 141 +++++++++++++++++++++++++++++
>  9 files changed, 312 insertions(+)
>  create mode 100644 Documentation/cxl/cxl-identify.txt
> 
> 
> base-commit: b830c4af984e72e5849c0705669aad2ffa19db13
> -- 
> 2.17.1
> 

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Jehoon Park]

[-- Attachment #1: Type: text/plain, Size: 5225 bytes --]

On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > From: jehoon park <jehoon.park@samsung.com>
> > 
> > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > cxl tool interface command.
> > 
> > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > information about CXL memory device. The information consist of device's
> > firmware version, capacity, LSA size, event log size, poison list size,
> > inject poison limit, poison handling capabilities and QoS telemetry
> > capabilities. Firmware version, capacity and LSA size are already supported
> > and used for partition commands or sysfs attributes while others are not.
> > Since patches about event log [1] and poison [2] are discussed recently,
> > support for those information will be helpful.
> 
> Hi Jehoon,
> 
> Does this need to be a separate command? Identify fields can be included
> in cxl list options. For example, the -I option to cxl list, issues the
> identify command and includes the partition related entries in that json
> output.
> 
> There are other identify fields that need to be picked up, like the
> poison related fields.  They need to be added to the cxl list 
> options. We may want to include some when we list the poison, and
> some as an option in the memdev listing.
> 
> Is there some reasoning behind separating this out? If not, can we look
> to add the missing fields to the various cxl-list options and add
> new cxl-list options where needed?
> 
> Alison
> 

Hi Alison, thank you for comments.

I suggested separate identify command since it retrieves basic information
about memdev. Since cxl-list command lists all cxl objects, I intended to
focus memdev information by separating it. Also, I referred to nvme-cli
which has id-ctrl and id-ns commands.

However, as you commented, some fields were already included in cxl-list.
I think the idea that providing information to proper listing option also
makes sense.

Then, by following the approach, including fields to cxl-list options,
identify fields could be included like below. Do they look fine?

1. FW version and LSA size are included when listing memdev. ("list -m memdev")
2. For poison related fields (poison_list_max size and inject_poison_limit),
include them when listing poison. ("--media-errors" option, patch [1])
3. For capabilities fields, add new option "-C, --capabilities" to the
memdev listing. (I see there exists same option for listing nvdimm device)

However, I'm confused about event_log_size fields. Could they be included
in capabilities option too? or require new option like "--event"?

Jehoon

[1] https://lore.kernel.org/nvdimm/cover.1668133294.git.alison.schofield@intel.com/

> > 
> > Example:
> 
> Users expect json formatted output here.
> 
> 
> > # cxl identify mem0
> > FW Revision                              : BWFW VERSION 00
> > Total Capacity                           : 1.00 GB
> > Volatile Only Capacity                   : 1.00 GB
> > Persistent Only Capacity                 : 0 B
> > Partition Alignment                      : 0 B
> > Informational Event Log Size             : 0
> > Warning Event Log Size                   : 0
> > Failure Event Log Size                   : 0
> > Fatal Event Log Size                     : 0
> > LSA Size                                 : 0 B
> > Poison List Maximum Media Error Records  : 256
> > Inject Poison Limit                      : 0
> > Poison Handling Capabilities
> > Injects Persistent Poison                : Not Supported
> > Scans for Poison                         : Not Supported
> > QoS Telemetry Capabilities
> > Egress Port Congestion                   : Not Supported
> > Temporary Throughput Reduction           : Not Supported
> > cxl memdev: cmd_identify: identified 1 mem
> > 
> > This patch is RFC because some of the information are already provided by
> > "list -m <memdev>" command and sysfs attributes.
> > I think a separate cxl tool interface command for identify is useful to provide
> > device's information clearly. In case of nvme-cli, there are separate
> > interface commands for identifying controller and namespace: id-ctrl and id-ns.
> > 
> > [1] https://lore.kernel.org/linux-cxl/20221216-cxl-ev-log-v7-0-2316a5c8f7d8@intel.com/
> > [2] https://lore.kernel.org/linux-cxl/cover.1676685180.git.alison.schofield@intel.com/
> > 
> > jehoon park (2):
> >   libcxl: add accessors for IDENTIFY command
> >   cxl: add identify command to cxl tool
> > 
> >  Documentation/cxl/cxl-identify.txt |  57 ++++++++++++
> >  Documentation/cxl/meson.build      |   1 +
> >  cxl/builtin.h                      |   1 +
> >  cxl/cxl.c                          |   1 +
> >  cxl/lib/libcxl.c                   |  73 +++++++++++++++
> >  cxl/lib/libcxl.sym                 |  11 +++
> >  cxl/lib/private.h                  |  11 +++
> >  cxl/libcxl.h                       |  16 ++++
> >  cxl/memdev.c                       | 141 +++++++++++++++++++++++++++++
> >  9 files changed, 312 insertions(+)
> >  create mode 100644 Documentation/cxl/cxl-identify.txt
> > 
> > 
> > base-commit: b830c4af984e72e5849c0705669aad2ffa19db13
> > -- 
> > 2.17.1
> > 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Ira Weiny]

Jehoon Park wrote:
> On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> > On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > > From: jehoon park <jehoon.park@samsung.com>
> > > 
> > > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > > cxl tool interface command.
> > > 
> > > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > > information about CXL memory device. The information consist of device's
> > > firmware version, capacity, LSA size, event log size, poison list size,
> > > inject poison limit, poison handling capabilities and QoS telemetry
> > > capabilities. Firmware version, capacity and LSA size are already supported
> > > and used for partition commands or sysfs attributes while others are not.
> > > Since patches about event log [1] and poison [2] are discussed recently,
> > > support for those information will be helpful.
> > 
> > Hi Jehoon,
> > 
> > Does this need to be a separate command? Identify fields can be included
> > in cxl list options. For example, the -I option to cxl list, issues the
> > identify command and includes the partition related entries in that json
> > output.
> > 
> > There are other identify fields that need to be picked up, like the
> > poison related fields.  They need to be added to the cxl list 
> > options. We may want to include some when we list the poison, and
> > some as an option in the memdev listing.
> > 
> > Is there some reasoning behind separating this out? If not, can we look
> > to add the missing fields to the various cxl-list options and add
> > new cxl-list options where needed?
> > 
> > Alison
> > 
> 
> Hi Alison, thank you for comments.
> 
> I suggested separate identify command since it retrieves basic information
> about memdev. Since cxl-list command lists all cxl objects, I intended to
> focus memdev information by separating it. Also, I referred to nvme-cli
> which has id-ctrl and id-ns commands.
> 
> However, as you commented, some fields were already included in cxl-list.
> I think the idea that providing information to proper listing option also
> makes sense.
> 
> Then, by following the approach, including fields to cxl-list options,
> identify fields could be included like below. Do they look fine?
> 
> 1. FW version and LSA size are included when listing memdev. ("list -m memdev")
> 2. For poison related fields (poison_list_max size and inject_poison_limit),
> include them when listing poison. ("--media-errors" option, patch [1])
> 3. For capabilities fields, add new option "-C, --capabilities" to the
> memdev listing. (I see there exists same option for listing nvdimm device)
> 
> However, I'm confused about event_log_size fields. Could they be included
> in capabilities option too? or require new option like "--event"?

Fundamentally why does user space need to know the event log sizes?

I do like the idea of getting the 'raw' results of the identify command in
it's entirety.

What if list has an '--identify' option which adds the list of Identify
values as a child json object.

Ira

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Alison Schofield]

On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote:
> Jehoon Park wrote:
> > On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> > > On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > > > From: jehoon park <jehoon.park@samsung.com>
> > > > 
> > > > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > > > cxl tool interface command.
> > > > 
> > > > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > > > information about CXL memory device. The information consist of device's
> > > > firmware version, capacity, LSA size, event log size, poison list size,
> > > > inject poison limit, poison handling capabilities and QoS telemetry
> > > > capabilities. Firmware version, capacity and LSA size are already supported
> > > > and used for partition commands or sysfs attributes while others are not.
> > > > Since patches about event log [1] and poison [2] are discussed recently,
> > > > support for those information will be helpful.
> > > 
> > > Hi Jehoon,
> > > 
> > > Does this need to be a separate command? Identify fields can be included
> > > in cxl list options. For example, the -I option to cxl list, issues the
> > > identify command and includes the partition related entries in that json
> > > output.
> > > 
> > > There are other identify fields that need to be picked up, like the
> > > poison related fields.  They need to be added to the cxl list 
> > > options. We may want to include some when we list the poison, and
> > > some as an option in the memdev listing.
> > > 
> > > Is there some reasoning behind separating this out? If not, can we look
> > > to add the missing fields to the various cxl-list options and add
> > > new cxl-list options where needed?
> > > 
> > > Alison
> > > 
> > 
> > Hi Alison, thank you for comments.
> > 
> > I suggested separate identify command since it retrieves basic information
> > about memdev. Since cxl-list command lists all cxl objects, I intended to
> > focus memdev information by separating it. Also, I referred to nvme-cli
> > which has id-ctrl and id-ns commands.
> > 
> > However, as you commented, some fields were already included in cxl-list.
> > I think the idea that providing information to proper listing option also
> > makes sense.
> > 
> > Then, by following the approach, including fields to cxl-list options,
> > identify fields could be included like below. Do they look fine?
> > 
> > 1. FW version and LSA size are included when listing memdev. ("list -m memdev")
> > 2. For poison related fields (poison_list_max size and inject_poison_limit),
> > include them when listing poison. ("--media-errors" option, patch [1])
> > 3. For capabilities fields, add new option "-C, --capabilities" to the
> > memdev listing. (I see there exists same option for listing nvdimm device)
> > 
> > However, I'm confused about event_log_size fields. Could they be included
> > in capabilities option too? or require new option like "--event"?
> 
> Fundamentally why does user space need to know the event log sizes?
> 
> I do like the idea of getting the 'raw' results of the identify command in
> it's entirety.
> 
> What if list has an '--identify' option which adds the list of Identify
> values as a child json object.
>
That's a good way to add it. That'll give you all the fields in one
place, and then, we can still think about if we want to spit out
specific fields (ie poison related) when we are doing a poison command.
You will have added all the libcxl accessors, making those easier to add.
Alison


> Ira

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Jehoon Park]

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

On Wed, Mar 08, 2023 at 01:58:11PM -0800, Alison Schofield wrote:
> On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote:
> > Jehoon Park wrote:
> > > On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> > > > On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > > > > From: jehoon park <jehoon.park@samsung.com>
> > > > > 
> > > > > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > > > > cxl tool interface command.
> > > > > 
> > > > > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > > > > information about CXL memory device. The information consist of device's
> > > > > firmware version, capacity, LSA size, event log size, poison list size,
> > > > > inject poison limit, poison handling capabilities and QoS telemetry
> > > > > capabilities. Firmware version, capacity and LSA size are already supported
> > > > > and used for partition commands or sysfs attributes while others are not.
> > > > > Since patches about event log [1] and poison [2] are discussed recently,
> > > > > support for those information will be helpful.
> > > > 
> > > > Hi Jehoon,
> > > > 
> > > > Does this need to be a separate command? Identify fields can be included
> > > > in cxl list options. For example, the -I option to cxl list, issues the
> > > > identify command and includes the partition related entries in that json
> > > > output.
> > > > 
> > > > There are other identify fields that need to be picked up, like the
> > > > poison related fields.  They need to be added to the cxl list 
> > > > options. We may want to include some when we list the poison, and
> > > > some as an option in the memdev listing.
> > > > 
> > > > Is there some reasoning behind separating this out? If not, can we look
> > > > to add the missing fields to the various cxl-list options and add
> > > > new cxl-list options where needed?
> > > > 
> > > > Alison
> > > > 
> > > 
> > > Hi Alison, thank you for comments.
> > > 
> > > I suggested separate identify command since it retrieves basic information
> > > about memdev. Since cxl-list command lists all cxl objects, I intended to
> > > focus memdev information by separating it. Also, I referred to nvme-cli
> > > which has id-ctrl and id-ns commands.
> > > 
> > > However, as you commented, some fields were already included in cxl-list.
> > > I think the idea that providing information to proper listing option also
> > > makes sense.
> > > 
> > > Then, by following the approach, including fields to cxl-list options,
> > > identify fields could be included like below. Do they look fine?
> > > 
> > > 1. FW version and LSA size are included when listing memdev. ("list -m memdev")
> > > 2. For poison related fields (poison_list_max size and inject_poison_limit),
> > > include them when listing poison. ("--media-errors" option, patch [1])
> > > 3. For capabilities fields, add new option "-C, --capabilities" to the
> > > memdev listing. (I see there exists same option for listing nvdimm device)
> > > 
> > > However, I'm confused about event_log_size fields. Could they be included
> > > in capabilities option too? or require new option like "--event"?
> > 
> > Fundamentally why does user space need to know the event log sizes?
> > 
> > I do like the idea of getting the 'raw' results of the identify command in
> > it's entirety.
> > 
> > What if list has an '--identify' option which adds the list of Identify
> > values as a child json object.
> >
> That's a good way to add it. That'll give you all the fields in one
> place, and then, we can still think about if we want to spit out
> specific fields (ie poison related) when we are doing a poison command.
> You will have added all the libcxl accessors, making those easier to add.
> Alison
> 
> 
> > Ira

The idea adding new list option “—identify” to display raw data from Identify
looks good! Providing proper fields to other options will be helpful for users,
however, I think it may be covered by different patchset after basic support
for Identify command is fully handled.

I will revise this patchset by applying your valuable comments. Thank you.

Jehoon

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Dan Williams]

Jehoon Park wrote:
> On Wed, Mar 08, 2023 at 01:58:11PM -0800, Alison Schofield wrote:
> > On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote:
> > > Jehoon Park wrote:
> > > > On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> > > > > On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > > > > > From: jehoon park <jehoon.park@samsung.com>
> > > > > > 
> > > > > > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > > > > > cxl tool interface command.
> > > > > > 
> > > > > > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > > > > > information about CXL memory device. The information consist of device's
> > > > > > firmware version, capacity, LSA size, event log size, poison list size,
> > > > > > inject poison limit, poison handling capabilities and QoS telemetry
> > > > > > capabilities. Firmware version, capacity and LSA size are already supported
> > > > > > and used for partition commands or sysfs attributes while others are not.
> > > > > > Since patches about event log [1] and poison [2] are discussed recently,
> > > > > > support for those information will be helpful.
> > > > > 
> > > > > Hi Jehoon,
> > > > > 
> > > > > Does this need to be a separate command? Identify fields can be included
> > > > > in cxl list options. For example, the -I option to cxl list, issues the
> > > > > identify command and includes the partition related entries in that json
> > > > > output.
> > > > > 
> > > > > There are other identify fields that need to be picked up, like the
> > > > > poison related fields.  They need to be added to the cxl list 
> > > > > options. We may want to include some when we list the poison, and
> > > > > some as an option in the memdev listing.
> > > > > 
> > > > > Is there some reasoning behind separating this out? If not, can we look
> > > > > to add the missing fields to the various cxl-list options and add
> > > > > new cxl-list options where needed?
> > > > > 
> > > > > Alison
> > > > > 
> > > > 
> > > > Hi Alison, thank you for comments.
> > > > 
> > > > I suggested separate identify command since it retrieves basic information
> > > > about memdev. Since cxl-list command lists all cxl objects, I intended to
> > > > focus memdev information by separating it. Also, I referred to nvme-cli
> > > > which has id-ctrl and id-ns commands.
> > > > 
> > > > However, as you commented, some fields were already included in cxl-list.
> > > > I think the idea that providing information to proper listing option also
> > > > makes sense.
> > > > 
> > > > Then, by following the approach, including fields to cxl-list options,
> > > > identify fields could be included like below. Do they look fine?
> > > > 
> > > > 1. FW version and LSA size are included when listing memdev. ("list -m memdev")
> > > > 2. For poison related fields (poison_list_max size and inject_poison_limit),
> > > > include them when listing poison. ("--media-errors" option, patch [1])
> > > > 3. For capabilities fields, add new option "-C, --capabilities" to the
> > > > memdev listing. (I see there exists same option for listing nvdimm device)
> > > > 
> > > > However, I'm confused about event_log_size fields. Could they be included
> > > > in capabilities option too? or require new option like "--event"?
> > > 
> > > Fundamentally why does user space need to know the event log sizes?
> > > 
> > > I do like the idea of getting the 'raw' results of the identify command in
> > > it's entirety.
> > > 
> > > What if list has an '--identify' option which adds the list of Identify
> > > values as a child json object.
> > >
> > That's a good way to add it. That'll give you all the fields in one
> > place, and then, we can still think about if we want to spit out
> > specific fields (ie poison related) when we are doing a poison command.
> > You will have added all the libcxl accessors, making those easier to add.
> > Alison
> > 
> > 
> > > Ira
> 
> The idea adding new list option “—identify” to display raw data from Identify
> looks good! Providing proper fields to other options will be helpful for users,
> however, I think it may be covered by different patchset after basic support
> for Identify command is fully handled.
> 
> I will revise this patchset by applying your valuable comments. Thank you.

I would prefer that users not need to know about a new --identify
option, just include all the fields you would emit in that scenario in
"cxl list -M" by default. Firmware version makes sense to add, but I do
not see the end user value of emitting the Event Log Size, for example.
I am just asking to be thoughtful about not including inactionable
information in the output, as most of what identify reports is already
available via "cxl list -MI".