[ndctl PATCH 0/7] Add partitioning support for CXL memdevs

[ndctl PATCH 0/7] Add partitioning support for CXL memdevs

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

To support changing partitions on CXL memdevs, first provide access
to device partitioning info. The first 4 patches add accessors to all
the partition info a CXL command parser needs in order to validate
the command. This info is added to cxl list to assist the user in
creating valid partition requests.

# cxl list -MP
[
  {
    "memdev":"mem0",
    "pmem_size":0,
    "ram_size":273535729664,
    "partition":{
      "active_volatile_capacity":273535729664,
      "active_persistent_capacity":0,
      "next_volatile_capacity":268435456,
      "next_persistent_capacity":273267294208,
      "total_capacity":273535729664,
      "volatile_only_capacity":0,
      "persistent_only_capacity":0,
      "partition_alignment":268435456
    }
  }
]

Next introduce libcxl ioctl() interfaces for the SET_PARTITION_INFO
mailbox command and the new CXL command. cxl-cli does the constraints
checking. It does not offer the IMMEDIATE mode option since we do not
have driver support for that yet.

# cxl set-partition-info
 usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]

    -v, --verbose         turn on debug
    -s, --volatile_size <n>
                          next volatile partition size in bytes

Guessing that a libcxl user could send the SET_PARTITION_INFO mailbox
command outside of cxl-cli tool, so a kernel patch that disables the
immediate bit, on the receiving end of the ioctl, follows.

It may be simpler to block the immediate bit in the libcxl API today,
(and as I write this cover letter I'm wondering just how far this goes
astray ;)) However, the kernel patch to peek in the payload sets us on
the path of inspecting set-partition-info mailbox commands in the future,
when immediate mode support is required.

Testing - so far I've only tested w one memdev in a Simics env. So,
next will be growing that Simics config, using cxl_test env, and 
adding a unit test.

Alison Schofield (7):
  libcxl: add GET_PARTITION_INFO mailbox command and accessors
  libcxl: add accessors for capacity fields of the IDENTIFY command
  libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field
  cxl: add memdev partition information to cxl-list
  libcxl: add interfaces for SET_PARTITION_INFO mailbox command
  ndctl, util: use 'unsigned long long' type in OPT_U64 define
  cxl: add command set-partition-info

 Documentation/cxl/cxl-list.txt               |  23 ++++
 Documentation/cxl/cxl-set-partition-info.txt |  27 +++++
 Documentation/cxl/partition-description.txt  |  15 +++
 Documentation/cxl/partition-options.txt      |  19 +++
 Documentation/cxl/Makefile.am                |   3 +-
 cxl/builtin.h                                |   1 +
 cxl/lib/private.h                            |  19 +++
 cxl/libcxl.h                                 |  12 ++
 util/json.h                                  |   1 +
 util/parse-options.h                         |   2 +-
 util/size.h                                  |   1 +
 cxl/cxl.c                                    |   1 +
 cxl/lib/libcxl.c                             | 117 ++++++++++++++++++-
 cxl/lib/libcxl.sym                           |  11 ++
 cxl/list.c                                   |   5 +
 cxl/memdev.c                                 |  89 ++++++++++++++
 util/json.c                                  | 112 ++++++++++++++++++
 17 files changed, 455 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
 create mode 100644 Documentation/cxl/partition-description.txt
 create mode 100644 Documentation/cxl/partition-options.txt

-- 
2.31.1

[ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
command output data structure (privately), and accessor APIs to return
the different fields in the partition info output.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/lib/private.h  | 10 ++++++++++
 cxl/libcxl.h       |  5 +++++
 util/size.h        |  1 +
 cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 5 files changed, 62 insertions(+)

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index a1b8b50..dd9234f 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -7,6 +7,7 @@
 #include <cxl/cxl_mem.h>
 #include <ccan/endian/endian.h>
 #include <ccan/short_types/short_types.h>
+#include <util/size.h>
 
 #define CXL_EXPORT __attribute__ ((visibility("default")))
 
@@ -104,6 +105,15 @@ struct cxl_cmd_get_health_info {
 	le32 pmem_errors;
 } __attribute__((packed));
 
+struct cxl_cmd_get_partition_info {
+	le64 active_volatile_cap;
+	le64 active_persistent_cap;
+	le64 next_volatile_cap;
+	le64 next_persistent_cap;
+} __attribute__((packed));
+
+#define CXL_CAPACITY_MULTIPLIER		SZ_256M
+
 /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
 #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
 #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 89d35ba..7cf9061 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
 		unsigned int length);
 struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
 		void *buf, unsigned int offset, unsigned int length);
+struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
+unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/util/size.h b/util/size.h
index a0f3593..e72467f 100644
--- a/util/size.h
+++ b/util/size.h
@@ -15,6 +15,7 @@
 #define SZ_4M     0x00400000
 #define SZ_16M    0x01000000
 #define SZ_64M    0x04000000
+#define SZ_256M	  0x10000000
 #define SZ_1G     0x40000000
 #define SZ_1T 0x10000000000ULL
 
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index f0664be..f3d4022 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1157,6 +1157,47 @@ CXL_EXPORT ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd,
 	return length;
 }
 
+CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev)
+{
+	return cxl_cmd_new_generic(memdev,
+				   CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
+}
+
+#define cmd_partition_get_capacity_field(cmd, field)			\
+do {										\
+	struct cxl_cmd_get_partition_info *c =					\
+		(struct cxl_cmd_get_partition_info *)cmd->send_cmd->out.payload;\
+	int rc = cxl_cmd_validate_status(cmd,					\
+			CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);			\
+	if (rc)									\
+		return ULLONG_MAX;							\
+	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
+} while (0)
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_persistent_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, next_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
+}
+
 CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
 {
 	struct cxl_memdev *memdev = cmd->memdev;
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 077d104..09d6d94 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -70,6 +70,11 @@ global:
 	cxl_memdev_zero_label;
 	cxl_memdev_write_label;
 	cxl_memdev_read_label;
+	cxl_cmd_new_get_partition_info;
+	cxl_cmd_get_partition_info_get_active_volatile_cap;
+	cxl_cmd_get_partition_info_get_active_persistent_cap;
+	cxl_cmd_get_partition_info_get_next_volatile_cap;
+	cxl_cmd_get_partition_info_get_next_persistent_cap;
 local:
         *;
 };
-- 
2.31.1

[ndctl PATCH 2/7] libcxl: add accessors for capacity fields of the IDENTIFY command

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add accessors to retrieve total capacity, volatile only capacity,
and persistent only capacity from the IDENTIFY mailbox command.
These values are useful when partitioning the device.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/libcxl.h       |  3 +++
 cxl/lib/libcxl.c   | 29 +++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  3 +++
 3 files changed, 35 insertions(+)

diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 7cf9061..d333b6d 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -68,6 +68,9 @@ int cxl_cmd_get_mbox_status(struct cxl_cmd *cmd);
 int cxl_cmd_get_out_size(struct cxl_cmd *cmd);
 struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev);
 int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev, int fw_len);
+unsigned long long cxl_cmd_identify_get_total_capacity(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_volatile_only_capacity(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_persistent_only_capacity(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);
 struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index f3d4022..715d8e4 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1102,6 +1102,35 @@ CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
 	return le32_to_cpu(id->lsa_size);
 }
 
+#define cmd_identify_get_capacity_field(cmd, field)			\
+do {										\
+	struct cxl_cmd_identify *c =					\
+		(struct cxl_cmd_identify *)cmd->send_cmd->out.payload;\
+	int rc = cxl_cmd_validate_status(cmd,					\
+			CXL_MEM_COMMAND_ID_IDENTIFY);			\
+	if (rc)									\
+		return ULLONG_MAX;							\
+	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
+} while (0)
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_total_capacity(struct cxl_cmd *cmd)
+{
+	cmd_identify_get_capacity_field(cmd, total_capacity);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_volatile_only_capacity(struct cxl_cmd *cmd)
+{
+	cmd_identify_get_capacity_field(cmd, volatile_capacity);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_persistent_only_capacity(struct cxl_cmd *cmd)
+{
+	cmd_identify_get_capacity_field(cmd, persistent_capacity);
+}
+
 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 09d6d94..bed6427 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -31,6 +31,9 @@ global:
 	cxl_cmd_get_out_size;
 	cxl_cmd_new_identify;
 	cxl_cmd_identify_get_fw_rev;
+	cxl_cmd_identify_get_total_capacity;
+	cxl_cmd_identify_get_volatile_only_capacity;
+	cxl_cmd_identify_get_persistent_only_capacity;
 	cxl_cmd_identify_get_partition_align;
 	cxl_cmd_identify_get_label_size;
 	cxl_cmd_new_get_health_info;
-- 
2.31.1

[ndctl PATCH 3/7] libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The IDENTIFY mailbox command returns the partition alignment field
expressed in multiples of 256 MB. Use CXL_CAPACITY_MULTIPLIER when
returning this field.

This is in sync with all the other partitioning related fields using
the multiplier.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/lib/libcxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 715d8e4..85a6c0e 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1086,7 +1086,7 @@ CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
 	if (cmd->status < 0)
 		return cmd->status;
 
-	return le64_to_cpu(id->partition_align);
+	return le64_to_cpu(id->partition_align) * CXL_CAPACITY_MULTIPLIER;
 }
 
 CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
-- 
2.31.1

[ndctl PATCH 4/7] cxl: add memdev partition information to cxl-list

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add information useful for managing memdev partitions to cxl-list
output. Include all of the fields from GET_PARTITION_INFO and the
partitioning related fields from the IDENTIFY mailbox command.

    "partition":{
      "active_volatile_capacity":273535729664,
      "active_persistent_capacity":0,
      "next_volatile_capacity":0,
      "next_persistent_capacity":0,
      "total_capacity":273535729664,
      "volatile_only_capacity":0,
      "persistent_only_capacity":0,
      "partition_alignment":268435456
    }

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/cxl-list.txt |  23 +++++++
 util/json.h                    |   1 +
 cxl/list.c                     |   5 ++
 util/json.c                    | 112 +++++++++++++++++++++++++++++++++
 4 files changed, 141 insertions(+)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index c8d10fb..e65e944 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -85,6 +85,29 @@ OPTIONS
   }
 ]
 ----
+-P::
+--partition::
+	Include partition information in the memdev listing. Example listing:
+----
+# cxl list -m mem0 -P
+[
+  {
+    "memdev":"mem0",
+    "pmem_size":0,
+    "ram_size":273535729664,
+    "partition":{
+      "active_volatile_capacity":273535729664,
+      "active_persistent_capacity":0,
+      "next_volatile_capacity":0,
+      "next_persistent_capacity":0,
+      "total_capacity":273535729664,
+      "volatile_only_capacity":0,
+      "persistent_only_capacity":0,
+      "partition_alignment":268435456
+    }
+  }
+]
+----
 
 include::human-option.txt[]
 
diff --git a/util/json.h b/util/json.h
index ce575e6..76a8816 100644
--- a/util/json.h
+++ b/util/json.h
@@ -20,6 +20,7 @@ enum util_json_flags {
 	UTIL_JSON_FIRMWARE	= (1 << 8),
 	UTIL_JSON_DAX_MAPPINGS	= (1 << 9),
 	UTIL_JSON_HEALTH	= (1 << 10),
+	UTIL_JSON_PARTITION	= (1 << 11),
 };
 
 struct json_object;
diff --git a/cxl/list.c b/cxl/list.c
index b1468b7..368ec21 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -17,6 +17,7 @@ static struct {
 	bool idle;
 	bool human;
 	bool health;
+	bool partition;
 } list;
 
 static unsigned long listopts_to_flags(void)
@@ -29,6 +30,8 @@ static unsigned long listopts_to_flags(void)
 		flags |= UTIL_JSON_HUMAN;
 	if (list.health)
 		flags |= UTIL_JSON_HEALTH;
+	if (list.partition)
+		flags |= UTIL_JSON_PARTITION;
 	return flags;
 }
 
@@ -62,6 +65,8 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 				"use human friendly number formats "),
 		OPT_BOOLEAN('H', "health", &list.health,
 				"include memory device health information "),
+		OPT_BOOLEAN('P', "partition", &list.partition,
+				"include memory device partition information "),
 		OPT_END(),
 	};
 	const char * const u[] = {
diff --git a/util/json.c b/util/json.c
index f97cf07..4254dea 100644
--- a/util/json.c
+++ b/util/json.c
@@ -1616,6 +1616,113 @@ err_jobj:
 	return NULL;
 }
 
+/*
+ * Present complete view of memdev partition by presenting fields from
+ * both GET_PARTITION_INFO and IDENTIFY mailbox commands.
+ */
+static struct json_object *util_cxl_memdev_partition_to_json(struct cxl_memdev *memdev,
+		unsigned long flags)
+{
+	struct json_object *jobj = NULL;
+	struct json_object *jpart;
+	unsigned long long cap;
+	struct cxl_cmd *cmd;
+	int rc;
+
+	jpart = json_object_new_object();
+	if (!jpart)
+		return NULL;
+	if (!memdev)
+		goto err_jobj;
+
+	/* Retrieve partition info in GET_PARTITION_INFO mbox cmd */
+	cmd = cxl_cmd_new_get_partition_info(memdev);
+	if (!cmd)
+		goto err_jobj;
+
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0)
+		goto err_cmd;
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0)
+		goto err_cmd;
+
+	cap = cxl_cmd_get_partition_info_get_active_volatile_cap(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_volatile_capacity", jobj);
+	}
+	cap = cxl_cmd_get_partition_info_get_active_persistent_cap(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_persistent_capacity", jobj);
+	}
+	cap = cxl_cmd_get_partition_info_get_next_volatile_cap(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_volatile_capacity", jobj);
+	}
+	cap = cxl_cmd_get_partition_info_get_next_persistent_cap(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_persistent_capacity", jobj);
+	}
+	cxl_cmd_unref(cmd);
+
+	/* Retrieve partition info in the IDENTIFY mbox cmd */
+	cmd = cxl_cmd_new_identify(memdev);
+	if (!cmd)
+		goto err_jobj;
+
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0)
+		goto err_cmd;
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0)
+		goto err_cmd;
+
+	cap = cxl_cmd_identify_get_total_capacity(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart, "total_capacity", jobj);
+	}
+	cap = cxl_cmd_identify_get_volatile_only_capacity(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"volatile_only_capacity", jobj);
+	}
+	cap = cxl_cmd_identify_get_persistent_only_capacity(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"persistent_only_capacity", jobj);
+	}
+	cap = cxl_cmd_identify_get_partition_align(cmd);
+	jobj = util_json_object_size(cap, flags);
+	if (jobj)
+		json_object_object_add(jpart, "partition_alignment", jobj);
+
+	return jpart;
+
+err_cmd:
+	cxl_cmd_unref(cmd);
+err_jobj:
+	json_object_put(jpart);
+	return NULL;
+}
+
 struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 		unsigned long flags)
 {
@@ -1643,5 +1750,10 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 		if (jobj)
 			json_object_object_add(jdev, "health", jobj);
 	}
+	if (flags & UTIL_JSON_PARTITION) {
+		jobj = util_cxl_memdev_partition_to_json(memdev, flags);
+		if (jobj)
+			json_object_object_add(jdev, "partition", jobj);
+	}
 	return jdev;
 }
-- 
2.31.1

[ndctl PATCH 5/7] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Add APIs to allocate and send a SET_PARTITION_INFO mailbox command.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/lib/private.h  |  9 +++++++++
 cxl/libcxl.h       |  4 ++++
 cxl/lib/libcxl.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  3 +++
 4 files changed, 61 insertions(+)

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index dd9234f..841aa80 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -114,6 +114,15 @@ struct cxl_cmd_get_partition_info {
 
 #define CXL_CAPACITY_MULTIPLIER		SZ_256M
 
+struct cxl_cmd_set_partition_info {
+	le64 volatile_capacity;
+	u8 flags;
+} __attribute__((packed));
+
+/* CXL 2.0 8.2.9.5.2 Set Partition Info */
+#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG				(0)
+#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG			(1)
+
 /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
 #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
 #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index d333b6d..67d6ffc 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -50,6 +50,8 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
+int cxl_memdev_set_partition_info(struct cxl_memdev *memdev,
+		unsigned long long volatile_capacity, int flags);
 
 #define cxl_memdev_foreach(ctx, memdev) \
         for (memdev = cxl_memdev_get_first(ctx); \
@@ -117,6 +119,8 @@ unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl
 unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
+struct cxl_cmd *cxl_cmd_new_set_partition_info(struct cxl_memdev *memdev,
+		unsigned long long volatile_capacity, int flags);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 85a6c0e..877a783 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1227,6 +1227,21 @@ cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
 	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
 }
 
+CXL_EXPORT struct cxl_cmd *cxl_cmd_new_set_partition_info(struct cxl_memdev *memdev,
+		unsigned long long volatile_capacity, int flags)
+{
+	struct cxl_cmd_set_partition_info *set_partition;
+	struct cxl_cmd *cmd;
+
+	cmd = cxl_cmd_new_generic(memdev,
+			CXL_MEM_COMMAND_ID_SET_PARTITION_INFO);
+
+	set_partition = (struct cxl_cmd_set_partition_info *)cmd->send_cmd->in.payload;
+	set_partition->volatile_capacity = cpu_to_le64(volatile_capacity);
+	set_partition->flags = flags;
+	return cmd;
+}
+
 CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
 {
 	struct cxl_memdev *memdev = cmd->memdev;
@@ -1425,3 +1440,33 @@ CXL_EXPORT int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf,
 {
 	return lsa_op(memdev, LSA_OP_GET, buf, length, offset);
 }
+
+CXL_EXPORT int cxl_memdev_set_partition_info(struct cxl_memdev *memdev,
+	       unsigned long long volatile_capacity, int flags)
+{
+	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
+	struct cxl_cmd *cmd;
+	int rc;
+
+	dbg(ctx, "%s: enter cap: %llx, flags %d\n", __func__,
+		volatile_capacity, flags);
+
+	cmd = cxl_cmd_new_set_partition_info(memdev,
+			volatile_capacity / CXL_CAPACITY_MULTIPLIER, flags);
+	if (!cmd)
+		return -ENXIO;
+
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0) {
+		err(ctx, "cmd submission failed: %s\n", strerror(-rc));
+		goto err;
+	}
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0) {
+		err(ctx, "%s: mbox status: %d\n", __func__, rc);
+		rc = -ENXIO;
+	}
+err:
+	cxl_cmd_unref(cmd);
+	return rc;
+}
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index bed6427..5d02c45 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -78,6 +78,9 @@ global:
 	cxl_cmd_get_partition_info_get_active_persistent_cap;
 	cxl_cmd_get_partition_info_get_next_volatile_cap;
 	cxl_cmd_get_partition_info_get_next_persistent_cap;
+	cxl_cmd_new_set_partition_info;
+	cxl_memdev_set_partition_info;
+
 local:
         *;
 };
-- 
2.31.1

[ndctl PATCH 6/7] ndctl, util: use 'unsigned long long' type in OPT_U64 define

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The OPT_U64 define failed in check_vtype() with unknown 'u64' type.
Replace with 'unsigned long long' to make the OPT_U64 define usable.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 util/parse-options.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/parse-options.h b/util/parse-options.h
index 9318fe7..91b7932 100644
--- a/util/parse-options.h
+++ b/util/parse-options.h
@@ -124,7 +124,7 @@ struct option {
 #define OPT_INTEGER(s, l, v, h)     { .type = OPTION_INTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h) }
 #define OPT_UINTEGER(s, l, v, h)    { .type = OPTION_UINTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned int *), .help = (h) }
 #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
-#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
+#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned long long *), .help = (h) }
 #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
 #define OPT_FILENAME(s, l, v, a, h) { .type = OPTION_FILENAME, .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
 #define OPT_DATE(s, l, v, h) \
-- 
2.31.1

[ndctl PATCH 7/7] cxl: add command set-partition-info

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The command 'cxl set-partition-info' operates on a CXL memdev,
or a set of memdevs, allowing the user to change the partition
layout of the device.

Synopsis:
Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]

    -v, --verbose         turn on debug
    -s, --volatile_size <n>
                          next volatile partition size in bytes

The MAN page explains how to find partitioning capabilities and
restrictions.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/cxl-set-partition-info.txt | 27 ++++++
 Documentation/cxl/partition-description.txt  | 15 ++++
 Documentation/cxl/partition-options.txt      | 19 +++++
 Documentation/cxl/Makefile.am                |  3 +-
 cxl/builtin.h                                |  1 +
 cxl/cxl.c                                    |  1 +
 cxl/memdev.c                                 | 89 ++++++++++++++++++++
 7 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
 create mode 100644 Documentation/cxl/partition-description.txt
 create mode 100644 Documentation/cxl/partition-options.txt

diff --git a/Documentation/cxl/cxl-set-partition-info.txt b/Documentation/cxl/cxl-set-partition-info.txt
new file mode 100644
index 0000000..32418b6
--- /dev/null
+++ b/Documentation/cxl/cxl-set-partition-info.txt
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-set-partition-info(1)
+=========================
+
+NAME
+----
+cxl-set-partition-info - set the partitioning between volatile and persistent capacity on a CXL memdev
+
+SYNOPSIS
+--------
+[verse]
+'cxl set-partition-info <mem> [ [<mem1>..<memN>] [<options>]'
+
+include::partition-description.txt[]
+Partition the device on the next device reset using the volatile capacity
+requested. Using this command to change the size of the persistent capacity
+shall result in the loss of stored data.
+
+OPTIONS
+-------
+include::partition-options.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1],
+CXL-2.0 8.2.9.5.2
diff --git a/Documentation/cxl/partition-description.txt b/Documentation/cxl/partition-description.txt
new file mode 100644
index 0000000..b3efac8
--- /dev/null
+++ b/Documentation/cxl/partition-description.txt
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+
+DESCRIPTION
+-----------
+Partition the device into volatile and persistent capacity. The change
+in partitioning will become the “next” configuration, to become active
+on the next device reset.
+
+Use "cxl list -m <memdev> -P" to examine the partition capacities
+supported on a device. Paritionable capacity must exist on the
+device. A partition_alignment of zero means no partitionable
+capacity is available.
+
+Using this command to change the size of the persistent capacity shall
+result in the loss of data stored.
diff --git a/Documentation/cxl/partition-options.txt b/Documentation/cxl/partition-options.txt
new file mode 100644
index 0000000..84e49c9
--- /dev/null
+++ b/Documentation/cxl/partition-options.txt
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+<memory device(s)>::
+include::memdev-option.txt[]
+
+-s::
+--size=::
+	Size in bytes of the volatile partition requested.
+
+	Size must align to the devices partition_alignment.
+	Use 'cxl list -m <memdev> -P' to find partition_alignment.
+
+	Size must be less than or equal to the devices partitionable bytes.
+	(total_capacity - volatile_only_capacity - persistent_only_capacity)
+	Use 'cxl list -m <memdev> -P' to find *_capacity values.
+
+-v::
+	Turn on verbose debug messages in the library (if libcxl was built with
+	logging and debug enabled).
diff --git a/Documentation/cxl/Makefile.am b/Documentation/cxl/Makefile.am
index efabaa3..c5faf04 100644
--- a/Documentation/cxl/Makefile.am
+++ b/Documentation/cxl/Makefile.am
@@ -22,7 +22,8 @@ man1_MANS = \
 	cxl-list.1 \
 	cxl-read-labels.1 \
 	cxl-write-labels.1 \
-	cxl-zero-labels.1
+	cxl-zero-labels.1 \
+	cxl-set-partition-info.1
 
 EXTRA_DIST = $(man1_MANS)
 
diff --git a/cxl/builtin.h b/cxl/builtin.h
index 78eca6e..7f11f28 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -10,4 +10,5 @@ int cmd_read_labels(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_init_labels(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_check_labels(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index 4b1661d..3153cf0 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
 	{ "zero-labels", .c_fn = cmd_zero_labels },
 	{ "read-labels", .c_fn = cmd_read_labels },
 	{ "write-labels", .c_fn = cmd_write_labels },
+	{ "set-partition-info", .c_fn = cmd_set_partition_info },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 5ee38e5..fa63317 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -6,6 +6,7 @@
 #include <unistd.h>
 #include <limits.h>
 #include <util/log.h>
+#include <util/size.h>
 #include <util/filter.h>
 #include <cxl/libcxl.h>
 #include <util/parse-options.h>
@@ -23,6 +24,7 @@ static struct parameters {
 	unsigned len;
 	unsigned offset;
 	bool verbose;
+	unsigned long long volatile_size;
 } param;
 
 #define fail(fmt, ...) \
@@ -47,6 +49,10 @@ OPT_UINTEGER('s', "size", &param.len, "number of label bytes to operate"), \
 OPT_UINTEGER('O', "offset", &param.offset, \
 	"offset into the label area to start operation")
 
+#define SET_PARTITION_OPTIONS() \
+OPT_U64('s', "volatile_size",  &param.volatile_size, \
+	"next volatile partition size in bytes")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
 	LABEL_OPTIONS(),
@@ -67,6 +73,12 @@ static const struct option zero_options[] = {
 	OPT_END(),
 };
 
+static const struct option set_partition_options[] = {
+	BASE_OPTIONS(),
+	SET_PARTITION_OPTIONS(),
+	OPT_END(),
+};
+
 static int action_zero(struct cxl_memdev *memdev, struct action_context *actx)
 {
 	size_t size;
@@ -174,6 +186,73 @@ out:
 	return rc;
 }
 
+static int validate_partition(struct cxl_memdev *memdev,
+		unsigned long long volatile_request)
+{
+	unsigned long long total_cap, volatile_only, persistent_only;
+	unsigned long long partitionable_bytes, partition_align_bytes;
+	const char *devname = cxl_memdev_get_devname(memdev);
+	struct cxl_cmd *cmd;
+	int rc;
+
+	cmd = cxl_cmd_new_identify(memdev);
+	if (!cmd)
+		return -ENXIO;
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0)
+		goto err;
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0)
+		goto err;
+
+	partition_align_bytes = cxl_cmd_identify_get_partition_align(cmd);
+	if (partition_align_bytes == 0) {
+		fprintf(stderr, "%s: no partitionable capacity\n", devname);
+		rc = -EINVAL;
+		goto err;
+	}
+
+	total_cap = cxl_cmd_identify_get_total_capacity(cmd);
+	volatile_only = cxl_cmd_identify_get_volatile_only_capacity(cmd);
+	persistent_only = cxl_cmd_identify_get_persistent_only_capacity(cmd);
+
+	partitionable_bytes = total_cap - volatile_only - persistent_only;
+
+	if (volatile_request > partitionable_bytes) {
+		fprintf(stderr, "%s: volatile size %lld exceeds partitionable capacity %lld\n",
+			devname, volatile_request, partitionable_bytes);
+		rc = -EINVAL;
+		goto err;
+	}
+	if (!IS_ALIGNED(volatile_request, partition_align_bytes)) {
+		fprintf(stderr, "%s: volatile size %lld is not partition aligned %lld\n",
+			devname, volatile_request, partition_align_bytes);
+		rc = -EINVAL;
+	}
+err:
+	cxl_cmd_unref(cmd);
+	return rc;
+}
+
+static int action_set_partition(struct cxl_memdev *memdev,
+		struct action_context *actx)
+{
+	const char *devname = cxl_memdev_get_devname(memdev);
+	unsigned long long volatile_request;
+	int rc;
+
+	volatile_request = param.volatile_size;
+
+	rc = validate_partition(memdev, volatile_request);
+	if (rc)
+		return rc;
+
+	rc = cxl_memdev_set_partition_info(memdev, volatile_request, 0);
+	if (rc)
+		fprintf(stderr, "%s error: %s\n", devname, strerror(-rc));
+	return rc;
+}
+
 static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 		int (*action)(struct cxl_memdev *memdev, struct action_context *actx),
 		const struct option *options, const char *usage)
@@ -322,3 +401,13 @@ int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx)
 			count > 1 ? "s" : "");
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	int count = memdev_action(argc, argv, ctx, action_set_partition,
+			set_partition_options,
+			"cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]");
+	fprintf(stderr, "set_partition %d mem%s\n", count >= 0 ? count : 0,
+			count > 1 ? "s" : "");
+	return count >= 0 ? 0 : EXIT_FAILURE;
+}
-- 
2.31.1

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> command output data structure (privately), and accessor APIs to return
> the different fields in the partition info output.

I would rephrase this:

libcxl provides functions for C code to issue cxl mailbox commands as well as
parse the output returned.  Get partition info should be part of this API.

Add the libcxl get partition info mailbox support as well as an API to parse
the fields of the command returned.

All fields are specified in multiples of 256MB so also define a capacity
multiplier to convert the raw data into bytes.

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/private.h  | 10 ++++++++++
>  cxl/libcxl.h       |  5 +++++
>  util/size.h        |  1 +
>  cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  5 +++++
>  5 files changed, 62 insertions(+)
> 
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index a1b8b50..dd9234f 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -7,6 +7,7 @@
>  #include <cxl/cxl_mem.h>
>  #include <ccan/endian/endian.h>
>  #include <ccan/short_types/short_types.h>
> +#include <util/size.h>
>  
>  #define CXL_EXPORT __attribute__ ((visibility("default")))
>  
> @@ -104,6 +105,15 @@ struct cxl_cmd_get_health_info {
>  	le32 pmem_errors;
>  } __attribute__((packed));
>  
> +struct cxl_cmd_get_partition_info {
> +	le64 active_volatile_cap;
> +	le64 active_persistent_cap;
> +	le64 next_volatile_cap;
> +	le64 next_persistent_cap;
> +} __attribute__((packed));
> +
> +#define CXL_CAPACITY_MULTIPLIER		SZ_256M
> +
>  /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
>  #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
>  #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 89d35ba..7cf9061 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
>  		unsigned int length);
>  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
>  		void *buf, unsigned int offset, unsigned int length);
> +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);

why 'new'?  Why not:

cxl_cmd_get_partition_info()

?

> +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);

These are pretty long function names.  :-/

I also wonder if the conversion to bytes should be reflected in the function
name.  Because returning 'cap' might imply to someone they are getting the raw
data field.

Ira

>  
>  #ifdef __cplusplus
>  } /* extern "C" */
> diff --git a/util/size.h b/util/size.h
> index a0f3593..e72467f 100644
> --- a/util/size.h
> +++ b/util/size.h
> @@ -15,6 +15,7 @@
>  #define SZ_4M     0x00400000
>  #define SZ_16M    0x01000000
>  #define SZ_64M    0x04000000
> +#define SZ_256M	  0x10000000
>  #define SZ_1G     0x40000000
>  #define SZ_1T 0x10000000000ULL
>  
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index f0664be..f3d4022 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1157,6 +1157,47 @@ CXL_EXPORT ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd,
>  	return length;
>  }
>  
> +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev)
> +{
> +	return cxl_cmd_new_generic(memdev,
> +				   CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
> +}
> +
> +#define cmd_partition_get_capacity_field(cmd, field)			\
> +do {										\
> +	struct cxl_cmd_get_partition_info *c =					\
> +		(struct cxl_cmd_get_partition_info *)cmd->send_cmd->out.payload;\
> +	int rc = cxl_cmd_validate_status(cmd,					\
> +			CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);			\
> +	if (rc)									\
> +		return ULLONG_MAX;							\
> +	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
> +} while (0)
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, active_volatile_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, active_persistent_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, next_volatile_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
> +}
> +
>  CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
>  {
>  	struct cxl_memdev *memdev = cmd->memdev;
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 077d104..09d6d94 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -70,6 +70,11 @@ global:
>  	cxl_memdev_zero_label;
>  	cxl_memdev_write_label;
>  	cxl_memdev_read_label;
> +	cxl_cmd_new_get_partition_info;
> +	cxl_cmd_get_partition_info_get_active_volatile_cap;
> +	cxl_cmd_get_partition_info_get_active_persistent_cap;
> +	cxl_cmd_get_partition_info_get_next_volatile_cap;
> +	cxl_cmd_get_partition_info_get_next_persistent_cap;
>  local:
>          *;
>  };
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 0/7] Add partitioning support for CXL memdevs

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:11PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
 
First, thanks for taking this on!  :-D

> To support changing partitions on CXL memdevs, first provide access to device
> partitioning info.
>

How about:

Users will want to configure CXL memdevs on CXL devices which support
partitioning.  CXL provides get and set partition info mailbox command to do
this.

Add support to retrieve partition info and set partition info.

...

>
>
> The first 4 patches add accessors to all
> the partition info a CXL command parser needs in order to validate
> the command. This info is added to cxl list to assist the user in
> creating valid partition requests.

Great but what is a valid partition request?

> 
> # cxl list -MP
> [
>   {
>     "memdev":"mem0",
>     "pmem_size":0,
>     "ram_size":273535729664,
>     "partition":{
>       "active_volatile_capacity":273535729664,
>       "active_persistent_capacity":0,
>       "next_volatile_capacity":268435456,
>       "next_persistent_capacity":273267294208,
>       "total_capacity":273535729664,
>       "volatile_only_capacity":0,
>       "persistent_only_capacity":0,
>       "partition_alignment":268435456
>     }
>   }
> ]
> 
> Next introduce libcxl ioctl() interfaces for the SET_PARTITION_INFO
> mailbox command and the new CXL command. cxl-cli does the constraints
> checking. It does not offer the IMMEDIATE mode option since we do not
> have driver support for that yet.

How about something like 'cxl-cli restricts the use of the IMMEDIATE flag until
such time as the driver supports it'?

But we probably should just let the command through and rely on the driver to
do what it does...

> 
> # cxl set-partition-info
>  usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
> 
>     -v, --verbose         turn on debug
>     -s, --volatile_size <n>
>                           next volatile partition size in bytes
> 
> Guessing that
  ^^^^^^^^^^^^^
  Delete

'A libcxl user can...'

>
> a libcxl user could send the SET_PARTITION_INFO mailbox
> command outside of cxl-cli tool, so a kernel patch that disables the
> immediate bit, on the receiving end of the ioctl, follows.

cool!

> 
> It may be simpler to block the immediate bit in the libcxl API today,
> (and as I write this cover letter I'm wondering just how far this goes
> astray ;)) However, the kernel patch to peek in the payload sets us on
> the path of inspecting set-partition-info mailbox commands in the future,
> when immediate mode support is required.

I'd just delete this.  I think it is best to leave the kernel to the
enforcement and not complicate the user space.

Ira

> 
> Testing - so far I've only tested w one memdev in a Simics env. So,
> next will be growing that Simics config, using cxl_test env, and 
> adding a unit test.
> 
> Alison Schofield (7):
>   libcxl: add GET_PARTITION_INFO mailbox command and accessors
>   libcxl: add accessors for capacity fields of the IDENTIFY command
>   libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field
>   cxl: add memdev partition information to cxl-list
>   libcxl: add interfaces for SET_PARTITION_INFO mailbox command
>   ndctl, util: use 'unsigned long long' type in OPT_U64 define
>   cxl: add command set-partition-info
> 
>  Documentation/cxl/cxl-list.txt               |  23 ++++
>  Documentation/cxl/cxl-set-partition-info.txt |  27 +++++
>  Documentation/cxl/partition-description.txt  |  15 +++
>  Documentation/cxl/partition-options.txt      |  19 +++
>  Documentation/cxl/Makefile.am                |   3 +-
>  cxl/builtin.h                                |   1 +
>  cxl/lib/private.h                            |  19 +++
>  cxl/libcxl.h                                 |  12 ++
>  util/json.h                                  |   1 +
>  util/parse-options.h                         |   2 +-
>  util/size.h                                  |   1 +
>  cxl/cxl.c                                    |   1 +
>  cxl/lib/libcxl.c                             | 117 ++++++++++++++++++-
>  cxl/lib/libcxl.sym                           |  11 ++
>  cxl/list.c                                   |   5 +
>  cxl/memdev.c                                 |  89 ++++++++++++++
>  util/json.c                                  | 112 ++++++++++++++++++
>  17 files changed, 455 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
>  create mode 100644 Documentation/cxl/partition-description.txt
>  create mode 100644 Documentation/cxl/partition-options.txt
> 
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 2/7] libcxl: add accessors for capacity fields of the IDENTIFY command

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:13PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add accessors to retrieve total capacity, volatile only capacity,
> and persistent only capacity from the IDENTIFY mailbox command.
> These values are useful when partitioning the device.

Reword:

The total capacity, volatile only capacity, and persistent only capacity are
required to properly formulate a set partition info command.

Provide functions to retrieve these values from the IDENTIFY command.  Like the
partition information commands these return the values in bytes.

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/libcxl.h       |  3 +++
>  cxl/lib/libcxl.c   | 29 +++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  3 +++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 7cf9061..d333b6d 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -68,6 +68,9 @@ int cxl_cmd_get_mbox_status(struct cxl_cmd *cmd);
>  int cxl_cmd_get_out_size(struct cxl_cmd *cmd);
>  struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev);
>  int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev, int fw_len);
> +unsigned long long cxl_cmd_identify_get_total_capacity(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_identify_get_volatile_only_capacity(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_identify_get_persistent_only_capacity(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);
>  struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index f3d4022..715d8e4 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1102,6 +1102,35 @@ CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
>  	return le32_to_cpu(id->lsa_size);
>  }
>  
> +#define cmd_identify_get_capacity_field(cmd, field)			\
> +do {										\
> +	struct cxl_cmd_identify *c =					\
> +		(struct cxl_cmd_identify *)cmd->send_cmd->out.payload;\
> +	int rc = cxl_cmd_validate_status(cmd,					\
> +			CXL_MEM_COMMAND_ID_IDENTIFY);			\
> +	if (rc)									\
> +		return ULLONG_MAX;							\
> +	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
> +} while (0)
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_total_capacity(struct cxl_cmd *cmd)

Is there someplace that all the libcxl functions are documented?  Like the
other functions I would like to ensure the user knows these are returning
values in bytes.

Ira

> +{
> +	cmd_identify_get_capacity_field(cmd, total_capacity);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_volatile_only_capacity(struct cxl_cmd *cmd)
> +{
> +	cmd_identify_get_capacity_field(cmd, volatile_capacity);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_persistent_only_capacity(struct cxl_cmd *cmd)
> +{
> +	cmd_identify_get_capacity_field(cmd, persistent_capacity);
> +}
> +
>  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 09d6d94..bed6427 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -31,6 +31,9 @@ global:
>  	cxl_cmd_get_out_size;
>  	cxl_cmd_new_identify;
>  	cxl_cmd_identify_get_fw_rev;
> +	cxl_cmd_identify_get_total_capacity;
> +	cxl_cmd_identify_get_volatile_only_capacity;
> +	cxl_cmd_identify_get_persistent_only_capacity;
>  	cxl_cmd_identify_get_partition_align;
>  	cxl_cmd_identify_get_label_size;
>  	cxl_cmd_new_get_health_info;
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 3/7] libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:14PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> The IDENTIFY mailbox command returns the partition alignment field
> expressed in multiples of 256 MB.

Interesting...

I don't think anyone is using this function just yet but this does technically
change the behavior of this function.

Will that break anyone or cxl-cli?

> Use CXL_CAPACITY_MULTIPLIER when
> returning this field.
> 
> This is in sync with all the other partitioning related fields using
> the multiplier.

To me the fact that this was not in bytes implies that the original API of
libcxl was intended to not convert these values.

Vishal may have an opinion.  Should these be in bytes or 'CXL Capacity' units
(ie 256MB's)?

I think I prefer bytes as well.

Ira

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/libcxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 715d8e4..85a6c0e 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1086,7 +1086,7 @@ CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
>  	if (cmd->status < 0)
>  		return cmd->status;
>  
> -	return le64_to_cpu(id->partition_align);
> +	return le64_to_cpu(id->partition_align) * CXL_CAPACITY_MULTIPLIER;
>  }
>  
>  CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 4/7] cxl: add memdev partition information to cxl-list

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:15PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 

"Users will want to be able to check the current partition information.  In
addition they will need to know the capacity values to form a valid set
partition information command."

> Add information useful for managing memdev partitions to cxl-list
     ^
   "optional"

> output. Include all of the fields from GET_PARTITION_INFO and the
> partitioning related fields from the IDENTIFY mailbox command.
> 

"Sample output for this section is:"

>     "partition":{
>       "active_volatile_capacity":273535729664,
>       "active_persistent_capacity":0,
>       "next_volatile_capacity":0,
>       "next_persistent_capacity":0,
>       "total_capacity":273535729664,
>       "volatile_only_capacity":0,
>       "persistent_only_capacity":0,
>       "partition_alignment":268435456
>     }
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-list.txt |  23 +++++++
>  util/json.h                    |   1 +
>  cxl/list.c                     |   5 ++
>  util/json.c                    | 112 +++++++++++++++++++++++++++++++++
>  4 files changed, 141 insertions(+)
> 
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index c8d10fb..e65e944 100644
> --- a/Documentation/cxl/cxl-list.txt
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -85,6 +85,29 @@ OPTIONS
>    }
>  ]
>  ----
> +-P::
> +--partition::
> +	Include partition information in the memdev listing. Example listing:
> +----
> +# cxl list -m mem0 -P
> +[
> +  {
> +    "memdev":"mem0",
> +    "pmem_size":0,
> +    "ram_size":273535729664,
> +    "partition":{
> +      "active_volatile_capacity":273535729664,
> +      "active_persistent_capacity":0,
> +      "next_volatile_capacity":0,
> +      "next_persistent_capacity":0,
> +      "total_capacity":273535729664,
> +      "volatile_only_capacity":0,
> +      "persistent_only_capacity":0,
> +      "partition_alignment":268435456
> +    }
> +  }
> +]
> +----
>  
>  include::human-option.txt[]
>  
> diff --git a/util/json.h b/util/json.h
> index ce575e6..76a8816 100644
> --- a/util/json.h
> +++ b/util/json.h
> @@ -20,6 +20,7 @@ enum util_json_flags {
>  	UTIL_JSON_FIRMWARE	= (1 << 8),
>  	UTIL_JSON_DAX_MAPPINGS	= (1 << 9),
>  	UTIL_JSON_HEALTH	= (1 << 10),
> +	UTIL_JSON_PARTITION	= (1 << 11),
>  };
>  
>  struct json_object;
> diff --git a/cxl/list.c b/cxl/list.c
> index b1468b7..368ec21 100644
> --- a/cxl/list.c
> +++ b/cxl/list.c
> @@ -17,6 +17,7 @@ static struct {
>  	bool idle;
>  	bool human;
>  	bool health;
> +	bool partition;
>  } list;
>  
>  static unsigned long listopts_to_flags(void)
> @@ -29,6 +30,8 @@ static unsigned long listopts_to_flags(void)
>  		flags |= UTIL_JSON_HUMAN;
>  	if (list.health)
>  		flags |= UTIL_JSON_HEALTH;
> +	if (list.partition)
> +		flags |= UTIL_JSON_PARTITION;
>  	return flags;
>  }
>  
> @@ -62,6 +65,8 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>  				"use human friendly number formats "),
>  		OPT_BOOLEAN('H', "health", &list.health,
>  				"include memory device health information "),
> +		OPT_BOOLEAN('P', "partition", &list.partition,
> +				"include memory device partition information "),
>  		OPT_END(),
>  	};
>  	const char * const u[] = {
> diff --git a/util/json.c b/util/json.c
> index f97cf07..4254dea 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -1616,6 +1616,113 @@ err_jobj:
>  	return NULL;
>  }
>  
> +/*
> + * Present complete view of memdev partition by presenting fields from
> + * both GET_PARTITION_INFO and IDENTIFY mailbox commands.
> + */
> +static struct json_object *util_cxl_memdev_partition_to_json(struct cxl_memdev *memdev,
> +		unsigned long flags)
> +{
> +	struct json_object *jobj = NULL;
> +	struct json_object *jpart;
> +	unsigned long long cap;
> +	struct cxl_cmd *cmd;
> +	int rc;
> +
> +	jpart = json_object_new_object();
> +	if (!jpart)
> +		return NULL;
> +	if (!memdev)
> +		goto err_jobj;
> +
> +	/* Retrieve partition info in GET_PARTITION_INFO mbox cmd */
> +	cmd = cxl_cmd_new_get_partition_info(memdev);

Oh...  now I understand the 'new'...  Sorry...

> +	if (!cmd)
> +		goto err_jobj;
> +
> +	rc = cxl_cmd_submit(cmd);
> +	if (rc < 0)
> +		goto err_cmd;
> +	rc = cxl_cmd_get_mbox_status(cmd);
> +	if (rc != 0)
> +		goto err_cmd;
> +
> +	cap = cxl_cmd_get_partition_info_get_active_volatile_cap(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"active_volatile_capacity", jobj);
> +	}
> +	cap = cxl_cmd_get_partition_info_get_active_persistent_cap(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"active_persistent_capacity", jobj);
> +	}
> +	cap = cxl_cmd_get_partition_info_get_next_volatile_cap(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"next_volatile_capacity", jobj);
> +	}
> +	cap = cxl_cmd_get_partition_info_get_next_persistent_cap(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"next_persistent_capacity", jobj);
> +	}
> +	cxl_cmd_unref(cmd);
> +
> +	/* Retrieve partition info in the IDENTIFY mbox cmd */
> +	cmd = cxl_cmd_new_identify(memdev);
> +	if (!cmd)
> +		goto err_jobj;
> +
> +	rc = cxl_cmd_submit(cmd);
> +	if (rc < 0)
> +		goto err_cmd;
> +	rc = cxl_cmd_get_mbox_status(cmd);
> +	if (rc != 0)
> +		goto err_cmd;
> +
> +	cap = cxl_cmd_identify_get_total_capacity(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart, "total_capacity", jobj);
> +	}
> +	cap = cxl_cmd_identify_get_volatile_only_capacity(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"volatile_only_capacity", jobj);
> +	}
> +	cap = cxl_cmd_identify_get_persistent_only_capacity(cmd);
> +	if (cap != ULLONG_MAX) {
> +		jobj = util_json_object_size(cap, flags);
> +		if (jobj)
> +			json_object_object_add(jpart,
> +					"persistent_only_capacity", jobj);
> +	}
> +	cap = cxl_cmd_identify_get_partition_align(cmd);
> +	jobj = util_json_object_size(cap, flags);
> +	if (jobj)
> +		json_object_object_add(jpart, "partition_alignment", jobj);
> +
> +	return jpart;
> +
> +err_cmd:

Doesn't this need to be called always, not just on error?

Ira

> +	cxl_cmd_unref(cmd);
> +err_jobj:
> +	json_object_put(jpart);
> +	return NULL;
> +}
> +
>  struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  		unsigned long flags)
>  {
> @@ -1643,5 +1750,10 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  		if (jobj)
>  			json_object_object_add(jdev, "health", jobj);
>  	}
> +	if (flags & UTIL_JSON_PARTITION) {
> +		jobj = util_cxl_memdev_partition_to_json(memdev, flags);
> +		if (jobj)
> +			json_object_object_add(jdev, "partition", jobj);
> +	}
>  	return jdev;
>  }
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 5/7] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:16PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add APIs to allocate and send a SET_PARTITION_INFO mailbox command.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/private.h  |  9 +++++++++
>  cxl/libcxl.h       |  4 ++++
>  cxl/lib/libcxl.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  3 +++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index dd9234f..841aa80 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -114,6 +114,15 @@ struct cxl_cmd_get_partition_info {
>  
>  #define CXL_CAPACITY_MULTIPLIER		SZ_256M
>  
> +struct cxl_cmd_set_partition_info {
> +	le64 volatile_capacity;
> +	u8 flags;
> +} __attribute__((packed));
> +
> +/* CXL 2.0 8.2.9.5.2 Set Partition Info */
> +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG				(0)
> +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG			(1)

BIT(0) and BIT(1)?

I can't remember which bit is the immediate flag.

> +
>  /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
>  #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
>  #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index d333b6d..67d6ffc 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -50,6 +50,8 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
>  		size_t offset);
>  int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
>  		size_t offset);
> +int cxl_memdev_set_partition_info(struct cxl_memdev *memdev,
> +		unsigned long long volatile_capacity, int flags);
>  
>  #define cxl_memdev_foreach(ctx, memdev) \
>          for (memdev = cxl_memdev_get_first(ctx); \
> @@ -117,6 +119,8 @@ unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl
>  unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
>  unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
>  unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
> +struct cxl_cmd *cxl_cmd_new_set_partition_info(struct cxl_memdev *memdev,
> +		unsigned long long volatile_capacity, int flags);
>  
>  #ifdef __cplusplus
>  } /* extern "C" */
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 85a6c0e..877a783 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1227,6 +1227,21 @@ cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
>  	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
>  }
>  
> +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_set_partition_info(struct cxl_memdev *memdev,
> +		unsigned long long volatile_capacity, int flags)
> +{
> +	struct cxl_cmd_set_partition_info *set_partition;
> +	struct cxl_cmd *cmd;
> +
> +	cmd = cxl_cmd_new_generic(memdev,
> +			CXL_MEM_COMMAND_ID_SET_PARTITION_INFO);
> +
> +	set_partition = (struct cxl_cmd_set_partition_info *)cmd->send_cmd->in.payload;
> +	set_partition->volatile_capacity = cpu_to_le64(volatile_capacity);
> +	set_partition->flags = flags;
> +	return cmd;
> +}
> +
>  CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
>  {
>  	struct cxl_memdev *memdev = cmd->memdev;
> @@ -1425,3 +1440,33 @@ CXL_EXPORT int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf,
>  {
>  	return lsa_op(memdev, LSA_OP_GET, buf, length, offset);
>  }
> +
> +CXL_EXPORT int cxl_memdev_set_partition_info(struct cxl_memdev *memdev,
> +	       unsigned long long volatile_capacity, int flags)
> +{
> +	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
> +	struct cxl_cmd *cmd;
> +	int rc;
> +
> +	dbg(ctx, "%s: enter cap: %llx, flags %d\n", __func__,
> +		volatile_capacity, flags);
> +
> +	cmd = cxl_cmd_new_set_partition_info(memdev,
> +			volatile_capacity / CXL_CAPACITY_MULTIPLIER, flags);

I'll reiterate that I agree with this capacity being in bytes.  But I'm not
sure what the rest of libcxl does.

Ira

> +	if (!cmd)
> +		return -ENXIO;
> +
> +	rc = cxl_cmd_submit(cmd);
> +	if (rc < 0) {
> +		err(ctx, "cmd submission failed: %s\n", strerror(-rc));
> +		goto err;
> +	}
> +	rc = cxl_cmd_get_mbox_status(cmd);
> +	if (rc != 0) {
> +		err(ctx, "%s: mbox status: %d\n", __func__, rc);
> +		rc = -ENXIO;
> +	}
> +err:
> +	cxl_cmd_unref(cmd);
> +	return rc;
> +}
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index bed6427..5d02c45 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -78,6 +78,9 @@ global:
>  	cxl_cmd_get_partition_info_get_active_persistent_cap;
>  	cxl_cmd_get_partition_info_get_next_volatile_cap;
>  	cxl_cmd_get_partition_info_get_next_persistent_cap;
> +	cxl_cmd_new_set_partition_info;
> +	cxl_memdev_set_partition_info;
> +
>  local:
>          *;
>  };
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 6/7] ndctl, util: use 'unsigned long long' type in OPT_U64 define

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:17PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> The OPT_U64 define failed in check_vtype() with unknown 'u64' type.
> Replace with 'unsigned long long' to make the OPT_U64 define usable.

I feel like this should be the first patch in the series.

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  util/parse-options.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/parse-options.h b/util/parse-options.h
> index 9318fe7..91b7932 100644
> --- a/util/parse-options.h
> +++ b/util/parse-options.h
> @@ -124,7 +124,7 @@ struct option {
>  #define OPT_INTEGER(s, l, v, h)     { .type = OPTION_INTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h) }
>  #define OPT_UINTEGER(s, l, v, h)    { .type = OPTION_UINTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned int *), .help = (h) }
>  #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
> -#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
> +#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned long long *), .help = (h) }

Why can't this be uint64_t?

Ira

>  #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
>  #define OPT_FILENAME(s, l, v, a, h) { .type = OPTION_FILENAME, .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
>  #define OPT_DATE(s, l, v, h) \
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Ira Weiny]

On Mon, Jan 03, 2022 at 12:16:18PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 

"User will need a command line option for setting partition info.  Add
'set-partition-info' to the cxl command line tool."

> The command 'cxl set-partition-info' operates on a CXL memdev,
> or a set of memdevs, allowing the user to change the partition
> layout of the device.
                ^^^^^^
		device(s).

> 
> Synopsis:
> Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
> 
>     -v, --verbose         turn on debug
>     -s, --volatile_size <n>
>                           next volatile partition size in bytes
> 
> The MAN page explains how to find partitioning capabilities and
> restrictions.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-set-partition-info.txt | 27 ++++++
>  Documentation/cxl/partition-description.txt  | 15 ++++
>  Documentation/cxl/partition-options.txt      | 19 +++++
>  Documentation/cxl/Makefile.am                |  3 +-
>  cxl/builtin.h                                |  1 +
>  cxl/cxl.c                                    |  1 +
>  cxl/memdev.c                                 | 89 ++++++++++++++++++++
>  7 files changed, 154 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
>  create mode 100644 Documentation/cxl/partition-description.txt
>  create mode 100644 Documentation/cxl/partition-options.txt
> 
> diff --git a/Documentation/cxl/cxl-set-partition-info.txt b/Documentation/cxl/cxl-set-partition-info.txt
> new file mode 100644
> index 0000000..32418b6
> --- /dev/null
> +++ b/Documentation/cxl/cxl-set-partition-info.txt
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-set-partition-info(1)
> +=========================
> +
> +NAME
> +----
> +cxl-set-partition-info - set the partitioning between volatile and persistent capacity on a CXL memdev
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl set-partition-info <mem> [ [<mem1>..<memN>] [<options>]'
> +
> +include::partition-description.txt[]
> +Partition the device on the next device reset using the volatile capacity
> +requested. Using this command to change the size of the persistent capacity
> +shall result in the loss of stored data.
> +
> +OPTIONS
> +-------
> +include::partition-options.txt[]
> +
> +SEE ALSO
> +--------
> +linkcxl:cxl-list[1],

Did I miss the update to the cxl-list command documentation?

> +CXL-2.0 8.2.9.5.2
> diff --git a/Documentation/cxl/partition-description.txt b/Documentation/cxl/partition-description.txt
> new file mode 100644
> index 0000000..b3efac8
> --- /dev/null
> +++ b/Documentation/cxl/partition-description.txt
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +DESCRIPTION
> +-----------
> +Partition the device into volatile and persistent capacity. The change
> +in partitioning will become the “next” configuration, to become active
> +on the next device reset.
> +
> +Use "cxl list -m <memdev> -P" to examine the partition capacities
> +supported on a device. Paritionable capacity must exist on the
> +device. A partition_alignment of zero means no partitionable
> +capacity is available.
> +
> +Using this command to change the size of the persistent capacity shall
> +result in the loss of data stored.
> diff --git a/Documentation/cxl/partition-options.txt b/Documentation/cxl/partition-options.txt
> new file mode 100644
> index 0000000..84e49c9
> --- /dev/null
> +++ b/Documentation/cxl/partition-options.txt
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +<memory device(s)>::
> +include::memdev-option.txt[]
> +
> +-s::
> +--size=::
> +	Size in bytes of the volatile partition requested.
> +
> +	Size must align to the devices partition_alignment.
> +	Use 'cxl list -m <memdev> -P' to find partition_alignment.
> +
> +	Size must be less than or equal to the devices partitionable bytes.
> +	(total_capacity - volatile_only_capacity - persistent_only_capacity)
> +	Use 'cxl list -m <memdev> -P' to find *_capacity values.
> +
> +-v::
> +	Turn on verbose debug messages in the library (if libcxl was built with
> +	logging and debug enabled).
> diff --git a/Documentation/cxl/Makefile.am b/Documentation/cxl/Makefile.am
> index efabaa3..c5faf04 100644
> --- a/Documentation/cxl/Makefile.am
> +++ b/Documentation/cxl/Makefile.am
> @@ -22,7 +22,8 @@ man1_MANS = \
>  	cxl-list.1 \
>  	cxl-read-labels.1 \
>  	cxl-write-labels.1 \
> -	cxl-zero-labels.1
> +	cxl-zero-labels.1 \
> +	cxl-set-partition-info.1
>  
>  EXTRA_DIST = $(man1_MANS)
>  
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index 78eca6e..7f11f28 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -10,4 +10,5 @@ int cmd_read_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_init_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_check_labels(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index 4b1661d..3153cf0 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
>  	{ "zero-labels", .c_fn = cmd_zero_labels },
>  	{ "read-labels", .c_fn = cmd_read_labels },
>  	{ "write-labels", .c_fn = cmd_write_labels },
> +	{ "set-partition-info", .c_fn = cmd_set_partition_info },
>  };
>  
>  int main(int argc, const char **argv)
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 5ee38e5..fa63317 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -6,6 +6,7 @@
>  #include <unistd.h>
>  #include <limits.h>
>  #include <util/log.h>
> +#include <util/size.h>
>  #include <util/filter.h>
>  #include <cxl/libcxl.h>
>  #include <util/parse-options.h>
> @@ -23,6 +24,7 @@ static struct parameters {
>  	unsigned len;
>  	unsigned offset;
>  	bool verbose;
> +	unsigned long long volatile_size;
>  } param;
>  
>  #define fail(fmt, ...) \
> @@ -47,6 +49,10 @@ OPT_UINTEGER('s', "size", &param.len, "number of label bytes to operate"), \
>  OPT_UINTEGER('O', "offset", &param.offset, \
>  	"offset into the label area to start operation")
>  
> +#define SET_PARTITION_OPTIONS() \
> +OPT_U64('s', "volatile_size",  &param.volatile_size, \
> +	"next volatile partition size in bytes")
> +
>  static const struct option read_options[] = {
>  	BASE_OPTIONS(),
>  	LABEL_OPTIONS(),
> @@ -67,6 +73,12 @@ static const struct option zero_options[] = {
>  	OPT_END(),
>  };
>  
> +static const struct option set_partition_options[] = {
> +	BASE_OPTIONS(),
> +	SET_PARTITION_OPTIONS(),
> +	OPT_END(),
> +};
> +
>  static int action_zero(struct cxl_memdev *memdev, struct action_context *actx)
>  {
>  	size_t size;
> @@ -174,6 +186,73 @@ out:
>  	return rc;
>  }
>  
> +static int validate_partition(struct cxl_memdev *memdev,
> +		unsigned long long volatile_request)
> +{
> +	unsigned long long total_cap, volatile_only, persistent_only;
> +	unsigned long long partitionable_bytes, partition_align_bytes;
> +	const char *devname = cxl_memdev_get_devname(memdev);
> +	struct cxl_cmd *cmd;
> +	int rc;
> +
> +	cmd = cxl_cmd_new_identify(memdev);
> +	if (!cmd)
> +		return -ENXIO;
> +	rc = cxl_cmd_submit(cmd);
> +	if (rc < 0)
> +		goto err;
> +	rc = cxl_cmd_get_mbox_status(cmd);
> +	if (rc != 0)
> +		goto err;
> +
> +	partition_align_bytes = cxl_cmd_identify_get_partition_align(cmd);
> +	if (partition_align_bytes == 0) {
> +		fprintf(stderr, "%s: no partitionable capacity\n", devname);
> +		rc = -EINVAL;
> +		goto err;
> +	}
> +
> +	total_cap = cxl_cmd_identify_get_total_capacity(cmd);
> +	volatile_only = cxl_cmd_identify_get_volatile_only_capacity(cmd);
> +	persistent_only = cxl_cmd_identify_get_persistent_only_capacity(cmd);
> +
> +	partitionable_bytes = total_cap - volatile_only - persistent_only;
> +
> +	if (volatile_request > partitionable_bytes) {
> +		fprintf(stderr, "%s: volatile size %lld exceeds partitionable capacity %lld\n",
> +			devname, volatile_request, partitionable_bytes);
> +		rc = -EINVAL;
> +		goto err;
> +	}
> +	if (!IS_ALIGNED(volatile_request, partition_align_bytes)) {
> +		fprintf(stderr, "%s: volatile size %lld is not partition aligned %lld\n",
> +			devname, volatile_request, partition_align_bytes);
> +		rc = -EINVAL;
> +	}
> +err:
> +	cxl_cmd_unref(cmd);
> +	return rc;
> +}
> +
> +static int action_set_partition(struct cxl_memdev *memdev,
> +		struct action_context *actx)
> +{
> +	const char *devname = cxl_memdev_get_devname(memdev);
> +	unsigned long long volatile_request;
> +	int rc;
> +
> +	volatile_request = param.volatile_size;
> +
> +	rc = validate_partition(memdev, volatile_request);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_memdev_set_partition_info(memdev, volatile_request, 0);

CXL_CMD_SET_PARTITION_INFO_NO_FLAG?

Ira

> +	if (rc)
> +		fprintf(stderr, "%s error: %s\n", devname, strerror(-rc));
> +	return rc;
> +}
> +
>  static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  		int (*action)(struct cxl_memdev *memdev, struct action_context *actx),
>  		const struct option *options, const char *usage)
> @@ -322,3 +401,13 @@ int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx)
>  			count > 1 ? "s" : "");
>  	return count >= 0 ? 0 : EXIT_FAILURE;
>  }
> +
> +int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +	int count = memdev_action(argc, argv, ctx, action_set_partition,
> +			set_partition_options,
> +			"cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]");
> +	fprintf(stderr, "set_partition %d mem%s\n", count >= 0 ? count : 0,
> +			count > 1 ? "s" : "");
> +	return count >= 0 ? 0 : EXIT_FAILURE;
> +}
> -- 
> 2.31.1
> 

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Dan Williams]

On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> > command output data structure (privately), and accessor APIs to return
> > the different fields in the partition info output.
>
> I would rephrase this:
>
> libcxl provides functions for C code to issue cxl mailbox commands as well as
> parse the output returned.  Get partition info should be part of this API.
>
> Add the libcxl get partition info mailbox support as well as an API to parse
> the fields of the command returned.
>
> All fields are specified in multiples of 256MB so also define a capacity
> multiplier to convert the raw data into bytes.
>
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/private.h  | 10 ++++++++++
> >  cxl/libcxl.h       |  5 +++++
> >  util/size.h        |  1 +
> >  cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym |  5 +++++
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index a1b8b50..dd9234f 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -7,6 +7,7 @@
> >  #include <cxl/cxl_mem.h>
> >  #include <ccan/endian/endian.h>
> >  #include <ccan/short_types/short_types.h>
> > +#include <util/size.h>
> >
> >  #define CXL_EXPORT __attribute__ ((visibility("default")))
> >
> > @@ -104,6 +105,15 @@ struct cxl_cmd_get_health_info {
> >       le32 pmem_errors;
> >  } __attribute__((packed));
> >
> > +struct cxl_cmd_get_partition_info {
> > +     le64 active_volatile_cap;
> > +     le64 active_persistent_cap;
> > +     le64 next_volatile_cap;
> > +     le64 next_persistent_cap;
> > +} __attribute__((packed));
> > +
> > +#define CXL_CAPACITY_MULTIPLIER              SZ_256M
> > +
> >  /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
> >  #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK           BIT(0)
> >  #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK         BIT(1)
> > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > index 89d35ba..7cf9061 100644
> > --- a/cxl/libcxl.h
> > +++ b/cxl/libcxl.h
> > @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
> >               unsigned int length);
> >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> >               void *buf, unsigned int offset, unsigned int length);
> > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
>
> why 'new'?  Why not:
>
> cxl_cmd_get_partition_info()
>
> ?

The "new" is the naming convention inherited from ndctl indicating the
allocation of a new command object. The motivation is to have a verb /
action in all of the APIs.

>
> > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
>
> These are pretty long function names.  :-/

If you think those are long, how about:

cxl_cmd_health_info_get_media_powerloss_persistence_loss

The motivation here is to keep data structure layouts hidden behind
APIs to ease the maintenance of binary compatibility from one library
release and specification release to the next. The side effect though
is some long function names in places.

> I also wonder if the conversion to bytes should be reflected in the function
> name.  Because returning 'cap' might imply to someone they are getting the raw
> data field.

Makes sense.

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Ira Weiny]

On Thu, Jan 06, 2022 at 01:21:45PM -0800, Dan Williams wrote:
> On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > > index 89d35ba..7cf9061 100644
> > > --- a/cxl/libcxl.h
> > > +++ b/cxl/libcxl.h
> > > @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
> > >               unsigned int length);
> > >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> > >               void *buf, unsigned int offset, unsigned int length);
> > > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
> >
> > why 'new'?  Why not:
> >
> > cxl_cmd_get_partition_info()
> >
> > ?
> 
> The "new" is the naming convention inherited from ndctl indicating the
> allocation of a new command object. The motivation is to have a verb /
> action in all of the APIs.

Yea my bad.  I realized that later on.  Sorry.

> 
> >
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
> >
> > These are pretty long function names.  :-/
> 
> If you think those are long, how about:
> 
> cxl_cmd_health_info_get_media_powerloss_persistence_loss
> 
> The motivation here is to keep data structure layouts hidden behind
> APIs to ease the maintenance of binary compatibility from one library
> release and specification release to the next. The side effect though
> is some long function names in places.

Sure.  I'm ok with that.

Ira

> 
> > I also wonder if the conversion to bytes should be reflected in the function
> > name.  Because returning 'cap' might imply to someone they are getting the raw
> > data field.
> 
> Makes sense.

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Dan Williams]

On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> The command 'cxl set-partition-info' operates on a CXL memdev,
> or a set of memdevs, allowing the user to change the partition
> layout of the device.
>
> Synopsis:
> Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
>
>     -v, --verbose         turn on debug
>     -s, --volatile_size <n>
>                           next volatile partition size in bytes
>
> The MAN page explains how to find partitioning capabilities and
> restrictions.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
[..]
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 5ee38e5..fa63317 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -6,6 +6,7 @@
>  #include <unistd.h>
>  #include <limits.h>
>  #include <util/log.h>
> +#include <util/size.h>
>  #include <util/filter.h>
>  #include <cxl/libcxl.h>
>  #include <util/parse-options.h>
> @@ -23,6 +24,7 @@ static struct parameters {
>         unsigned len;
>         unsigned offset;
>         bool verbose;
> +       unsigned long long volatile_size;

This should be a string.

See parse_size64() that handles suffixes like K,M,G,T, so you can do
things like "cxl set-partition -s 256M" and behave like the other
"--size" options in ndctl.

Re: [ndctl PATCH 4/7] cxl: add memdev partition information to cxl-list

[Dan Williams]

On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Add information useful for managing memdev partitions to cxl-list
> output. Include all of the fields from GET_PARTITION_INFO and the
> partitioning related fields from the IDENTIFY mailbox command.
>
>     "partition":{

Perhaps call it "parition_info"?

>       "active_volatile_capacity":273535729664,
>       "active_persistent_capacity":0,
>       "next_volatile_capacity":0,
>       "next_persistent_capacity":0,
>       "total_capacity":273535729664,
>       "volatile_only_capacity":0,
>       "persistent_only_capacity":0,
>       "partition_alignment":268435456
>     }
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-list.txt |  23 +++++++
>  util/json.h                    |   1 +
>  cxl/list.c                     |   5 ++
>  util/json.c                    | 112 +++++++++++++++++++++++++++++++++
>  4 files changed, 141 insertions(+)
>
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index c8d10fb..e65e944 100644
> --- a/Documentation/cxl/cxl-list.txt
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -85,6 +85,29 @@ OPTIONS
>    }
>  ]
>  ----
> +-P::
> +--partition::
> +       Include partition information in the memdev listing. Example listing:

How about -I/--partition for partition "Info". I had earmarked -P for
including "Port" object in the listing.

Other than that, looks good:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

> +----
> +# cxl list -m mem0 -P
> +[
> +  {
> +    "memdev":"mem0",
> +    "pmem_size":0,
> +    "ram_size":273535729664,
> +    "partition":{
> +      "active_volatile_capacity":273535729664,
> +      "active_persistent_capacity":0,
> +      "next_volatile_capacity":0,
> +      "next_persistent_capacity":0,
> +      "total_capacity":273535729664,
> +      "volatile_only_capacity":0,
> +      "persistent_only_capacity":0,
> +      "partition_alignment":268435456
> +    }
> +  }
> +]
> +----
>
>  include::human-option.txt[]
>
> diff --git a/util/json.h b/util/json.h
> index ce575e6..76a8816 100644
> --- a/util/json.h
> +++ b/util/json.h
> @@ -20,6 +20,7 @@ enum util_json_flags {
>         UTIL_JSON_FIRMWARE      = (1 << 8),
>         UTIL_JSON_DAX_MAPPINGS  = (1 << 9),
>         UTIL_JSON_HEALTH        = (1 << 10),
> +       UTIL_JSON_PARTITION     = (1 << 11),
>  };
>
>  struct json_object;
> diff --git a/cxl/list.c b/cxl/list.c
> index b1468b7..368ec21 100644
> --- a/cxl/list.c
> +++ b/cxl/list.c
> @@ -17,6 +17,7 @@ static struct {
>         bool idle;
>         bool human;
>         bool health;
> +       bool partition;
>  } list;
>
>  static unsigned long listopts_to_flags(void)
> @@ -29,6 +30,8 @@ static unsigned long listopts_to_flags(void)
>                 flags |= UTIL_JSON_HUMAN;
>         if (list.health)
>                 flags |= UTIL_JSON_HEALTH;
> +       if (list.partition)
> +               flags |= UTIL_JSON_PARTITION;
>         return flags;
>  }
>
> @@ -62,6 +65,8 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>                                 "use human friendly number formats "),
>                 OPT_BOOLEAN('H', "health", &list.health,
>                                 "include memory device health information "),
> +               OPT_BOOLEAN('P', "partition", &list.partition,
> +                               "include memory device partition information "),
>                 OPT_END(),
>         };
>         const char * const u[] = {
> diff --git a/util/json.c b/util/json.c
> index f97cf07..4254dea 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -1616,6 +1616,113 @@ err_jobj:
>         return NULL;
>  }
>
> +/*
> + * Present complete view of memdev partition by presenting fields from
> + * both GET_PARTITION_INFO and IDENTIFY mailbox commands.
> + */
> +static struct json_object *util_cxl_memdev_partition_to_json(struct cxl_memdev *memdev,
> +               unsigned long flags)
> +{
> +       struct json_object *jobj = NULL;
> +       struct json_object *jpart;
> +       unsigned long long cap;
> +       struct cxl_cmd *cmd;
> +       int rc;
> +
> +       jpart = json_object_new_object();
> +       if (!jpart)
> +               return NULL;
> +       if (!memdev)
> +               goto err_jobj;
> +
> +       /* Retrieve partition info in GET_PARTITION_INFO mbox cmd */
> +       cmd = cxl_cmd_new_get_partition_info(memdev);
> +       if (!cmd)
> +               goto err_jobj;
> +
> +       rc = cxl_cmd_submit(cmd);
> +       if (rc < 0)
> +               goto err_cmd;
> +       rc = cxl_cmd_get_mbox_status(cmd);
> +       if (rc != 0)
> +               goto err_cmd;
> +
> +       cap = cxl_cmd_get_partition_info_get_active_volatile_cap(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "active_volatile_capacity", jobj);
> +       }
> +       cap = cxl_cmd_get_partition_info_get_active_persistent_cap(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "active_persistent_capacity", jobj);
> +       }
> +       cap = cxl_cmd_get_partition_info_get_next_volatile_cap(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "next_volatile_capacity", jobj);
> +       }
> +       cap = cxl_cmd_get_partition_info_get_next_persistent_cap(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "next_persistent_capacity", jobj);
> +       }
> +       cxl_cmd_unref(cmd);
> +
> +       /* Retrieve partition info in the IDENTIFY mbox cmd */
> +       cmd = cxl_cmd_new_identify(memdev);
> +       if (!cmd)
> +               goto err_jobj;
> +
> +       rc = cxl_cmd_submit(cmd);
> +       if (rc < 0)
> +               goto err_cmd;
> +       rc = cxl_cmd_get_mbox_status(cmd);
> +       if (rc != 0)
> +               goto err_cmd;
> +
> +       cap = cxl_cmd_identify_get_total_capacity(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart, "total_capacity", jobj);
> +       }
> +       cap = cxl_cmd_identify_get_volatile_only_capacity(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "volatile_only_capacity", jobj);
> +       }
> +       cap = cxl_cmd_identify_get_persistent_only_capacity(cmd);
> +       if (cap != ULLONG_MAX) {
> +               jobj = util_json_object_size(cap, flags);
> +               if (jobj)
> +                       json_object_object_add(jpart,
> +                                       "persistent_only_capacity", jobj);
> +       }
> +       cap = cxl_cmd_identify_get_partition_align(cmd);
> +       jobj = util_json_object_size(cap, flags);
> +       if (jobj)
> +               json_object_object_add(jpart, "partition_alignment", jobj);
> +
> +       return jpart;
> +
> +err_cmd:
> +       cxl_cmd_unref(cmd);
> +err_jobj:
> +       json_object_put(jpart);
> +       return NULL;
> +}
> +
>  struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>                 unsigned long flags)
>  {
> @@ -1643,5 +1750,10 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>                 if (jobj)
>                         json_object_object_add(jdev, "health", jobj);
>         }
> +       if (flags & UTIL_JSON_PARTITION) {
> +               jobj = util_cxl_memdev_partition_to_json(memdev, flags);
> +               if (jobj)
> +                       json_object_object_add(jdev, "partition", jobj);
> +       }
>         return jdev;
>  }
> --
> 2.31.1
>

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Verma, Vishal L]

On Thu, 2022-01-06 at 13:21 -0800, Dan Williams wrote:
> On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
> > 

[..]

> > > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
> > 
> > why 'new'?  Why not:
> > 
> > cxl_cmd_get_partition_info()
> > 
> > ?
> 
> The "new" is the naming convention inherited from ndctl indicating the
> allocation of a new command object. The motivation is to have a verb /
> action in all of the APIs.
> 
> > 
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);

Just one nit here about the double verb 'get'. In such cases,
get_partition_info can just become 'partition_info'

e.g.: cxl_cmd_partition_info_get_active_volatile_cap(...

> 

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Dan Williams]

On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> The command 'cxl set-partition-info' operates on a CXL memdev,
> or a set of memdevs, allowing the user to change the partition
> layout of the device.
>
> Synopsis:
> Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
>
>     -v, --verbose         turn on debug
>     -s, --volatile_size <n>
>                           next volatile partition size in bytes
>
> The MAN page explains how to find partitioning capabilities and
> restrictions.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-set-partition-info.txt | 27 ++++++
>  Documentation/cxl/partition-description.txt  | 15 ++++
>  Documentation/cxl/partition-options.txt      | 19 +++++
>  Documentation/cxl/Makefile.am                |  3 +-
>  cxl/builtin.h                                |  1 +
>  cxl/cxl.c                                    |  1 +
>  cxl/memdev.c                                 | 89 ++++++++++++++++++++
>  7 files changed, 154 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
>  create mode 100644 Documentation/cxl/partition-description.txt
>  create mode 100644 Documentation/cxl/partition-options.txt
>
> diff --git a/Documentation/cxl/cxl-set-partition-info.txt b/Documentation/cxl/cxl-set-partition-info.txt
> new file mode 100644
> index 0000000..32418b6
> --- /dev/null
> +++ b/Documentation/cxl/cxl-set-partition-info.txt
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-set-partition-info(1)
> +=========================
> +
> +NAME
> +----
> +cxl-set-partition-info - set the partitioning between volatile and persistent capacity on a CXL memdev
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl set-partition-info <mem> [ [<mem1>..<memN>] [<options>]'
> +
> +include::partition-description.txt[]
> +Partition the device on the next device reset using the volatile capacity
> +requested. Using this command to change the size of the persistent capacity
> +shall result in the loss of stored data.
> +
> +OPTIONS
> +-------
> +include::partition-options.txt[]
> +
> +SEE ALSO
> +--------
> +linkcxl:cxl-list[1],
> +CXL-2.0 8.2.9.5.2
> diff --git a/Documentation/cxl/partition-description.txt b/Documentation/cxl/partition-description.txt
> new file mode 100644
> index 0000000..b3efac8
> --- /dev/null
> +++ b/Documentation/cxl/partition-description.txt
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +DESCRIPTION
> +-----------
> +Partition the device into volatile and persistent capacity. The change
> +in partitioning will become the “next” configuration, to become active
> +on the next device reset.
> +
> +Use "cxl list -m <memdev> -P" to examine the partition capacities
> +supported on a device. Paritionable capacity must exist on the
> +device. A partition_alignment of zero means no partitionable
> +capacity is available.
> +
> +Using this command to change the size of the persistent capacity shall
> +result in the loss of data stored.
> diff --git a/Documentation/cxl/partition-options.txt b/Documentation/cxl/partition-options.txt
> new file mode 100644
> index 0000000..84e49c9
> --- /dev/null
> +++ b/Documentation/cxl/partition-options.txt
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +<memory device(s)>::
> +include::memdev-option.txt[]
> +
> +-s::
> +--size=::
> +       Size in bytes of the volatile partition requested.
> +
> +       Size must align to the devices partition_alignment.
> +       Use 'cxl list -m <memdev> -P' to find partition_alignment.
> +
> +       Size must be less than or equal to the devices partitionable bytes.
> +       (total_capacity - volatile_only_capacity - persistent_only_capacity)
> +       Use 'cxl list -m <memdev> -P' to find *_capacity values.
> +
> +-v::
> +       Turn on verbose debug messages in the library (if libcxl was built with
> +       logging and debug enabled).
> diff --git a/Documentation/cxl/Makefile.am b/Documentation/cxl/Makefile.am
> index efabaa3..c5faf04 100644
> --- a/Documentation/cxl/Makefile.am
> +++ b/Documentation/cxl/Makefile.am
> @@ -22,7 +22,8 @@ man1_MANS = \
>         cxl-list.1 \
>         cxl-read-labels.1 \
>         cxl-write-labels.1 \
> -       cxl-zero-labels.1
> +       cxl-zero-labels.1 \
> +       cxl-set-partition-info.1
>
>  EXTRA_DIST = $(man1_MANS)
>
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index 78eca6e..7f11f28 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -10,4 +10,5 @@ int cmd_read_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_init_labels(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_check_labels(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index 4b1661d..3153cf0 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
>         { "zero-labels", .c_fn = cmd_zero_labels },
>         { "read-labels", .c_fn = cmd_read_labels },
>         { "write-labels", .c_fn = cmd_write_labels },
> +       { "set-partition-info", .c_fn = cmd_set_partition_info },
>  };
>
>  int main(int argc, const char **argv)
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 5ee38e5..fa63317 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -6,6 +6,7 @@
>  #include <unistd.h>
>  #include <limits.h>
>  #include <util/log.h>
> +#include <util/size.h>
>  #include <util/filter.h>
>  #include <cxl/libcxl.h>
>  #include <util/parse-options.h>
> @@ -23,6 +24,7 @@ static struct parameters {
>         unsigned len;
>         unsigned offset;
>         bool verbose;
> +       unsigned long long volatile_size;
>  } param;
>
>  #define fail(fmt, ...) \
> @@ -47,6 +49,10 @@ OPT_UINTEGER('s', "size", &param.len, "number of label bytes to operate"), \
>  OPT_UINTEGER('O', "offset", &param.offset, \
>         "offset into the label area to start operation")
>
> +#define SET_PARTITION_OPTIONS() \
> +OPT_U64('s', "volatile_size",  &param.volatile_size, \
> +       "next volatile partition size in bytes")
> +
>  static const struct option read_options[] = {
>         BASE_OPTIONS(),
>         LABEL_OPTIONS(),
> @@ -67,6 +73,12 @@ static const struct option zero_options[] = {
>         OPT_END(),
>  };
>
> +static const struct option set_partition_options[] = {
> +       BASE_OPTIONS(),
> +       SET_PARTITION_OPTIONS(),
> +       OPT_END(),
> +};
> +
>  static int action_zero(struct cxl_memdev *memdev, struct action_context *actx)
>  {
>         size_t size;
> @@ -174,6 +186,73 @@ out:
>         return rc;
>  }
>
> +static int validate_partition(struct cxl_memdev *memdev,
> +               unsigned long long volatile_request)
> +{
> +       unsigned long long total_cap, volatile_only, persistent_only;
> +       unsigned long long partitionable_bytes, partition_align_bytes;
> +       const char *devname = cxl_memdev_get_devname(memdev);
> +       struct cxl_cmd *cmd;
> +       int rc;
> +
> +       cmd = cxl_cmd_new_identify(memdev);
> +       if (!cmd)
> +               return -ENXIO;
> +       rc = cxl_cmd_submit(cmd);
> +       if (rc < 0)
> +               goto err;
> +       rc = cxl_cmd_get_mbox_status(cmd);
> +       if (rc != 0)
> +               goto err;
> +
> +       partition_align_bytes = cxl_cmd_identify_get_partition_align(cmd);
> +       if (partition_align_bytes == 0) {
> +               fprintf(stderr, "%s: no partitionable capacity\n", devname);
> +               rc = -EINVAL;
> +               goto err;
> +       }
> +
> +       total_cap = cxl_cmd_identify_get_total_capacity(cmd);
> +       volatile_only = cxl_cmd_identify_get_volatile_only_capacity(cmd);
> +       persistent_only = cxl_cmd_identify_get_persistent_only_capacity(cmd);
> +
> +       partitionable_bytes = total_cap - volatile_only - persistent_only;
> +
> +       if (volatile_request > partitionable_bytes) {
> +               fprintf(stderr, "%s: volatile size %lld exceeds partitionable capacity %lld\n",
> +                       devname, volatile_request, partitionable_bytes);
> +               rc = -EINVAL;
> +               goto err;
> +       }
> +       if (!IS_ALIGNED(volatile_request, partition_align_bytes)) {
> +               fprintf(stderr, "%s: volatile size %lld is not partition aligned %lld\n",
> +                       devname, volatile_request, partition_align_bytes);
> +               rc = -EINVAL;
> +       }
> +err:
> +       cxl_cmd_unref(cmd);
> +       return rc;
> +}
> +
> +static int action_set_partition(struct cxl_memdev *memdev,
> +               struct action_context *actx)
> +{
> +       const char *devname = cxl_memdev_get_devname(memdev);
> +       unsigned long long volatile_request;
> +       int rc;
> +
> +       volatile_request = param.volatile_size;
> +
> +       rc = validate_partition(memdev, volatile_request);
> +       if (rc)
> +               return rc;
> +
> +       rc = cxl_memdev_set_partition_info(memdev, volatile_request, 0);
> +       if (rc)
> +               fprintf(stderr, "%s error: %s\n", devname, strerror(-rc));
> +       return rc;

One of the conventions from ndctl is that any command that modifies an
object should also emit the updated state of that object in JSON. For
example, "ndctl reconfigure-namespace" arranges for:

                unsigned long flags = UTIL_JSON_DAX | UTIL_JSON_DAX_DEVS;
                struct json_object *jndns;

                if (isatty(1))
                        flags |= UTIL_JSON_HUMAN;
                jndns = util_namespace_to_json(ndns, flags);
                if (jndns)
                        printf("%s\n", json_object_to_json_string_ext(jndns,
                                                JSON_C_TO_STRING_PRETTY));

...to dump the updated state of the namespace, so a similar
util_memdev_to_json() seems appropriate here. However, perhaps that
can come later. I have work-in-progress patches to move the core of
cxl/list.c to a cxl_filter_walk() helper that would allow you to just
call that to dump a listing for all the memdevs that were passed on
the command line.

Re: [ndctl PATCH 0/7] Add partitioning support for CXL memdevs

[Alison Schofield]

Thanks for the review Ira!
There's one question back at you wrt adding MAN page to cover letter.
Otherwise, I'm absorbing all your feedback.


On Thu, Jan 06, 2022 at 12:32:46PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:11PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
>  
> First, thanks for taking this on!  :-D
> 
> > To support changing partitions on CXL memdevs, first provide access to device
> > partitioning info.
> >
> 
> How about:
> 
> Users will want to configure CXL memdevs on CXL devices which support
> partitioning.  CXL provides get and set partition info mailbox command to do
> this.
> 
> Add support to retrieve partition info and set partition info.
> 
> ...
> 

Will reword, thanks.

> >
> >
> > The first 4 patches add accessors to all
> > the partition info a CXL command parser needs in order to validate
> > the command. This info is added to cxl list to assist the user in
> > creating valid partition requests.
> 
> Great but what is a valid partition request?
>

I didn't regurgitate the restrictions here in the cover letter, nor
in the patch commit logs. They are only visible in the MAN page, and
I reference the MAN page in the cxl set-partition-info command patch.
I could add a pretty version of the MAN page here to give the cover
letter more details.

Would you like to see the MAN page here?

> > 
> > # cxl list -MP
> > [
> >   {
> >     "memdev":"mem0",
> >     "pmem_size":0,
> >     "ram_size":273535729664,
> >     "partition":{
> >       "active_volatile_capacity":273535729664,
> >       "active_persistent_capacity":0,
> >       "next_volatile_capacity":268435456,
> >       "next_persistent_capacity":273267294208,
> >       "total_capacity":273535729664,
> >       "volatile_only_capacity":0,
> >       "persistent_only_capacity":0,
> >       "partition_alignment":268435456
> >     }
> >   }
> > ]
> > 
> > Next introduce libcxl ioctl() interfaces for the SET_PARTITION_INFO
> > mailbox command and the new CXL command. cxl-cli does the constraints
> > checking. It does not offer the IMMEDIATE mode option since we do not
> > have driver support for that yet.
> 
> How about something like 'cxl-cli restricts the use of the IMMEDIATE flag until
> such time as the driver supports it'?

Those words are really not true. cxl-cli doesn't restrict the flags use,
it simply does not offer the option.

> 
> But we probably should just let the command through and rely on the driver to
> do what it does...
> 
Nah. I don't think I should add -i --immediate to the command yet.

(I think we covered this in chatroom)

> > 
> > # cxl set-partition-info
> >  usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
> > 
> >     -v, --verbose         turn on debug
> >     -s, --volatile_size <n>
> >                           next volatile partition size in bytes
> > 
> > Guessing that
>   ^^^^^^^^^^^^^
>   Delete
> 
> 'A libcxl user can...'
> 
> >

Will delete.


> > a libcxl user could send the SET_PARTITION_INFO mailbox
> > command outside of cxl-cli tool, so a kernel patch that disables the
> > immediate bit, on the receiving end of the ioctl, follows.
> 
> cool!
> 
> > 
> > It may be simpler to block the immediate bit in the libcxl API today,
> > (and as I write this cover letter I'm wondering just how far this goes
> > astray ;)) However, the kernel patch to peek in the payload sets us on
> > the path of inspecting set-partition-info mailbox commands in the future,
> > when immediate mode support is required.
> 
> I'd just delete this.  I think it is best to leave the kernel to the
> enforcement and not complicate the user space.

got it.

> 
> Ira
> 
> > 
> > Testing - so far I've only tested w one memdev in a Simics env. So,
> > next will be growing that Simics config, using cxl_test env, and 
> > adding a unit test.
> > 
> > Alison Schofield (7):
> >   libcxl: add GET_PARTITION_INFO mailbox command and accessors
> >   libcxl: add accessors for capacity fields of the IDENTIFY command
> >   libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field
> >   cxl: add memdev partition information to cxl-list
> >   libcxl: add interfaces for SET_PARTITION_INFO mailbox command
> >   ndctl, util: use 'unsigned long long' type in OPT_U64 define
> >   cxl: add command set-partition-info
> > 
> >  Documentation/cxl/cxl-list.txt               |  23 ++++
> >  Documentation/cxl/cxl-set-partition-info.txt |  27 +++++
> >  Documentation/cxl/partition-description.txt  |  15 +++
> >  Documentation/cxl/partition-options.txt      |  19 +++
> >  Documentation/cxl/Makefile.am                |   3 +-
> >  cxl/builtin.h                                |   1 +
> >  cxl/lib/private.h                            |  19 +++
> >  cxl/libcxl.h                                 |  12 ++
> >  util/json.h                                  |   1 +
> >  util/parse-options.h                         |   2 +-
> >  util/size.h                                  |   1 +
> >  cxl/cxl.c                                    |   1 +
> >  cxl/lib/libcxl.c                             | 117 ++++++++++++++++++-
> >  cxl/lib/libcxl.sym                           |  11 ++
> >  cxl/list.c                                   |   5 +
> >  cxl/memdev.c                                 |  89 ++++++++++++++
> >  util/json.c                                  | 112 ++++++++++++++++++
> >  17 files changed, 455 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/cxl/cxl-set-partition-info.txt
> >  create mode 100644 Documentation/cxl/partition-description.txt
> >  create mode 100644 Documentation/cxl/partition-options.txt
> > 
> > -- 
> > 2.31.1
> > 

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Alison Schofield]

Thanks for the review Ira!

On Thu, Jan 06, 2022 at 12:19:07PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> > command output data structure (privately), and accessor APIs to return
> > the different fields in the partition info output.
> 
> I would rephrase this:
> 
> libcxl provides functions for C code to issue cxl mailbox commands as well as
> parse the output returned.  Get partition info should be part of this API.
> 
> Add the libcxl get partition info mailbox support as well as an API to parse
> the fields of the command returned.
> 
> All fields are specified in multiples of 256MB so also define a capacity
> multiplier to convert the raw data into bytes.
> 
Will do. Thanks.

> > 

snip.
 
> I also wonder if the conversion to bytes should be reflected in the function
> name.  Because returning 'cap' might imply to someone they are getting the raw
> data field.

Agree. Will s/cap/bytes

>

Some additional comments were addressed by Dan & Vishal responses.

Re: [ndctl PATCH 3/7] libcxl: apply CXL_CAPACITY_MULTIPLIER to partition alignment field

[Verma, Vishal L]

On Thu, 2022-01-06 at 12:40 -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:14PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > The IDENTIFY mailbox command returns the partition alignment field
> > expressed in multiples of 256 MB.
> 
> Interesting...
> 
> I don't think anyone is using this function just yet but this does technically
> change the behavior of this function.
> 
> Will that break anyone or cxl-cli?
> 
> > Use CXL_CAPACITY_MULTIPLIER when
> > returning this field.
> > 
> > This is in sync with all the other partitioning related fields using
> > the multiplier.
> 
> To me the fact that this was not in bytes implies that the original API of
> libcxl was intended to not convert these values.
> 
> Vishal may have an opinion.  Should these be in bytes or 'CXL Capacity' units
> (ie 256MB's)?
> 
> I think I prefer bytes as well.

Hm yeah there's not an internal (i.e. cxl-cli) user yet. I think bytes
makes sense. It is common for libcxl to convert spec-isms to whatever
makes sense at the library/cxl-cli level, and for that bytes makes
sense.

> 
> Ira
> 
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/libcxl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 715d8e4..85a6c0e 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1086,7 +1086,7 @@ CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
> >  	if (cmd->status < 0)
> >  		return cmd->status;
> >  
> > -	return le64_to_cpu(id->partition_align);
> > +	return le64_to_cpu(id->partition_align) * CXL_CAPACITY_MULTIPLIER;
> >  }
> >  
> >  CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
> > -- 
> > 2.31.1
> > 

Re: [ndctl PATCH 2/7] libcxl: add accessors for capacity fields of the IDENTIFY command

[Alison Schofield]

On Thu, Jan 06, 2022 at 12:36:39PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:13PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add accessors to retrieve total capacity, volatile only capacity,
> > and persistent only capacity from the IDENTIFY mailbox command.
> > These values are useful when partitioning the device.
> 
> Reword:
> 
> The total capacity, volatile only capacity, and persistent only capacity are
> required to properly formulate a set partition info command.
> 
> Provide functions to retrieve these values from the IDENTIFY command.  Like the
> partition information commands these return the values in bytes.
> 

Will reword. Thanks.

> > 
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_identify_get_total_capacity(struct cxl_cmd *cmd)
> 
> Is there someplace that all the libcxl functions are documented?  Like the
> other functions I would like to ensure the user knows these are returning
> values in bytes.

There is a libcxl manpage, source at:
ndctl/Documentation/cxl/lib/libcxl.txt

Synopsis is:
#include <cxl/libcxl.h>
cc ... -lcxl

It describes how to use libcxl, ie alloc, submit, and get info back from
a command. I believe the intent is the user references cxl/libcxl.h to
find the accessors available. Along that line, it doesn't make any
sweeping statements about formats of data returned and I believe, based
on Dan's comments about the long descriptive names, that is by design.
ie. the name should say it all.

I'll rename these all to be _bytes instead of _capacity, as you
suggested in the prior patch.

> 
> Ira
> 
snip
> > +{

Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Alison Schofield]

On Thu, Jan 06, 2022 at 01:57:59PM -0800, Vishal Verma wrote:
> 
> Just one nit here about the double verb 'get'. In such cases,
> get_partition_info can just become 'partition_info'
> 
> e.g.: cxl_cmd_partition_info_get_active_volatile_cap(...
> 

Will do - thanks Vishal!

Combiningg w Ira's feedback, it'll be:
cxl_cmd_partition_info_get_active_volatile_bytes(...

> >
> 

Re: [ndctl PATCH 4/7] cxl: add memdev partition information to cxl-list

[Alison Schofield]

On Thu, Jan 06, 2022 at 01:51:47PM -0800, Dan Williams wrote:
> On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Add information useful for managing memdev partitions to cxl-list
> > output. Include all of the fields from GET_PARTITION_INFO and the
> > partitioning related fields from the IDENTIFY mailbox command.
> >
> >     "partition":{
> 
> Perhaps call it "parition_info"?
> 
Got it!

> >       "active_volatile_capacity":273535729664,
> >       "active_persistent_capacity":0,
> >       "next_volatile_capacity":0,
> >       "next_persistent_capacity":0,
> >       "total_capacity":273535729664,
> >       "volatile_only_capacity":0,
> >       "persistent_only_capacity":0,
> >       "partition_alignment":268435456
> >     }
> >
> >    }
> >  ]
> >  ----
> > +-P::
> > +--partition::
> > +       Include partition information in the memdev listing. Example listing:
> 
> How about -I/--partition for partition "Info". I had earmarked -P for
> including "Port" object in the listing.

Sure. -I it is!

> 
> Other than that, looks good:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
snip

Re: [ndctl PATCH 4/7] cxl: add memdev partition information to cxl-list

[Alison Schofield]

On Thu, Jan 06, 2022 at 12:49:05PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:15PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> 
> "Users will want to be able to check the current partition information.  In
> addition they will need to know the capacity values to form a valid set
> partition information command."
>

I do see the pattern you are after. Problem statement separate from
solution statement. Will reword.

> > Add information useful for managing memdev partitions to cxl-list
>      ^
>    "optional"
> 
> > output. Include all of the fields from GET_PARTITION_INFO and the
> > partitioning related fields from the IDENTIFY mailbox command.
> > 
> 
> "Sample output for this section is:"
> 
> >     "partition":{
> >       "active_volatile_capacity":273535729664,
> >       "active_persistent_capacity":0,
> >       "next_volatile_capacity":0,
> >       "next_persistent_capacity":0,
> >       "total_capacity":273535729664,
> >       "volatile_only_capacity":0,
> >       "persistent_only_capacity":0,
> >       "partition_alignment":268435456
> >     }
> > 

snip

> > +	if (jobj)
> > +		json_object_object_add(jpart, "partition_alignment", jobj);
> > +
> > +	return jpart;
> > +
> > +err_cmd:
> 
> Doesn't this need to be called always, not just on error?

cxl_cmd_unref(), sure does. Thanks for the catch!

> 
> Ira
> 

Re: [ndctl PATCH 6/7] ndctl, util: use 'unsigned long long' type in OPT_U64 define

[Alison Schofield]

On Thu, Jan 06, 2022 at 12:54:54PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:17PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > The OPT_U64 define failed in check_vtype() with unknown 'u64' type.
> > Replace with 'unsigned long long' to make the OPT_U64 define usable.
> 
> I feel like this should be the first patch in the series.

I felt like it was a fixup, that should go right before I use it.

Now that the -size parameter is getting changed to a string,
(next patch feedback), this isn't needed.

I'll drop this patch from the set and save it for trivial cleanup
day.

more below...
> 
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  util/parse-options.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/util/parse-options.h b/util/parse-options.h
> > index 9318fe7..91b7932 100644
> > --- a/util/parse-options.h
> > +++ b/util/parse-options.h
> > @@ -124,7 +124,7 @@ struct option {
> >  #define OPT_INTEGER(s, l, v, h)     { .type = OPTION_INTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h) }
> >  #define OPT_UINTEGER(s, l, v, h)    { .type = OPTION_UINTEGER, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned int *), .help = (h) }
> >  #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
> > -#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
> > +#define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned long long *), .help = (h) }
> 
> Why can't this be uint64_t?

I don't know. ULL worked so I didn't look further.
Is uint64_t more suitable?

> 
> Ira
> 
> >  #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
> >  #define OPT_FILENAME(s, l, v, a, h) { .type = OPTION_FILENAME, .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
> >  #define OPT_DATE(s, l, v, h) \
> > -- 
> > 2.31.1
> > 

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Alison Schofield]

On Thu, Jan 06, 2022 at 02:19:08PM -0800, Dan Williams wrote:
> On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
> >

snip

> 
> One of the conventions from ndctl is that any command that modifies an
> object should also emit the updated state of that object in JSON. For
> example, "ndctl reconfigure-namespace" arranges for:
> 
>                 unsigned long flags = UTIL_JSON_DAX | UTIL_JSON_DAX_DEVS;
>                 struct json_object *jndns;
> 
>                 if (isatty(1))
>                         flags |= UTIL_JSON_HUMAN;
>                 jndns = util_namespace_to_json(ndns, flags);
>                 if (jndns)
>                         printf("%s\n", json_object_to_json_string_ext(jndns,
>                                                 JSON_C_TO_STRING_PRETTY));
> 
> ...to dump the updated state of the namespace, so a similar
> util_memdev_to_json() seems appropriate here. However, perhaps that
> can come later. I have work-in-progress patches to move the core of
> cxl/list.c to a cxl_filter_walk() helper that would allow you to just
> call that to dump a listing for all the memdevs that were passed on
> the command line.

Will add. Thanks!

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Alison Schofield]

On Thu, Jan 06, 2022 at 01:35:03PM -0800, Dan Williams wrote:
> On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
> >
> >         unsigned offset;
> >         bool verbose;
> > +       unsigned long long volatile_size;
> 
> This should be a string.
> 
> See parse_size64() that handles suffixes like K,M,G,T, so you can do
> things like "cxl set-partition -s 256M" and behave like the other
> "--size" options in ndctl.

Got it. Thanks! This will fix a couple of pesky UI things I was
worried about.

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Alison Schofield]

On Thu, Jan 06, 2022 at 01:05:26PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:18PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> 
> "User will need a command line option for setting partition info.  Add
> 'set-partition-info' to the cxl command line tool."
> 
> > The command 'cxl set-partition-info' operates on a CXL memdev,
> > or a set of memdevs, allowing the user to change the partition
> > layout of the device.
>                 ^^^^^^
> 		device(s).
> 

Got it. Thanks!

> > 
snip

> 
> Did I miss the update to the cxl-list command documentation?
You must've blinked ;)
See Patch 4 - cxl: add memdev partition information to cxl-list

> 
snip
>
> > +
> > +	rc = cxl_memdev_set_partition_info(memdev, volatile_request, 0);
> 
> CXL_CMD_SET_PARTITION_INFO_NO_FLAG?

Yeah...I couldn't get to that at the last minute. 
Will figure that out and use it here.
Thanks!

> 
> Ira

Re: [ndctl PATCH 5/7] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[Alison Schofield]

On Thu, Jan 06, 2022 at 12:53:02PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:16PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add APIs to allocate and send a SET_PARTITION_INFO mailbox command.
> > 
> > +	le64 volatile_capacity;
> > +	u8 flags;
> > +} __attribute__((packed));
> > +
> > +/* CXL 2.0 8.2.9.5.2 Set Partition Info */
> > +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG				(0)
> > +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG			(1)
> 
> BIT(0) and BIT(1)?
> 
> I can't remember which bit is the immediate flag.
> 
Immediate flag is BIT(0).
Seemed awkward/overkill to use bit macro -
+#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG				(0)
+#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG			BIT(1)

I just added api to use this so you'll see it in action in v2
of this patchset and can comment again.

Re: [ndctl PATCH 5/7] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[Dan Williams]

On Fri, Jan 7, 2022 at 5:46 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> On Thu, Jan 06, 2022 at 12:53:02PM -0800, Ira Weiny wrote:
> > On Mon, Jan 03, 2022 at 12:16:16PM -0800, Schofield, Alison wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > Add APIs to allocate and send a SET_PARTITION_INFO mailbox command.
> > >
> > > +   le64 volatile_capacity;
> > > +   u8 flags;
> > > +} __attribute__((packed));
> > > +
> > > +/* CXL 2.0 8.2.9.5.2 Set Partition Info */
> > > +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG                         (0)
> > > +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG                  (1)
> >
> > BIT(0) and BIT(1)?
> >
> > I can't remember which bit is the immediate flag.
> >
> Immediate flag is BIT(0).
> Seemed awkward/overkill to use bit macro -
> +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG                             (0)
> +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG                      BIT(1)
>
> I just added api to use this so you'll see it in action in v2
> of this patchset and can comment again.

Why is a "no flag" definition needed? Isn't that just "!IMMEDIATE"

Also BIT(1) == 0x2, so that should be BIT(0), right?

Re: [ndctl PATCH 5/7] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[Alison Schofield]

On Fri, Jan 07, 2022 at 06:27:40PM -0800, Dan Williams wrote:
> On Fri, Jan 7, 2022 at 5:46 PM Alison Schofield
> <alison.schofield@intel.com> wrote:
> >
> > On Thu, Jan 06, 2022 at 12:53:02PM -0800, Ira Weiny wrote:
> > > On Mon, Jan 03, 2022 at 12:16:16PM -0800, Schofield, Alison wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > >
> > > > Add APIs to allocate and send a SET_PARTITION_INFO mailbox command.
> > > >
> > > > +   le64 volatile_capacity;
> > > > +   u8 flags;
> > > > +} __attribute__((packed));
> > > > +
> > > > +/* CXL 2.0 8.2.9.5.2 Set Partition Info */
> > > > +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG                         (0)
> > > > +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG                  (1)
> > >
> > > BIT(0) and BIT(1)?
> > >
> > > I can't remember which bit is the immediate flag.
> > >
> > Immediate flag is BIT(0).
> > Seemed awkward/overkill to use bit macro -
> > +#define CXL_CMD_SET_PARTITION_INFO_NO_FLAG                             (0)
> > +#define CXL_CMD_SET_PARTITION_INFO_IMMEDIATE_FLAG                      BIT(1)
> >
> > I just added api to use this so you'll see it in action in v2
> > of this patchset and can comment again.
> 
> Why is a "no flag" definition needed? Isn't that just "!IMMEDIATE"
>
You are right. The no flag set case is !IMMEDIATE.

>Also BIT(1) == 0x2, so that should be BIT(0), right?
Yes IMMEDIATE FLAG IS BIT(0). 

This chatter is related to using something more descriptive that '0'
for !IMMEDIATE when the cxl command makes the call to the api. (Patch 7)
I added a new accessor in v2 that returns the bit.

Re: [ndctl PATCH 7/7] cxl: add command set-partition-info

[Alison Schofield]

On Fri, Jan 07, 2022 at 02:45:15PM -0800, Alison Schofield wrote:
> On Thu, Jan 06, 2022 at 02:19:08PM -0800, Dan Williams wrote:
> > On Mon, Jan 3, 2022 at 12:11 PM <alison.schofield@intel.com> wrote:
> > >
> 
> snip
> 
> > 
> > One of the conventions from ndctl is that any command that modifies an
> > object should also emit the updated state of that object in JSON. For
> > example, "ndctl reconfigure-namespace" arranges for:
> > 
> >                 unsigned long flags = UTIL_JSON_DAX | UTIL_JSON_DAX_DEVS;
> >                 struct json_object *jndns;
> > 
> >                 if (isatty(1))
> >                         flags |= UTIL_JSON_HUMAN;
> >                 jndns = util_namespace_to_json(ndns, flags);
> >                 if (jndns)
> >                         printf("%s\n", json_object_to_json_string_ext(jndns,
> >                                                 JSON_C_TO_STRING_PRETTY));
> > 
> > ...to dump the updated state of the namespace, so a similar
> > util_memdev_to_json() seems appropriate here. However, perhaps that
> > can come later. I have work-in-progress patches to move the core of
> > cxl/list.c to a cxl_filter_walk() helper that would allow you to just
> > call that to dump a listing for all the memdevs that were passed on
> > the command line.
> 
> Will add. Thanks!
Oops...should've said - 'Will wait for helper.'