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

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

[alison.schofield]

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

Users may want to view and change partition layouts of CXL memory
devices that support partitioning. Provide userspace access to
the device partitioning capabilities as defined in the CXL 2.0
specification.

The first 4 patches add accessors for all the information needed
to formulate a new partition layout. This info is accessible via
the libcxl API and a new option in the cxl list command:

    "partition_info":{
      "active_volatile_bytes":273535729664,
      "active_persistent_bytes":0,
      "next_volatile_bytes":268435456,
      "next_persistent_bytes":273267294208,
      "total_bytes":273535729664,
      "volatile_only_bytes":0,
      "persistent_only_bytes":0,
      "partition_alignment_bytes":268435456
    }

Patch 5 introduces the libcxl interfaces for the SET_PARTITION_INFO
mailbox command and Patch 6 adds the new CXL command: 

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

-t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
-s, --size=<size>       size in bytes (Default: all partitionable capacity)
-a, --align             allow alignment correction
-v, --verbose           turn on debug

The CXL command does not offer the IMMEDIATE mode option defined
in the CXL 2.0 spec because the CXL kernel driver does not support
immediate mode yet. Since userspace could use the libcxl API to
send a SET_PARTITION_INFO mailbox command with immediate mode
selected, a kernel patch that rejects such requests is in review
for the CXL driver.

Changes in v4: (from Dan's review)
- cxl set-partition command: add type (pmem | volatile),
  add defaults for type and size, and add an align option.
- Replace macros with return statements with functions.
- Add cxl_set_partition_set_mode() to the libcxl API.
- Add API documentation to Documentation/cxl/lib/libcxl.txt.
- Use log_err/info mechanism.
- Display memdev JSON info upon completion of set-partition command.
- Remove unneeded casts when accessing command payloads.
- Name changes - like drop _info suffix, use _size instead of _bytes.

Changes in v3:
- Back out the API name change to the partition align accessor.
  The API was already released in v72.
- Man page: collapse into one .txt file. 
- Man page: add to Documentation/meson.build 

Changes in v2:
- Rebase onto https://github.com/pmem/ndctl.git djbw/meson.
- Clarify that capacities are reported in bytes. (Ira)
  This led to renaming accessors like *_capacity to  *_bytes and 
  a spattering of changes in the man pages and commit messages.
- Add missing cxl_cmd_unref() when creating json objects. (Ira)
- Change the cxl list option to: -I --partition (Dan) 
- Use OPT_STRING for the --volatile_size parameter (Dan, Vishal)
- Drop extra _get_ in accessor names. (Vishal)
- Add an accessor for the SET_PARTITION_INFO mbox cmd flag.
- Display usage_with_options if size parameter is missing.
- Drop the OPT64 define patch. Use OPT_STRING instead.

Alison Schofield (6):
  libcxl: add GET_PARTITION_INFO mailbox command and accessors
  libcxl: add accessors for capacity fields of the IDENTIFY command
  libcxl: return the partition alignment field in bytes
  cxl: add memdev partition information to cxl-list
  libcxl: add interfaces for SET_PARTITION_INFO mailbox command
  cxl: add command set-partition

 Documentation/cxl/cxl-list.txt          |  23 +++
 Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
 Documentation/cxl/lib/libcxl.txt        |   5 +
 Documentation/cxl/meson.build           |   1 +
 cxl/builtin.h                           |   1 +
 cxl/cxl.c                               |   1 +
 cxl/filter.c                            |   2 +
 cxl/filter.h                            |   1 +
 cxl/json.c                              | 113 ++++++++++++++
 cxl/lib/libcxl.c                        | 123 ++++++++++++++-
 cxl/lib/libcxl.sym                      |  10 ++
 cxl/lib/private.h                       |  18 +++
 cxl/libcxl.h                            |  18 +++
 cxl/list.c                              |   2 +
 cxl/memdev.c                            | 196 ++++++++++++++++++++++++
 util/json.h                             |   1 +
 util/size.h                             |   1 +
 17 files changed, 575 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/cxl-set-partition.txt

-- 
2.31.1

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

[alison.schofield]

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

Users need access to the CXL GET_PARTITION_INFO mailbox command
to inspect and confirm changes to the partition layout of a memory
device.

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.

Per the CXL 2.0 specification, devices report partition capacities
as multiples of 256MB. Define and use a capacity multiplier to
convert the raw data into bytes for user consumption. Use byte
format as the norm for all capacity values produced or consumed
using CXL Mailbox commands.

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

diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index 4392b47..a6986ab 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -131,6 +131,7 @@ 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);
+struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
 
 ----
 
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index e0b443f..33cf06b 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1985,6 +1985,12 @@ static int cxl_cmd_validate_status(struct cxl_cmd *cmd, u32 id)
 	return 0;
 }
 
+static unsigned long long
+capacity_to_bytes(unsigned long long size)
+{
+	return le64_to_cpu(size) * CXL_CAPACITY_MULTIPLIER;
+}
+
 /* Helpers for health_info fields (no endian conversion) */
 #define cmd_get_field_u8(cmd, n, N, field)				\
 do {									\
@@ -2371,6 +2377,57 @@ 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(struct cxl_memdev *memdev)
+{
+	return cxl_cmd_new_generic(memdev,
+				   CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
+}
+
+static struct cxl_cmd_get_partition *
+cmd_to_get_partition(struct cxl_cmd *cmd)
+{
+	if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_GET_PARTITION_INFO))
+		return NULL;
+
+	return cmd->output_payload;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_get_partition *c;
+
+	c = cmd_to_get_partition(cmd);
+	return c ? capacity_to_bytes(c->active_volatile) : ULLONG_MAX;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_get_active_persistent_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_get_partition *c;
+
+	c = cmd_to_get_partition(cmd);
+	return c ? capacity_to_bytes(c->active_persistent) : ULLONG_MAX;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_get_next_volatile_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_get_partition *c;
+
+	c = cmd_to_get_partition(cmd);
+	return c ? capacity_to_bytes(c->next_volatile) : ULLONG_MAX;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_get_next_persistent_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_get_partition *c;
+
+	c = cmd_to_get_partition(cmd);
+	return c ? capacity_to_bytes(c->next_persistent) : ULLONG_MAX;
+}
+
 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 e56a2bf..509e62d 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -155,4 +155,9 @@ global:
 	cxl_dport_get_port;
 	cxl_port_get_dport_by_memdev;
 	cxl_dport_maps_memdev;
+	cxl_cmd_new_get_partition;
+	cxl_cmd_partition_get_active_volatile_size;
+	cxl_cmd_partition_get_active_persistent_size;
+	cxl_cmd_partition_get_next_volatile_size;
+	cxl_cmd_partition_get_next_persistent_size;
 } LIBCXL_1;
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index f483c30..7f3a562 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")))
 
@@ -185,6 +186,15 @@ struct cxl_cmd_get_health_info {
 	le32 pmem_errors;
 } __attribute__((packed));
 
+struct cxl_cmd_get_partition {
+	le64 active_volatile;
+	le64 active_persistent;
+	le64 next_volatile;
+	le64 next_persistent;
+} __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 3b2293b..2c0a8d1 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -242,6 +242,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(struct cxl_memdev *memdev);
+unsigned long long cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_get_active_persistent_size(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_get_next_volatile_size(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_get_next_persistent_size(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
 
-- 
2.31.1

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

[alison.schofield]

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

Users need access to a few additional fields reported by the IDENTIFY
mailbox command: total, volatile_only, and persistent_only capacities.
These values are useful when defining partition layouts.

Add accessors to the libcxl API to retrieve these values from the
IDENTIFY command.

The fields are specified in multiples of 256MB per the CXL 2.0 spec.
Use the capacity multiplier to convert the raw data into bytes for user
consumption.

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

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 33cf06b..e9d7762 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -2322,6 +2322,42 @@ CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
 	return le32_to_cpu(id->lsa_size);
 }
 
+static struct cxl_cmd_identify *
+cmd_to_identify(struct cxl_cmd *cmd)
+{
+	if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_IDENTIFY))
+		return NULL;
+
+	return cmd->output_payload;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_total_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_identify *c;
+
+	c = cmd_to_identify(cmd);
+	return c ? capacity_to_bytes(c->total_capacity) : ULLONG_MAX;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_volatile_only_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_identify *c;
+
+	c = cmd_to_identify(cmd);
+	return c ? capacity_to_bytes(c->volatile_capacity) : ULLONG_MAX;
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd)
+{
+	struct cxl_cmd_identify *c;
+
+	c = cmd_to_identify(cmd);
+	return c ? capacity_to_bytes(c->persistent_capacity) : ULLONG_MAX;
+}
+
 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 509e62d..5ac6e9b 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -160,4 +160,7 @@ global:
 	cxl_cmd_partition_get_active_persistent_size;
 	cxl_cmd_partition_get_next_volatile_size;
 	cxl_cmd_partition_get_next_persistent_size;
+	cxl_cmd_identify_get_total_size;
+	cxl_cmd_identify_get_volatile_only_size;
+	cxl_cmd_identify_get_persistent_only_size;
 } LIBCXL_1;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 2c0a8d1..6e18e84 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -201,6 +201,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_size(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_volatile_only_size(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_identify_get_partition_align(struct cxl_cmd *cmd);
 unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd);
 struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
-- 
2.31.1

[ndctl PATCH v4 3/6] libcxl: return the partition alignment field in bytes

[alison.schofield]

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

Per the CXL specification, the partition alignment field reports
the alignment value in multiples of 256MB. In the libcxl API, values
for all capacity fields are defined to return bytes.

Update the partition alignment accessor to return bytes so that it
is in sync with other capacity related fields.

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 e9d7762..307e5c4 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -2306,7 +2306,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 capacity_to_bytes(id->partition_align);
 }
 
 CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
-- 
2.31.1

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

[alison.schofield]

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

Users may want to check the partition information of a memory device
using the CXL command line tool. This is useful for understanding the
active, as well as creating the next, partition layout.

Add an option to the 'cxl list' command to display partition information.
Include all of the fields from GET_PARTITION_INFO and the partitioning
related fields from the IDENTIFY mailbox command.

Example:
    "partition_info":{
      "active_volatile_size":273535729664,
      "active_persistent_size":0,
      "next_volatile_size":0,
      "next_persistent_size":0,
      "total_size":273535729664,
      "volatile_only_size":0,
      "persistent_only_size":0,
      "partition_alignment_size":268435456
    }

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/cxl/cxl-list.txt |  23 +++++++
 cxl/filter.c                   |   2 +
 cxl/filter.h                   |   1 +
 cxl/json.c                     | 113 +++++++++++++++++++++++++++++++++
 cxl/list.c                     |   2 +
 util/json.h                    |   1 +
 6 files changed, 142 insertions(+)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index 90e6d9f..86fc4e7 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -196,6 +196,29 @@ OPTIONS
   }
 ]
 ----
+-I::
+--partition::
+	Include partition information in the memdev listing. Example listing:
+----
+# cxl list -m mem0 -I
+[
+  {
+    "memdev":"mem0",
+    "pmem_size":0,
+    "ram_size":273535729664,
+    "partition_info":{
+      "active_volatile_size":273535729664,
+      "active_persistent_size":0,
+      "next_volatile_size":0,
+      "next_persistent_size":0,
+      "total_size":273535729664,
+      "volatile_only_size":0,
+      "persistent_only_size":0,
+      "partition_alignment_size":268435456
+    }
+  }
+]
+----
 
 -B::
 --buses::
diff --git a/cxl/filter.c b/cxl/filter.c
index 925bf3a..b339642 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -581,6 +581,8 @@ static unsigned long params_to_flags(struct cxl_filter_params *param)
 		flags |= UTIL_JSON_HEALTH;
 	if (param->targets)
 		flags |= UTIL_JSON_TARGETS;
+	if (param->partition)
+		flags |= UTIL_JSON_PARTITION;
 	return flags;
 }
 
diff --git a/cxl/filter.h b/cxl/filter.h
index 5deabb3..697b777 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -23,6 +23,7 @@ struct cxl_filter_params {
 	bool idle;
 	bool human;
 	bool health;
+	bool partition;
 	struct log_ctx ctx;
 };
 
diff --git a/cxl/json.c b/cxl/json.c
index f3b536e..69671b3 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -185,6 +185,114 @@ 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(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_partition_get_active_volatile_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_volatile_size", jobj);
+	}
+	cap = cxl_cmd_partition_get_active_persistent_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_persistent_size", jobj);
+	}
+	cap = cxl_cmd_partition_get_next_volatile_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_volatile_size", jobj);
+	}
+	cap = cxl_cmd_partition_get_next_persistent_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_persistent_size", 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_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart, "total_size", jobj);
+	}
+	cap = cxl_cmd_identify_get_volatile_only_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"volatile_only_size", jobj);
+	}
+	cap = cxl_cmd_identify_get_persistent_only_size(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"persistent_only_size", 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_size", jobj);
+
+	cxl_cmd_unref(cmd);
+	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)
 {
@@ -239,6 +347,11 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 			json_object_object_add(jdev, "state", jobj);
 	}
 
+	if (flags & UTIL_JSON_PARTITION) {
+		jobj = util_cxl_memdev_partition_to_json(memdev, flags);
+		if (jobj)
+			json_object_object_add(jdev, "partition_info", jobj);
+	}
 	return jdev;
 }
 
diff --git a/cxl/list.c b/cxl/list.c
index de96ff9..1e9d441 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -48,6 +48,8 @@ static const struct option options[] = {
 		    "use human friendly number formats "),
 	OPT_BOOLEAN('H', "health", &param.health,
 		    "include memory device health information "),
+	OPT_BOOLEAN('I', "partition", &param.partition,
+		    "include memory device partition information "),
 #ifdef ENABLE_DEBUG
 	OPT_BOOLEAN(0, "debug", &debug, "debug list walk"),
 #endif
diff --git a/util/json.h b/util/json.h
index e026df1..73bb9f0 100644
--- a/util/json.h
+++ b/util/json.h
@@ -19,6 +19,7 @@ enum util_json_flags {
 	UTIL_JSON_DAX_MAPPINGS	= (1 << 9),
 	UTIL_JSON_HEALTH	= (1 << 10),
 	UTIL_JSON_TARGETS	= (1 << 11),
+	UTIL_JSON_PARTITION	= (1 << 12),
 };
 
 void util_display_json_array(FILE *f_out, struct json_object *jarray,
-- 
2.31.1

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

[alison.schofield]

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

Users may want the ability to change the partition layout of a CXL
memory device.

Add interfaces to libcxl to allocate and send a SET_PARTITION_INFO
mailbox command as defined in the CXL 2.0 specification.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/lib/libcxl.txt |  4 ++++
 cxl/lib/libcxl.c                 | 28 ++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym               |  2 ++
 cxl/lib/private.h                |  8 ++++++++
 cxl/libcxl.h                     | 10 ++++++++++
 5 files changed, 52 insertions(+)

diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index a6986ab..301b4d7 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -132,6 +132,8 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 			   size_t offset);
 struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
+struct cxl_cmd *cxl_cmd_new_set_partition(struct cxl_memdev *memdev,
+					  unsigned long long volatile_size);
 
 ----
 
@@ -148,6 +150,8 @@ this sub-class of interfaces, there are:
    a CXL standard opcode. See the potential command ids in
    /usr/include/linux/cxl_mem.h.
 
+ * 'cxl_cmd_<name>_set_<field>' interfaces that set specific fields in a cxl_cmd
+
  * 'cxl_cmd_submit' which submits the command via ioctl()
 
  * 'cxl_cmd_<name>_get_<field>' interfaces that get specific fields out of the
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 307e5c4..3f04421 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -2464,6 +2464,34 @@ cxl_cmd_partition_get_next_persistent_size(struct cxl_cmd *cmd)
 	return c ? capacity_to_bytes(c->next_persistent) : ULLONG_MAX;
 }
 
+CXL_EXPORT int cxl_cmd_partition_set_mode(struct cxl_cmd *cmd,
+		enum cxl_setpartition_mode mode)
+{
+	struct cxl_cmd_set_partition *setpart = cmd->input_payload;
+
+	if (mode == CXL_SETPART_IMMEDIATE)
+		setpart->flags = CXL_CMD_SET_PARTITION_FLAG_IMMEDIATE;
+	else
+		setpart->flags = !CXL_CMD_SET_PARTITION_FLAG_IMMEDIATE;
+
+	return 0;
+}
+
+CXL_EXPORT struct cxl_cmd *cxl_cmd_new_set_partition(struct cxl_memdev *memdev,
+		unsigned long long volatile_size)
+{
+	struct cxl_cmd_set_partition *setpart;
+	struct cxl_cmd *cmd;
+
+	cmd = cxl_cmd_new_generic(memdev,
+			CXL_MEM_COMMAND_ID_SET_PARTITION_INFO);
+
+	setpart = cmd->input_payload;
+	setpart->volatile_size = cpu_to_le64(volatile_size)
+					/ CXL_CAPACITY_MULTIPLIER;
+	return cmd;
+}
+
 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 5ac6e9b..aab1112 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -163,4 +163,6 @@ global:
 	cxl_cmd_identify_get_total_size;
 	cxl_cmd_identify_get_volatile_only_size;
 	cxl_cmd_identify_get_persistent_only_size;
+	cxl_cmd_new_set_partition;
+	cxl_cmd_partition_set_mode;
 } LIBCXL_1;
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 7f3a562..c6d88f7 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -195,6 +195,14 @@ struct cxl_cmd_get_partition {
 
 #define CXL_CAPACITY_MULTIPLIER		SZ_256M
 
+struct cxl_cmd_set_partition {
+	le64 volatile_size;
+	u8 flags;
+} __attribute__((packed));
+
+/* CXL 2.0 8.2.9.5.2 Set Partition Info */
+#define CXL_CMD_SET_PARTITION_FLAG_IMMEDIATE				BIT(0)
+
 /* 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 6e18e84..0063d31 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -250,6 +250,16 @@ unsigned long long cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cm
 unsigned long long cxl_cmd_partition_get_active_persistent_size(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_partition_get_next_volatile_size(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_partition_get_next_persistent_size(struct cxl_cmd *cmd);
+struct cxl_cmd *cxl_cmd_new_set_partition(struct cxl_memdev *memdev,
+		unsigned long long volatile_size);
+
+enum cxl_setpartition_mode {
+	CXL_SETPART_NEXTBOOT,
+	CXL_SETPART_IMMEDIATE,
+};
+
+int cxl_cmd_partition_set_mode(struct cxl_cmd *cmd,
+		enum cxl_setpartition_mode mode);
 
 #ifdef __cplusplus
 } /* extern "C" */
-- 
2.31.1

[ndctl PATCH v4 6/6] cxl: add command set-partition

[alison.schofield]

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

CXL devices may support both volatile and persistent memory capacity.
The amount of device capacity set aside for each type is typically
established at the factory, but some devices also allow for dynamic
re-partitioning, add a command for this purpose.

Synopsis:

cxl set-partition <mem0> [<mem1>..<memN>] [<options>]

-t, --type=<type>	'pmem' or 'volatile' (Default: 'pmem')
-s, --size=<size>	size in bytes (Default: all partitionable capacity)
-a, --align      	allow alignment correction
-v, --verbose    	turn on debug

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
 Documentation/cxl/meson.build           |   1 +
 cxl/builtin.h                           |   1 +
 cxl/cxl.c                               |   1 +
 cxl/memdev.c                            | 196 ++++++++++++++++++++++++
 5 files changed, 259 insertions(+)
 create mode 100644 Documentation/cxl/cxl-set-partition.txt

diff --git a/Documentation/cxl/cxl-set-partition.txt b/Documentation/cxl/cxl-set-partition.txt
new file mode 100644
index 0000000..e20afba
--- /dev/null
+++ b/Documentation/cxl/cxl-set-partition.txt
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-set-partition(1)
+====================
+
+NAME
+----
+cxl-set-partition - set the partitioning between volatile and persistent capacity on a CXL memdev
+
+SYNOPSIS
+--------
+[verse]
+'cxl set-partition <mem> [ [<mem1>..<memN>] [<options>]'
+
+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> -I" to examine the partitioning capabilities
+of a device. A partition_alignment_bytes value of zero means there are
+no partitionable bytes available and therefore the partitions cannot be
+changed.
+
+Using this command to change the size of the persistent capacity shall
+result in the loss of data stored.
+
+OPTIONS
+-------
+<memory device(s)>::
+include::memdev-option.txt[]
+
+-t::
+--type=::
+	Type of partition, 'pmem' or 'volatile', to create.
+	Default: 'pmem'
+
+-s::
+--size=::
+	Size of the <type> partition in bytes. Size must align to the
+	devices alignment size. Use 'cxl list -m <memdev> -I' to find
+	the 'partition_alignment_size', or, use the 'align' option.
+	Default: All partitionable capacity is assigned to <type>.
+
+-a::
+--align::
+	This option allows the size of the partition to be increased to
+	meet device alignment requirements.
+
+-v::
+        Turn on verbose debug messages in the library (if libcxl was built with
+        logging and debug enabled).
+
+include::../copyright.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1],
+CXL-2.0 8.2.9.5.2
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index 96f4666..e927644 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -34,6 +34,7 @@ cxl_manpages = [
   'cxl-disable-memdev.txt',
   'cxl-enable-port.txt',
   'cxl-disable-port.txt',
+  'cxl-set-partition.txt',
 ]
 
 foreach man : cxl_manpages
diff --git a/cxl/builtin.h b/cxl/builtin.h
index 3123d5e..7bbad98 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -14,4 +14,5 @@ int cmd_disable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_enable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_disable_port(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_enable_port(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_set_partition(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index c20c569..ab4bbec 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -68,6 +68,7 @@ static struct cmd_struct commands[] = {
 	{ "enable-memdev", .c_fn = cmd_enable_memdev },
 	{ "disable-port", .c_fn = cmd_disable_port },
 	{ "enable-port", .c_fn = cmd_enable_port },
+	{ "set-partition", .c_fn = cmd_set_partition },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 90b33e1..5d97610 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -6,11 +6,14 @@
 #include <unistd.h>
 #include <limits.h>
 #include <util/log.h>
+#include <util/json.h>
+#include <util/size.h>
 #include <cxl/libcxl.h>
 #include <util/parse-options.h>
 #include <ccan/minmax/minmax.h>
 #include <ccan/array_size/array_size.h>
 
+#include "json.h"
 #include "filter.h"
 
 struct action_context {
@@ -26,6 +29,9 @@ static struct parameters {
 	bool verbose;
 	bool serial;
 	bool force;
+	bool align;
+	const char *type;
+	const char *size;
 } param;
 
 static struct log_ctx ml;
@@ -51,6 +57,14 @@ OPT_UINTEGER('O', "offset", &param.offset, \
 OPT_BOOLEAN('f', "force", &param.force,                                \
 	    "DANGEROUS: override active memdev safety checks")
 
+#define SET_PARTITION_OPTIONS() \
+OPT_STRING('t', "type",  &param.type, "type",			\
+	"'pmem' or 'volatile' (Default: 'pmem')"),		\
+OPT_STRING('s', "size",  &param.size, "size",			\
+	"size in bytes (Default: all partitionable capacity)"),	\
+OPT_BOOLEAN('a', "align",  &param.align,			\
+	"allow alignment correction")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
 	LABEL_OPTIONS(),
@@ -82,6 +96,12 @@ static const struct option enable_options[] = {
 	OPT_END(),
 };
 
+static const struct option set_partition_options[] = {
+	BASE_OPTIONS(),
+	SET_PARTITION_OPTIONS(),
+	OPT_END(),
+};
+
 static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
 {
 	if (!cxl_memdev_is_enabled(memdev))
@@ -209,6 +229,171 @@ out:
 	return rc;
 }
 
+static unsigned long long
+partition_align(const char *devname, unsigned long long volatile_size,
+		unsigned long long alignment, unsigned long long partitionable)
+{
+	if (IS_ALIGNED(volatile_size, alignment))
+		return volatile_size;
+
+	if (!param.align) {
+		log_err(&ml, "%s: size %lld is not partition aligned %lld\n",
+			devname, volatile_size, alignment);
+		return ULLONG_MAX;
+	}
+
+	/* Align based on partition type to fulfill users size request */
+	if (strcmp(param.type, "pmem") == 0)
+		volatile_size = ALIGN_DOWN(volatile_size, alignment);
+	else
+		volatile_size = ALIGN(volatile_size, alignment);
+
+	/* Fail if the align pushes size over the partitionable limit. */
+	if (volatile_size > partitionable) {
+		log_err(&ml, "%s: aligned partition size %lld exceeds partitionable size %lld\n",
+			devname, volatile_size, partitionable);
+		volatile_size = ULLONG_MAX;
+	}
+
+	return volatile_size;
+}
+
+static unsigned long long
+param_size_to_volatile_size(const char *devname, unsigned long long size,
+		unsigned long long partitionable)
+{
+	/* User omits size option. Apply all partitionable capacity to type. */
+	if (size == ULLONG_MAX)
+		return (strcmp(param.type, "pmem") == 0) ? 0 : partitionable;
+
+	/* User includes a size option. Apply it to type */
+	if (size > partitionable) {
+		log_err(&ml, "%s: %lld exceeds partitionable capacity %lld\n",
+			devname, size, partitionable);
+			return ULLONG_MAX;
+	}
+	return (strcmp(param.type, "pmem") == 0) ? partitionable - size : size;
+}
+
+/*
+ * Return the volatile_size to use in the CXL set paritition
+ * command, or ULLONG_MAX if unable to validate the partition
+ * request.
+ */
+static unsigned long long
+validate_partition(struct cxl_memdev *memdev, unsigned long long size)
+{
+	unsigned long long total_cap, volatile_only, persistent_only;
+	const char *devname = cxl_memdev_get_devname(memdev);
+	unsigned long long volatile_size = ULLONG_MAX;
+	unsigned long long partitionable, alignment;
+	struct cxl_cmd *cmd;
+	int rc;
+
+	cmd = cxl_cmd_new_identify(memdev);
+	if (!cmd)
+		return ULLONG_MAX;
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0)
+		goto out;
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0)
+		goto out;
+
+	alignment = cxl_cmd_identify_get_partition_align(cmd);
+	if (alignment == 0) {
+		log_err(&ml, "%s: no partitionable capacity\n", devname);
+		goto out;
+	}
+
+	/* Calculate the actual partitionable capacity */
+	total_cap = cxl_cmd_identify_get_total_size(cmd);
+	volatile_only = cxl_cmd_identify_get_volatile_only_size(cmd);
+	persistent_only = cxl_cmd_identify_get_persistent_only_size(cmd);
+	partitionable = total_cap - volatile_only - persistent_only;
+
+	/* Translate the users size request into an aligned volatile_size */
+	volatile_size = param_size_to_volatile_size(devname, size,
+				partitionable);
+	if (volatile_size == ULLONG_MAX)
+		goto out;
+
+	volatile_size = partition_align(devname, volatile_size, alignment,
+				partitionable);
+
+out:
+	cxl_cmd_unref(cmd);
+	return volatile_size;
+}
+
+static int action_setpartition(struct cxl_memdev *memdev,
+		struct action_context *actx)
+{
+	const char *devname = cxl_memdev_get_devname(memdev);
+	unsigned long long size = ULLONG_MAX;
+	struct json_object *jmemdev;
+	struct cxl_cmd *cmd;
+	int rc;
+
+	if (param.type) {
+		if (strcmp(param.type, "pmem") == 0)
+			/* pass */;
+		else if (strcmp(param.type, "volatile") == 0)
+			/* pass */;
+		else {
+			log_err(&ml, "invalid type '%s'\n", param.type);
+			return -EINVAL;
+		}
+	} else {
+		/* Default type is PMEM */
+		param.type = "pmem";
+	}
+
+	if (param.size) {
+		size = parse_size64(param.size);
+		if (size == ULLONG_MAX) {
+			log_err(&ml, "%s: failed to parse size option '%s'\n",
+			devname, param.size);
+			return -EINVAL;
+		}
+	}
+
+	size = validate_partition(memdev, size);
+	if (size == ULLONG_MAX)
+		return -EINVAL;
+
+	cmd = cxl_cmd_new_set_partition(memdev, size);
+	if (!cmd) {
+		rc = -ENXIO;
+		goto out_err;
+	}
+
+	rc = cxl_cmd_submit(cmd);
+	if (rc < 0) {
+		log_err(&ml, "cmd submission failed: %s\n", strerror(-rc));
+		goto out_cmd;
+	}
+
+	rc = cxl_cmd_get_mbox_status(cmd);
+	if (rc != 0) {
+		log_err(&ml, "%s: mbox status: %d\n", __func__, rc);
+		rc = -ENXIO;
+	}
+
+out_cmd:
+	cxl_cmd_unref(cmd);
+out_err:
+	if (rc)
+		log_err(&ml, "%s error: %s\n", devname, strerror(-rc));
+
+	jmemdev = util_cxl_memdev_to_json(memdev, UTIL_JSON_PARTITION);
+	if (jmemdev)
+		printf("%s\n", json_object_to_json_string_ext(jmemdev,
+		       JSON_C_TO_STRING_PRETTY));
+
+	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),
@@ -398,3 +583,14 @@ int cmd_enable_memdev(int argc, const char **argv, struct cxl_ctx *ctx)
 		 count > 1 ? "s" : "");
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_set_partition(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	int count = memdev_action(argc, argv, ctx, action_setpartition,
+			set_partition_options,
+			"cxl set-partition <mem0> [<mem1>..<memN>] [<options>]");
+	log_info(&ml, "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 v4 0/6] Add partitioning support for CXL memdevs

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Users may want to view and change partition layouts of CXL memory
> devices that support partitioning. Provide userspace access to
> the device partitioning capabilities as defined in the CXL 2.0
> specification.

This is minor feedback if these end up being re-spun, but "Users may
want..." is too passive for what this is, which is a critical building
block in the provisioning model for PMEM over CXL. So, consider
rewriting in active voice, and avoid underselling the importance of
this capability.

> The first 4 patches add accessors for all the information needed
> to formulate a new partition layout. This info is accessible via
> the libcxl API and a new option in the cxl list command:
>
>     "partition_info":{
>       "active_volatile_bytes":273535729664,
>       "active_persistent_bytes":0,
>       "next_volatile_bytes":268435456,
>       "next_persistent_bytes":273267294208,
>       "total_bytes":273535729664,
>       "volatile_only_bytes":0,
>       "persistent_only_bytes":0,
>       "partition_alignment_bytes":268435456
>     }

Is this stale? I.e. we discussed aligning the names to other
'size'-like values in 'ndctl list' and 'cxl list'.

>
> Patch 5 introduces the libcxl interfaces for the SET_PARTITION_INFO
> mailbox command and Patch 6 adds the new CXL command:
>
> Synopsis:
> cxl set-partition <mem0> [<mem1>..<memN>] [<options>]
>
> -t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
> -s, --size=<size>       size in bytes (Default: all partitionable capacity)

Spell-check does not like "partitionable"

s/partitionable/available/

> -a, --align             allow alignment correction

How about:

"Auto-align --size per device's requirement."

> -v, --verbose           turn on debug
>
> The CXL command does not offer the IMMEDIATE mode option defined

s/CXL/'cxl set-parition'/

This is a general problem caused by the tool 'cxl' being the same name
as the specification CXL. When it is ambiguous, go ahead and spell out
'cxl <command>'.

> in the CXL 2.0 spec because the CXL kernel driver does not support
> immediate mode yet. Since userspace could use the libcxl API to
> send a SET_PARTITION_INFO mailbox command with immediate mode
> selected, a kernel patch that rejects such requests is in review
> for the CXL driver.
>
> Changes in v4: (from Dan's review)
> - cxl set-partition command: add type (pmem | volatile),
>   add defaults for type and size, and add an align option.
> - Replace macros with return statements with functions.
> - Add cxl_set_partition_set_mode() to the libcxl API.
> - Add API documentation to Documentation/cxl/lib/libcxl.txt.
> - Use log_err/info mechanism.
> - Display memdev JSON info upon completion of set-partition command.
> - Remove unneeded casts when accessing command payloads.
> - Name changes - like drop _info suffix, use _size instead of _bytes.
>
> Changes in v3:
> - Back out the API name change to the partition align accessor.
>   The API was already released in v72.
> - Man page: collapse into one .txt file.
> - Man page: add to Documentation/meson.build
>
> Changes in v2:
> - Rebase onto https://github.com/pmem/ndctl.git djbw/meson.
> - Clarify that capacities are reported in bytes. (Ira)
>   This led to renaming accessors like *_capacity to  *_bytes and
>   a spattering of changes in the man pages and commit messages.
> - Add missing cxl_cmd_unref() when creating json objects. (Ira)
> - Change the cxl list option to: -I --partition (Dan)
> - Use OPT_STRING for the --volatile_size parameter (Dan, Vishal)
> - Drop extra _get_ in accessor names. (Vishal)
> - Add an accessor for the SET_PARTITION_INFO mbox cmd flag.
> - Display usage_with_options if size parameter is missing.
> - Drop the OPT64 define patch. Use OPT_STRING instead.
>
> Alison Schofield (6):
>   libcxl: add GET_PARTITION_INFO mailbox command and accessors
>   libcxl: add accessors for capacity fields of the IDENTIFY command
>   libcxl: return the partition alignment field in bytes
>   cxl: add memdev partition information to cxl-list
>   libcxl: add interfaces for SET_PARTITION_INFO mailbox command
>   cxl: add command set-partition
>
>  Documentation/cxl/cxl-list.txt          |  23 +++
>  Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
>  Documentation/cxl/lib/libcxl.txt        |   5 +
>  Documentation/cxl/meson.build           |   1 +
>  cxl/builtin.h                           |   1 +
>  cxl/cxl.c                               |   1 +
>  cxl/filter.c                            |   2 +
>  cxl/filter.h                            |   1 +
>  cxl/json.c                              | 113 ++++++++++++++
>  cxl/lib/libcxl.c                        | 123 ++++++++++++++-
>  cxl/lib/libcxl.sym                      |  10 ++
>  cxl/lib/private.h                       |  18 +++
>  cxl/libcxl.h                            |  18 +++
>  cxl/list.c                              |   2 +
>  cxl/memdev.c                            | 196 ++++++++++++++++++++++++
>  util/json.h                             |   1 +
>  util/size.h                             |   1 +
>  17 files changed, 575 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/cxl-set-partition.txt
>
> --
> 2.31.1
>

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

[Alison Schofield]

On Tue, Feb 08, 2022 at 09:23:54AM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users may want to view and change partition layouts of CXL memory
> > devices that support partitioning. Provide userspace access to
> > the device partitioning capabilities as defined in the CXL 2.0
> > specification.
> 
> This is minor feedback if these end up being re-spun, but "Users may
> want..." is too passive for what this is, which is a critical building
> block in the provisioning model for PMEM over CXL. So, consider
> rewriting in active voice, and avoid underselling the importance of
> this capability.

Yes! I have some words you gave me in another commit I will draw upon.

> 
> > The first 4 patches add accessors for all the information needed
> > to formulate a new partition layout. This info is accessible via
> > the libcxl API and a new option in the cxl list command:
> >
> >     "partition_info":{
> >       "active_volatile_bytes":273535729664,
> >       "active_persistent_bytes":0,
> >       "next_volatile_bytes":268435456,
> >       "next_persistent_bytes":273267294208,
> >       "total_bytes":273535729664,
> >       "volatile_only_bytes":0,
> >       "persistent_only_bytes":0,
> >       "partition_alignment_bytes":268435456
> >     }
> 
> Is this stale? I.e. we discussed aligning the names to other
> 'size'-like values in 'ndctl list' and 'cxl list'.
> 

Yes - that is STALE. The cxl-list patch commit msg has it right.
Will fix here.

 "partition_info":{
      "active_volatile_size":273535729664,
      "active_persistent_size":0,
      "next_volatile_size":0,
      "next_persistent_size":0,
      "total_size":273535729664,
      "volatile_only_size":0,
      "persistent_only_size":0,
      "partition_alignment_size":268435456
    }



> >
> > Patch 5 introduces the libcxl interfaces for the SET_PARTITION_INFO
> > mailbox command and Patch 6 adds the new CXL command:
> >
> > Synopsis:
> > cxl set-partition <mem0> [<mem1>..<memN>] [<options>]
> >
> > -t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
> > -s, --size=<size>       size in bytes (Default: all partitionable capacity)
> 
> Spell-check does not like "partitionable"
> 
> s/partitionable/available/

hmm... passes my spell check, but alas, it is overuse of the root word.
I like available. Will change.

> 
> > -a, --align             allow alignment correction
> 
> How about:
> 
> "Auto-align --size per device's requirement."
> 

So much better. Thanks.

> > -v, --verbose           turn on debug
> >
> > The CXL command does not offer the IMMEDIATE mode option defined
> 
> s/CXL/'cxl set-parition'/
> 
> This is a general problem caused by the tool 'cxl' being the same name
> as the specification CXL. When it is ambiguous, go ahead and spell out
> 'cxl <command>'.
> 

Got it.

snip...

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

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Users need access to the CXL GET_PARTITION_INFO mailbox command
> to inspect and confirm changes to the partition layout of a memory
> device.
>
> 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.
>
> Per the CXL 2.0 specification, devices report partition capacities
> as multiples of 256MB. Define and use a capacity multiplier to
> convert the raw data into bytes for user consumption. Use byte
> format as the norm for all capacity values produced or consumed
> using CXL Mailbox commands.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/lib/libcxl.txt |  1 +
>  cxl/lib/libcxl.c                 | 57 ++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym               |  5 +++
>  cxl/lib/private.h                | 10 ++++++
>  cxl/libcxl.h                     |  5 +++
>  util/size.h                      |  1 +
>  6 files changed, 79 insertions(+)
>
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index 4392b47..a6986ab 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -131,6 +131,7 @@ 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);
> +struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
>
>  ----
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index e0b443f..33cf06b 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1985,6 +1985,12 @@ static int cxl_cmd_validate_status(struct cxl_cmd *cmd, u32 id)
>         return 0;
>  }
>
> +static unsigned long long
> +capacity_to_bytes(unsigned long long size)

If this helper converts an encoded le64 to bytes then the function
signature should reflect that:

static uint64_t cxl_capacity_to_bytes(leint64_t size)

> +{
> +       return le64_to_cpu(size) * CXL_CAPACITY_MULTIPLIER;
> +}
> +
>  /* Helpers for health_info fields (no endian conversion) */
>  #define cmd_get_field_u8(cmd, n, N, field)                             \
>  do {                                                                   \
> @@ -2371,6 +2377,57 @@ 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(struct cxl_memdev *memdev)
> +{
> +       return cxl_cmd_new_generic(memdev,
> +                                  CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
> +}
> +
> +static struct cxl_cmd_get_partition *
> +cmd_to_get_partition(struct cxl_cmd *cmd)
> +{

This could also check for cmd == NULL just to be complete.

> +       if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_GET_PARTITION_INFO))
> +               return NULL;
> +
> +       return cmd->output_payload;
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cmd)
> +{
> +       struct cxl_cmd_get_partition *c;
> +
> +       c = cmd_to_get_partition(cmd);
> +       return c ? capacity_to_bytes(c->active_volatile) : ULLONG_MAX;

I'd prefer kernel coding style which wants:

if (!c)
    return ULLONG_MAX;
return cxl_capacity_to_bytes(c->active_volatile);

Otherwise, looks good to me:

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

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

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Users need access to a few additional fields reported by the IDENTIFY

Ah, I see the "Users need" pattern... To me, the "Users need"
statement is a step removed / secondary from the real driving
motivation which is the "CXL PMEM provisioning model specifies /
mandates".

It feels like a watered down abstraction to me.

> mailbox command: total, volatile_only, and persistent_only capacities.
> These values are useful when defining partition layouts.
>
> Add accessors to the libcxl API to retrieve these values from the
> IDENTIFY command.
>
> The fields are specified in multiples of 256MB per the CXL 2.0 spec.
> Use the capacity multiplier to convert the raw data into bytes for user
> consumption.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/libcxl.c   | 36 ++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  3 +++
>  cxl/libcxl.h       |  3 +++
>  3 files changed, 42 insertions(+)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 33cf06b..e9d7762 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -2322,6 +2322,42 @@ CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
>         return le32_to_cpu(id->lsa_size);
>  }
>
> +static struct cxl_cmd_identify *
> +cmd_to_identify(struct cxl_cmd *cmd)
> +{
> +       if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_IDENTIFY))
> +               return NULL;
> +
> +       return cmd->output_payload;
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_total_size(struct cxl_cmd *cmd)
> +{
> +       struct cxl_cmd_identify *c;
> +
> +       c = cmd_to_identify(cmd);
> +       return c ? capacity_to_bytes(c->total_capacity) : ULLONG_MAX;
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_volatile_only_size(struct cxl_cmd *cmd)
> +{
> +       struct cxl_cmd_identify *c;
> +
> +       c = cmd_to_identify(cmd);
> +       return c ? capacity_to_bytes(c->volatile_capacity) : ULLONG_MAX;
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd)
> +{
> +       struct cxl_cmd_identify *c;
> +
> +       c = cmd_to_identify(cmd);
> +       return c ? capacity_to_bytes(c->persistent_capacity) : ULLONG_MAX;

Same style comments as last patch, but otherwise:

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

Re: [ndctl PATCH v4 3/6] libcxl: return the partition alignment field in bytes

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Per the CXL specification, the partition alignment field reports
> the alignment value in multiples of 256MB. In the libcxl API, values
> for all capacity fields are defined to return bytes.
>
> Update the partition alignment accessor to return bytes so that it
> is in sync with other capacity related fields.

Perhaps a note that the expectation is that this early in the
development cycle the expectation is that no third party consumers of
this library have come to depend on the encoded capacity field.


>
> 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 e9d7762..307e5c4 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -2306,7 +2306,7 @@ CXL_EXPORT unsigned long long cxl_cmd_identify_get_partition_align(
>         if (cmd->status < 0)
>                 return cmd->status;

Hmm, I like that this function does "if ()" instead of "? :", however,
it seems it also needs to be fixed to return ULLONG_MAX in the error
case. The status will otherwise be misinterpreted as the alignment.

>
> -       return le64_to_cpu(id->partition_align);
> +       return capacity_to_bytes(id->partition_align);
>  }
>
>  CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
> --
> 2.31.1
>

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

[Alison Schofield]

Thanks Dan! Got it all.
<eom>

On Tue, Feb 08, 2022 at 12:20:56PM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users need access to the CXL GET_PARTITION_INFO mailbox command
> > to inspect and confirm changes to the partition layout of a memory
> > device.
> >
> > 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.
> >
> > Per the CXL 2.0 specification, devices report partition capacities
> > as multiples of 256MB. Define and use a capacity multiplier to
> > convert the raw data into bytes for user consumption. Use byte
> > format as the norm for all capacity values produced or consumed
> > using CXL Mailbox commands.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  Documentation/cxl/lib/libcxl.txt |  1 +
> >  cxl/lib/libcxl.c                 | 57 ++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym               |  5 +++
> >  cxl/lib/private.h                | 10 ++++++
> >  cxl/libcxl.h                     |  5 +++
> >  util/size.h                      |  1 +
> >  6 files changed, 79 insertions(+)
> >
> > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > index 4392b47..a6986ab 100644
> > --- a/Documentation/cxl/lib/libcxl.txt
> > +++ b/Documentation/cxl/lib/libcxl.txt
> > @@ -131,6 +131,7 @@ 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);
> > +struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
> >
> >  ----
> >
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index e0b443f..33cf06b 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1985,6 +1985,12 @@ static int cxl_cmd_validate_status(struct cxl_cmd *cmd, u32 id)
> >         return 0;
> >  }
> >
> > +static unsigned long long
> > +capacity_to_bytes(unsigned long long size)
> 
> If this helper converts an encoded le64 to bytes then the function
> signature should reflect that:
> 
> static uint64_t cxl_capacity_to_bytes(leint64_t size)
> 
> > +{
> > +       return le64_to_cpu(size) * CXL_CAPACITY_MULTIPLIER;
> > +}
> > +
> >  /* Helpers for health_info fields (no endian conversion) */
> >  #define cmd_get_field_u8(cmd, n, N, field)                             \
> >  do {                                                                   \
> > @@ -2371,6 +2377,57 @@ 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(struct cxl_memdev *memdev)
> > +{
> > +       return cxl_cmd_new_generic(memdev,
> > +                                  CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
> > +}
> > +
> > +static struct cxl_cmd_get_partition *
> > +cmd_to_get_partition(struct cxl_cmd *cmd)
> > +{
> 
> This could also check for cmd == NULL just to be complete.
> 
> > +       if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_GET_PARTITION_INFO))
> > +               return NULL;
> > +
> > +       return cmd->output_payload;
> > +}
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_partition_get_active_volatile_size(struct cxl_cmd *cmd)
> > +{
> > +       struct cxl_cmd_get_partition *c;
> > +
> > +       c = cmd_to_get_partition(cmd);
> > +       return c ? capacity_to_bytes(c->active_volatile) : ULLONG_MAX;
> 
> I'd prefer kernel coding style which wants:
> 
> if (!c)
>     return ULLONG_MAX;
> return cxl_capacity_to_bytes(c->active_volatile);
> 
> Otherwise, looks good to me:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

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

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Users may want the ability to change the partition layout of a CXL
> memory device.
>
> Add interfaces to libcxl to allocate and send a SET_PARTITION_INFO
> mailbox command as defined in the CXL 2.0 specification.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/lib/libcxl.txt |  4 ++++
>  cxl/lib/libcxl.c                 | 28 ++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym               |  2 ++
>  cxl/lib/private.h                |  8 ++++++++
>  cxl/libcxl.h                     | 10 ++++++++++
>  5 files changed, 52 insertions(+)
>
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index a6986ab..301b4d7 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -132,6 +132,8 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
>  int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
>                            size_t offset);
>  struct cxl_cmd *cxl_cmd_new_get_partition(struct cxl_memdev *memdev);
> +struct cxl_cmd *cxl_cmd_new_set_partition(struct cxl_memdev *memdev,
> +                                         unsigned long long volatile_size);

Perhaps a note here about the role of cxl_cmd_partition_set_mode(),
that the default sets the partition for NEXTBOOT, and that IMMEDIATE
support depends on the device being inactive in all regions before the
kernel will allow the change?

Otherwise, looks good.

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

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

[Alison Schofield]

Got it, thanks!
<eom>

On Tue, Feb 08, 2022 at 12:34:23PM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users need access to a few additional fields reported by the IDENTIFY
> 
> Ah, I see the "Users need" pattern... To me, the "Users need"
> statement is a step removed / secondary from the real driving
> motivation which is the "CXL PMEM provisioning model specifies /
> mandates".
> 
> It feels like a watered down abstraction to me.
> 
> > mailbox command: total, volatile_only, and persistent_only capacities.
> > These values are useful when defining partition layouts.
> >
> > Add accessors to the libcxl API to retrieve these values from the
> > IDENTIFY command.
> >
> > The fields are specified in multiples of 256MB per the CXL 2.0 spec.
> > Use the capacity multiplier to convert the raw data into bytes for user
> > consumption.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/libcxl.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym |  3 +++
> >  cxl/libcxl.h       |  3 +++
> >  3 files changed, 42 insertions(+)
> >
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 33cf06b..e9d7762 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -2322,6 +2322,42 @@ CXL_EXPORT unsigned int cxl_cmd_identify_get_label_size(struct cxl_cmd *cmd)
> >         return le32_to_cpu(id->lsa_size);
> >  }
> >
> > +static struct cxl_cmd_identify *
> > +cmd_to_identify(struct cxl_cmd *cmd)
> > +{
> > +       if (cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_IDENTIFY))
> > +               return NULL;
> > +
> > +       return cmd->output_payload;
> > +}
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_identify_get_total_size(struct cxl_cmd *cmd)
> > +{
> > +       struct cxl_cmd_identify *c;
> > +
> > +       c = cmd_to_identify(cmd);
> > +       return c ? capacity_to_bytes(c->total_capacity) : ULLONG_MAX;
> > +}
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_identify_get_volatile_only_size(struct cxl_cmd *cmd)
> > +{
> > +       struct cxl_cmd_identify *c;
> > +
> > +       c = cmd_to_identify(cmd);
> > +       return c ? capacity_to_bytes(c->volatile_capacity) : ULLONG_MAX;
> > +}
> > +
> > +CXL_EXPORT unsigned long long
> > +cxl_cmd_identify_get_persistent_only_size(struct cxl_cmd *cmd)
> > +{
> > +       struct cxl_cmd_identify *c;
> > +
> > +       c = cmd_to_identify(cmd);
> > +       return c ? capacity_to_bytes(c->persistent_capacity) : ULLONG_MAX;
> 
> Same style comments as last patch, but otherwise:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [ndctl PATCH v4 6/6] cxl: add command set-partition

[Dan Williams]

On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> CXL devices may support both volatile and persistent memory capacity.
> The amount of device capacity set aside for each type is typically
> established at the factory, but some devices also allow for dynamic
> re-partitioning, add a command for this purpose.
>
> Synopsis:
>
> cxl set-partition <mem0> [<mem1>..<memN>] [<options>]
>
> -t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
> -s, --size=<size>       size in bytes (Default: all partitionable capacity)
> -a, --align             allow alignment correction
> -v, --verbose           turn on debug
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
>  Documentation/cxl/meson.build           |   1 +
>  cxl/builtin.h                           |   1 +
>  cxl/cxl.c                               |   1 +
>  cxl/memdev.c                            | 196 ++++++++++++++++++++++++
>  5 files changed, 259 insertions(+)
>  create mode 100644 Documentation/cxl/cxl-set-partition.txt
>
> diff --git a/Documentation/cxl/cxl-set-partition.txt b/Documentation/cxl/cxl-set-partition.txt
> new file mode 100644
> index 0000000..e20afba
> --- /dev/null
> +++ b/Documentation/cxl/cxl-set-partition.txt
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-set-partition(1)
> +====================
> +
> +NAME
> +----
> +cxl-set-partition - set the partitioning between volatile and persistent capacity on a CXL memdev
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl set-partition <mem> [ [<mem1>..<memN>] [<options>]'
> +
> +DESCRIPTION
> +-----------
> +Partition the device into volatile and persistent capacity.

I wonder if this should briefly describe the theory of operation of
partitioning of CXL devices. I.e. that this command is only relevant
for devices that support dual capacity (volatile + pmem) and only dual
capacity devices that have partitionable as opposed to static capacity
assignments. Maybe a reference to how to use 'cxl list' to determine
if 'cxl set-partition' is even relevant for the given memdev?


 The change
> +in partitioning will become the “next” configuration, to become active
> +on the next device reset.
> +
> +Use "cxl list -m <memdev> -I" to examine the partitioning capabilities
> +of a device. A partition_alignment_bytes value of zero means there are
> +no partitionable bytes available and therefore the partitions cannot be
> +changed.
> +
> +Using this command to change the size of the persistent capacity shall
> +result in the loss of data stored.
> +
> +OPTIONS
> +-------
> +<memory device(s)>::
> +include::memdev-option.txt[]
> +
> +-t::
> +--type=::
> +       Type of partition, 'pmem' or 'volatile', to create.

s/create/modify/

...since the partition is statically present, just variably sized.

> +       Default: 'pmem'
> +
> +-s::
> +--size=::
> +       Size of the <type> partition in bytes. Size must align to the
> +       devices alignment size. Use 'cxl list -m <memdev> -I' to find
> +       the 'partition_alignment_size', or, use the 'align' option.
> +       Default: All partitionable capacity is assigned to <type>.
> +
> +-a::
> +--align::
> +       This option allows the size of the partition to be increased to
> +       meet device alignment requirements.

Perhaps reword this to say it auto-aligns so that the user can specify
the minimum size of the partition?

> +
> +-v::
> +        Turn on verbose debug messages in the library (if libcxl was built with
> +        logging and debug enabled).
> +
> +include::../copyright.txt[]
> +
> +SEE ALSO
> +--------
> +linkcxl:cxl-list[1],
> +CXL-2.0 8.2.9.5.2
> diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
> index 96f4666..e927644 100644
> --- a/Documentation/cxl/meson.build
> +++ b/Documentation/cxl/meson.build
> @@ -34,6 +34,7 @@ cxl_manpages = [
>    'cxl-disable-memdev.txt',
>    'cxl-enable-port.txt',
>    'cxl-disable-port.txt',
> +  'cxl-set-partition.txt',
>  ]
>
>  foreach man : cxl_manpages
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index 3123d5e..7bbad98 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -14,4 +14,5 @@ int cmd_disable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_enable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_disable_port(int argc, const char **argv, struct cxl_ctx *ctx);
>  int cmd_enable_port(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_set_partition(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index c20c569..ab4bbec 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -68,6 +68,7 @@ static struct cmd_struct commands[] = {
>         { "enable-memdev", .c_fn = cmd_enable_memdev },
>         { "disable-port", .c_fn = cmd_disable_port },
>         { "enable-port", .c_fn = cmd_enable_port },
> +       { "set-partition", .c_fn = cmd_set_partition },
>  };
>
>  int main(int argc, const char **argv)
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 90b33e1..5d97610 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -6,11 +6,14 @@
>  #include <unistd.h>
>  #include <limits.h>
>  #include <util/log.h>
> +#include <util/json.h>
> +#include <util/size.h>
>  #include <cxl/libcxl.h>
>  #include <util/parse-options.h>
>  #include <ccan/minmax/minmax.h>
>  #include <ccan/array_size/array_size.h>
>
> +#include "json.h"
>  #include "filter.h"
>
>  struct action_context {
> @@ -26,6 +29,9 @@ static struct parameters {
>         bool verbose;
>         bool serial;
>         bool force;
> +       bool align;
> +       const char *type;
> +       const char *size;
>  } param;
>
>  static struct log_ctx ml;
> @@ -51,6 +57,14 @@ OPT_UINTEGER('O', "offset", &param.offset, \
>  OPT_BOOLEAN('f', "force", &param.force,                                \
>             "DANGEROUS: override active memdev safety checks")
>
> +#define SET_PARTITION_OPTIONS() \
> +OPT_STRING('t', "type",  &param.type, "type",                  \
> +       "'pmem' or 'volatile' (Default: 'pmem')"),              \
> +OPT_STRING('s', "size",  &param.size, "size",                  \
> +       "size in bytes (Default: all partitionable capacity)"), \
> +OPT_BOOLEAN('a', "align",  &param.align,                       \
> +       "allow alignment correction")
> +
>  static const struct option read_options[] = {
>         BASE_OPTIONS(),
>         LABEL_OPTIONS(),
> @@ -82,6 +96,12 @@ static const struct option enable_options[] = {
>         OPT_END(),
>  };
>
> +static const struct option set_partition_options[] = {
> +       BASE_OPTIONS(),
> +       SET_PARTITION_OPTIONS(),
> +       OPT_END(),
> +};
> +
>  static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
>  {
>         if (!cxl_memdev_is_enabled(memdev))
> @@ -209,6 +229,171 @@ out:
>         return rc;
>  }
>
> +static unsigned long long
> +partition_align(const char *devname, unsigned long long volatile_size,
> +               unsigned long long alignment, unsigned long long partitionable)
> +{
> +       if (IS_ALIGNED(volatile_size, alignment))
> +               return volatile_size;
> +
> +       if (!param.align) {
> +               log_err(&ml, "%s: size %lld is not partition aligned %lld\n",
> +                       devname, volatile_size, alignment);
> +               return ULLONG_MAX;
> +       }
> +
> +       /* Align based on partition type to fulfill users size request */
> +       if (strcmp(param.type, "pmem") == 0)
> +               volatile_size = ALIGN_DOWN(volatile_size, alignment);
> +       else
> +               volatile_size = ALIGN(volatile_size, alignment);
> +
> +       /* Fail if the align pushes size over the partitionable limit. */
> +       if (volatile_size > partitionable) {
> +               log_err(&ml, "%s: aligned partition size %lld exceeds partitionable size %lld\n",
> +                       devname, volatile_size, partitionable);
> +               volatile_size = ULLONG_MAX;
> +       }
> +
> +       return volatile_size;
> +}
> +
> +static unsigned long long
> +param_size_to_volatile_size(const char *devname, unsigned long long size,
> +               unsigned long long partitionable)
> +{
> +       /* User omits size option. Apply all partitionable capacity to type. */
> +       if (size == ULLONG_MAX)
> +               return (strcmp(param.type, "pmem") == 0) ? 0 : partitionable;

Somewhat inefficient to keep needing to parse the string parameter
buffer. How about plumb an 'enum cxl_setpart_type type' argument to
all the functions that are parsing param.type?

Where @type is:

enum cxl_setpart_type {
    CXL_SETPART_PMEM,
    CXL_SETPART_VOLATILE,
};

Other than the above,

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

Re: [ndctl PATCH v4 6/6] cxl: add command set-partition

[Alison Schofield]

Thanks Dan. Understand all comments & will do.

On Tue, Feb 08, 2022 at 01:08:39PM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > CXL devices may support both volatile and persistent memory capacity.
> > The amount of device capacity set aside for each type is typically
> > established at the factory, but some devices also allow for dynamic
> > re-partitioning, add a command for this purpose.
> >
> > Synopsis:
> >
> > cxl set-partition <mem0> [<mem1>..<memN>] [<options>]
> >
> > -t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
> > -s, --size=<size>       size in bytes (Default: all partitionable capacity)
> > -a, --align             allow alignment correction
> > -v, --verbose           turn on debug
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
> >  Documentation/cxl/meson.build           |   1 +
> >  cxl/builtin.h                           |   1 +
> >  cxl/cxl.c                               |   1 +
> >  cxl/memdev.c                            | 196 ++++++++++++++++++++++++
> >  5 files changed, 259 insertions(+)
> >  create mode 100644 Documentation/cxl/cxl-set-partition.txt
> >
> > diff --git a/Documentation/cxl/cxl-set-partition.txt b/Documentation/cxl/cxl-set-partition.txt
> > new file mode 100644
> > index 0000000..e20afba
> > --- /dev/null
> > +++ b/Documentation/cxl/cxl-set-partition.txt
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +cxl-set-partition(1)
> > +====================
> > +
> > +NAME
> > +----
> > +cxl-set-partition - set the partitioning between volatile and persistent capacity on a CXL memdev
> > +
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'cxl set-partition <mem> [ [<mem1>..<memN>] [<options>]'
> > +
> > +DESCRIPTION
> > +-----------
> > +Partition the device into volatile and persistent capacity.
> 
> I wonder if this should briefly describe the theory of operation of
> partitioning of CXL devices. I.e. that this command is only relevant
> for devices that support dual capacity (volatile + pmem) and only dual
> capacity devices that have partitionable as opposed to static capacity
> assignments. Maybe a reference to how to use 'cxl list' to determine
> if 'cxl set-partition' is even relevant for the given memdev?
> 
> 
>  The change
> > +in partitioning will become the “next” configuration, to become active
> > +on the next device reset.
> > +
> > +Use "cxl list -m <memdev> -I" to examine the partitioning capabilities
> > +of a device. A partition_alignment_bytes value of zero means there are
> > +no partitionable bytes available and therefore the partitions cannot be
> > +changed.
> > +
> > +Using this command to change the size of the persistent capacity shall
> > +result in the loss of data stored.
> > +
> > +OPTIONS
> > +-------
> > +<memory device(s)>::
> > +include::memdev-option.txt[]
> > +
> > +-t::
> > +--type=::
> > +       Type of partition, 'pmem' or 'volatile', to create.
> 
> s/create/modify/
> 
> ...since the partition is statically present, just variably sized.
> 
> > +       Default: 'pmem'
> > +
> > +-s::
> > +--size=::
> > +       Size of the <type> partition in bytes. Size must align to the
> > +       devices alignment size. Use 'cxl list -m <memdev> -I' to find
> > +       the 'partition_alignment_size', or, use the 'align' option.
> > +       Default: All partitionable capacity is assigned to <type>.
> > +
> > +-a::
> > +--align::
> > +       This option allows the size of the partition to be increased to
> > +       meet device alignment requirements.
> 
> Perhaps reword this to say it auto-aligns so that the user can specify
> the minimum size of the partition?
> 
> > +
> > +-v::
> > +        Turn on verbose debug messages in the library (if libcxl was built with
> > +        logging and debug enabled).
> > +
> > +include::../copyright.txt[]
> > +
> > +SEE ALSO
> > +--------
> > +linkcxl:cxl-list[1],
> > +CXL-2.0 8.2.9.5.2
> > diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
> > index 96f4666..e927644 100644
> > --- a/Documentation/cxl/meson.build
> > +++ b/Documentation/cxl/meson.build
> > @@ -34,6 +34,7 @@ cxl_manpages = [
> >    'cxl-disable-memdev.txt',
> >    'cxl-enable-port.txt',
> >    'cxl-disable-port.txt',
> > +  'cxl-set-partition.txt',
> >  ]
> >
> >  foreach man : cxl_manpages
> > diff --git a/cxl/builtin.h b/cxl/builtin.h
> > index 3123d5e..7bbad98 100644
> > --- a/cxl/builtin.h
> > +++ b/cxl/builtin.h
> > @@ -14,4 +14,5 @@ int cmd_disable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
> >  int cmd_enable_memdev(int argc, const char **argv, struct cxl_ctx *ctx);
> >  int cmd_disable_port(int argc, const char **argv, struct cxl_ctx *ctx);
> >  int cmd_enable_port(int argc, const char **argv, struct cxl_ctx *ctx);
> > +int cmd_set_partition(int argc, const char **argv, struct cxl_ctx *ctx);
> >  #endif /* _CXL_BUILTIN_H_ */
> > diff --git a/cxl/cxl.c b/cxl/cxl.c
> > index c20c569..ab4bbec 100644
> > --- a/cxl/cxl.c
> > +++ b/cxl/cxl.c
> > @@ -68,6 +68,7 @@ static struct cmd_struct commands[] = {
> >         { "enable-memdev", .c_fn = cmd_enable_memdev },
> >         { "disable-port", .c_fn = cmd_disable_port },
> >         { "enable-port", .c_fn = cmd_enable_port },
> > +       { "set-partition", .c_fn = cmd_set_partition },
> >  };
> >
> >  int main(int argc, const char **argv)
> > diff --git a/cxl/memdev.c b/cxl/memdev.c
> > index 90b33e1..5d97610 100644
> > --- a/cxl/memdev.c
> > +++ b/cxl/memdev.c
> > @@ -6,11 +6,14 @@
> >  #include <unistd.h>
> >  #include <limits.h>
> >  #include <util/log.h>
> > +#include <util/json.h>
> > +#include <util/size.h>
> >  #include <cxl/libcxl.h>
> >  #include <util/parse-options.h>
> >  #include <ccan/minmax/minmax.h>
> >  #include <ccan/array_size/array_size.h>
> >
> > +#include "json.h"
> >  #include "filter.h"
> >
> >  struct action_context {
> > @@ -26,6 +29,9 @@ static struct parameters {
> >         bool verbose;
> >         bool serial;
> >         bool force;
> > +       bool align;
> > +       const char *type;
> > +       const char *size;
> >  } param;
> >
> >  static struct log_ctx ml;
> > @@ -51,6 +57,14 @@ OPT_UINTEGER('O', "offset", &param.offset, \
> >  OPT_BOOLEAN('f', "force", &param.force,                                \
> >             "DANGEROUS: override active memdev safety checks")
> >
> > +#define SET_PARTITION_OPTIONS() \
> > +OPT_STRING('t', "type",  &param.type, "type",                  \
> > +       "'pmem' or 'volatile' (Default: 'pmem')"),              \
> > +OPT_STRING('s', "size",  &param.size, "size",                  \
> > +       "size in bytes (Default: all partitionable capacity)"), \
> > +OPT_BOOLEAN('a', "align",  &param.align,                       \
> > +       "allow alignment correction")
> > +
> >  static const struct option read_options[] = {
> >         BASE_OPTIONS(),
> >         LABEL_OPTIONS(),
> > @@ -82,6 +96,12 @@ static const struct option enable_options[] = {
> >         OPT_END(),
> >  };
> >
> > +static const struct option set_partition_options[] = {
> > +       BASE_OPTIONS(),
> > +       SET_PARTITION_OPTIONS(),
> > +       OPT_END(),
> > +};
> > +
> >  static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
> >  {
> >         if (!cxl_memdev_is_enabled(memdev))
> > @@ -209,6 +229,171 @@ out:
> >         return rc;
> >  }
> >
> > +static unsigned long long
> > +partition_align(const char *devname, unsigned long long volatile_size,
> > +               unsigned long long alignment, unsigned long long partitionable)
> > +{
> > +       if (IS_ALIGNED(volatile_size, alignment))
> > +               return volatile_size;
> > +
> > +       if (!param.align) {
> > +               log_err(&ml, "%s: size %lld is not partition aligned %lld\n",
> > +                       devname, volatile_size, alignment);
> > +               return ULLONG_MAX;
> > +       }
> > +
> > +       /* Align based on partition type to fulfill users size request */
> > +       if (strcmp(param.type, "pmem") == 0)
> > +               volatile_size = ALIGN_DOWN(volatile_size, alignment);
> > +       else
> > +               volatile_size = ALIGN(volatile_size, alignment);
> > +
> > +       /* Fail if the align pushes size over the partitionable limit. */
> > +       if (volatile_size > partitionable) {
> > +               log_err(&ml, "%s: aligned partition size %lld exceeds partitionable size %lld\n",
> > +                       devname, volatile_size, partitionable);
> > +               volatile_size = ULLONG_MAX;
> > +       }
> > +
> > +       return volatile_size;
> > +}
> > +
> > +static unsigned long long
> > +param_size_to_volatile_size(const char *devname, unsigned long long size,
> > +               unsigned long long partitionable)
> > +{
> > +       /* User omits size option. Apply all partitionable capacity to type. */
> > +       if (size == ULLONG_MAX)
> > +               return (strcmp(param.type, "pmem") == 0) ? 0 : partitionable;
> 
> Somewhat inefficient to keep needing to parse the string parameter
> buffer. How about plumb an 'enum cxl_setpart_type type' argument to
> all the functions that are parsing param.type?
> 
> Where @type is:
> 
> enum cxl_setpart_type {
>     CXL_SETPART_PMEM,
>     CXL_SETPART_VOLATILE,
> };
>
> Other than the above,
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>