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

[ndctl PATCH v3 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: 

# 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

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.

Tested in QEMU and Simics environment. No cxl_test yet.

Repo: https://github.com/AlisonSchofield/ndctl/tree/as/partitions

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-info

 Documentation/cxl/cxl-list.txt               |  23 ++++
 Documentation/cxl/cxl-set-partition-info.txt |  53 ++++++++
 Documentation/cxl/meson.build                |   1 +
 cxl/builtin.h                                |   1 +
 cxl/cxl.c                                    |   1 +
 cxl/json.c                                   | 114 +++++++++++++++++
 cxl/lib/libcxl.c                             | 122 ++++++++++++++++++-
 cxl/lib/libcxl.sym                           |  13 ++
 cxl/lib/private.h                            |  18 +++
 cxl/libcxl.h                                 |  13 ++
 cxl/list.c                                   |   5 +
 cxl/memdev.c                                 | 101 +++++++++++++++
 util/json.h                                  |   1 +
 util/size.h                                  |   1 +
 14 files changed, 466 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/cxl-set-partition-info.txt


base-commit: 25062cf34c70012f5d42ce1fef7e2dc129807c10
prerequisite-patch-id: d34c91059d77651eaf1e71adb6135b397f64c056
prerequisite-patch-id: 72a64823dc720b5c7b65fe949d75e9f6fbf79035
prerequisite-patch-id: f9fd1d50d96896f27ce1d6df59d1aa8ea06795ab
prerequisite-patch-id: 0e904349e14e74754a22bd72ce97add030d80e90
prerequisite-patch-id: 331899e7b565c625ef0647f71057850a919535d8
prerequisite-patch-id: 0ddeb2219011fe7b272cbbf59bc046bf26ee1bc7
prerequisite-patch-id: fe1023948a0202cca8d641e694de425512512f92
prerequisite-patch-id: 48b0b8feddcc98201f3c75b15bb1ed13d30e1269
prerequisite-patch-id: 5a9be92f860c0bf9144772b550d62e0c0a463e7c
prerequisite-patch-id: 113e1fa147e96ba5d4127246cc3dcae895d33e7c
prerequisite-patch-id: 03ad3eb99d14b51a0d0a48dfb3f6f3b2f84bac24
prerequisite-patch-id: 6c9b29768efc178c37d62c53501a98bc7baa9934
prerequisite-patch-id: d4086981a857f5f2d3720f33e1c823f906b3211f
prerequisite-patch-id: 3ad6ea54cc6430977c8ae4a0833e22499f1ccd96
prerequisite-patch-id: d4ff0ee2dc988440e0cad26d43e1a740fb89fbfd
prerequisite-patch-id: e33606c2a4ab9ac74034cb3bde8947317274d8b3
prerequisite-patch-id: e7b57274d44fd9cdf1b64ca802b33afe255cef44
-- 
2.31.1

[ndctl PATCH v3 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>
---
 cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 cxl/lib/private.h  | 10 ++++++++++
 cxl/libcxl.h       |  5 +++++
 util/size.h        |  1 +
 5 files changed, 62 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 3390eb9..58181c0 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1160,6 +1160,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_partition_info_get_active_volatile_bytes(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_info_get_active_persistent_bytes(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_persistent_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_info_get_next_volatile_bytes(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, next_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_partition_info_get_next_persistent_bytes(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..e019c3c 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_partition_info_get_active_volatile_bytes;
+	cxl_cmd_partition_info_get_active_persistent_bytes;
+	cxl_cmd_partition_info_get_next_volatile_bytes;
+	cxl_cmd_partition_info_get_next_persistent_bytes;
 local:
         *;
 };
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..08fd840 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_partition_info_get_active_volatile_bytes(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_info_get_active_persistent_bytes(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_info_get_next_volatile_bytes(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_partition_info_get_next_persistent_bytes(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 v3 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   | 29 +++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  3 +++
 cxl/libcxl.h       |  3 +++
 3 files changed, 35 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 58181c0..1fd584a 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1105,6 +1105,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_bytes(struct cxl_cmd *cmd)
+{
+	cmd_identify_get_capacity_field(cmd, total_capacity);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_volatile_only_bytes(struct cxl_cmd *cmd)
+{
+	cmd_identify_get_capacity_field(cmd, volatile_capacity);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_identify_get_persistent_only_bytes(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 e019c3c..b7e969f 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_bytes;
+	cxl_cmd_identify_get_volatile_only_bytes;
+	cxl_cmd_identify_get_persistent_only_bytes;
 	cxl_cmd_identify_get_partition_align;
 	cxl_cmd_identify_get_label_size;
 	cxl_cmd_new_get_health_info;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 08fd840..46f99fb 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_bytes(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_volatile_only_bytes(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_identify_get_persistent_only_bytes(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 v3 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 1fd584a..5b1fc32 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1089,7 +1089,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 v3 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 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_bytes":273535729664,
      "active_persistent_bytes":0,
      "next_volatile_bytes":0,
      "next_persistent_bytes":0,
      "total_bytes":273535729664,
      "volatile_only_bytes":0,
      "persistent_only_bytes":0,
      "partition_alignment_bytes":268435456
    }

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

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index c8d10fb..912ac11 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -85,6 +85,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_bytes":273535729664,
+      "active_persistent_bytes":0,
+      "next_volatile_bytes":0,
+      "next_persistent_bytes":0,
+      "total_bytes":273535729664,
+      "volatile_only_bytes":0,
+      "persistent_only_bytes":0,
+      "partition_alignment_bytes":268435456
+    }
+  }
+]
+----
 
 include::human-option.txt[]
 
diff --git a/cxl/json.c b/cxl/json.c
index e562502..e51a96e 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2015-2020 Intel Corporation. All rights reserved.
+#include <limits.h>
 #include <util/json.h>
 #include <uuid/uuid.h>
 #include <cxl/libcxl.h>
@@ -183,6 +184,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_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_partition_info_get_active_volatile_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_volatile_bytes", jobj);
+	}
+	cap = cxl_cmd_partition_info_get_active_persistent_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"active_persistent_bytes", jobj);
+	}
+	cap = cxl_cmd_partition_info_get_next_volatile_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_volatile_bytes", jobj);
+	}
+	cap = cxl_cmd_partition_info_get_next_persistent_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"next_persistent_bytes", 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_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart, "total_bytes", jobj);
+	}
+	cap = cxl_cmd_identify_get_volatile_only_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"volatile_only_bytes", jobj);
+	}
+	cap = cxl_cmd_identify_get_persistent_only_bytes(cmd);
+	if (cap != ULLONG_MAX) {
+		jobj = util_json_object_size(cap, flags);
+		if (jobj)
+			json_object_object_add(jpart,
+					"persistent_only_bytes", 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_bytes", 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)
 {
@@ -210,5 +319,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_info", jobj);
+	}
 	return jdev;
 }
diff --git a/cxl/list.c b/cxl/list.c
index 7f7a04d..73dc390 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -19,6 +19,7 @@ static struct {
 	bool idle;
 	bool human;
 	bool health;
+	bool partition;
 } list;
 
 static unsigned long listopts_to_flags(void)
@@ -31,6 +32,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;
 }
 
@@ -64,6 +67,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('I', "partition", &list.partition,
+				"include memory device partition information "),
 		OPT_END(),
 	};
 	const char * const u[] = {
diff --git a/util/json.h b/util/json.h
index 4ca2c89..f198036 100644
--- a/util/json.h
+++ b/util/json.h
@@ -17,6 +17,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;
-- 
2.31.1

[ndctl PATCH v3 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 as defined in the CXL 2.0 specification.

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

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 5b1fc32..5a5b189 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1230,6 +1230,21 @@ cxl_cmd_partition_info_get_next_persistent_bytes(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;
@@ -1428,3 +1443,38 @@ 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;
+}
+
+CXL_EXPORT int cxl_cmd_partition_info_flag_immediate(void)
+{
+	return CXL_CMD_SET_PARTITION_INFO_FLAG_IMMEDIATE;
+}
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index b7e969f..0ce931d 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -78,6 +78,11 @@ global:
 	cxl_cmd_partition_info_get_active_persistent_bytes;
 	cxl_cmd_partition_info_get_next_volatile_bytes;
 	cxl_cmd_partition_info_get_next_persistent_bytes;
+	cxl_cmd_new_set_partition_info;
+	cxl_memdev_set_partition_info;
+	cxl_cmd_partition_info_flag_none;
+	cxl_cmd_partition_info_flag_immediate;
+
 local:
         *;
 };
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index dd9234f..4da8ea7 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -114,6 +114,14 @@ 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_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 46f99fb..9b0a599 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,9 @@ unsigned long long cxl_cmd_partition_info_get_active_volatile_bytes(struct cxl_c
 unsigned long long cxl_cmd_partition_info_get_active_persistent_bytes(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_partition_info_get_next_volatile_bytes(struct cxl_cmd *cmd);
 unsigned long long cxl_cmd_partition_info_get_next_persistent_bytes(struct cxl_cmd *cmd);
+struct cxl_cmd *cxl_cmd_new_set_partition_info(struct cxl_memdev *memdev,
+		unsigned long long volatile_capacity, int flags);
+int cxl_cmd_partition_info_flag_immediate(void);
 
 #ifdef __cplusplus
 } /* extern "C" */
-- 
2.31.1

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

[alison.schofield]

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

Users may want to change the partition layout of a memory
device using the CXL command line tool. Add a new CXL command,
'cxl set-partition-info', that operates on a CXL memdev, or a
set of memdevs, and allows the user to change the partition
layout of the 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 included MAN page explains how to find the partitioning
capabilities and restrictions of a CXL memory device.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/cxl-set-partition-info.txt |  53 ++++++++++
 Documentation/cxl/meson.build                |   1 +
 cxl/builtin.h                                |   1 +
 cxl/cxl.c                                    |   1 +
 cxl/memdev.c                                 | 101 +++++++++++++++++++
 5 files changed, 157 insertions(+)
 create mode 100644 Documentation/cxl/cxl-set-partition-info.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..d99a1b9
--- /dev/null
+++ b/Documentation/cxl/cxl-set-partition-info.txt
@@ -0,0 +1,53 @@
+// 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>]'
+
+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[]
+
+-s::
+--size=::
+        Size in bytes of the volatile partition requested.
+
+        Size must align to the devices partition_alignment_bytes.
+        Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes.
+
+        Size must be less than or equal to the device's partitionable bytes.
+        Calculate partitionable bytes by subracting the volatile_only_bytes,
+        and the persistent_only_bytes, from the total_bytes.
+        Use 'cxl list -m <memdev> -I' to find the above mentioned_byte values.
+
+-v::
+        Turn on verbose debug messages in the library (if libcxl was built with
+        logging and debug enabled).
+
+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 64ce13f..0108eea 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -28,6 +28,7 @@ cxl_manpages = [
   'cxl-read-labels.txt',
   'cxl-write-labels.txt',
   'cxl-zero-labels.txt',
+  'cxl-set-partition-info.txt',
 ]
 
 foreach man : cxl_manpages
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 d063d51..e1348c8 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 <cxl/libcxl.h>
 #include <util/parse-options.h>
 #include <ccan/minmax/minmax.h>
@@ -24,6 +25,7 @@ static struct parameters {
 	unsigned len;
 	unsigned offset;
 	bool verbose;
+	const char *volatile_size;
 } param;
 
 #define fail(fmt, ...) \
@@ -48,6 +50,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_STRING('s', "volatile_size",  &param.volatile_size, "volatile-size", \
+	"next volatile partition size in bytes")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
 	LABEL_OPTIONS(),
@@ -68,6 +74,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;
@@ -175,6 +187,80 @@ 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_bytes(cmd);
+	volatile_only = cxl_cmd_identify_get_volatile_only_bytes(cmd);
+	persistent_only = cxl_cmd_identify_get_persistent_only_bytes(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 = parse_size64(param.volatile_size);
+	if (volatile_request == ULLONG_MAX) {
+		fprintf(stderr, "%s: failed to parse volatile size '%s'\n",
+			devname, param.volatile_size);
+		return -EINVAL;
+	}
+
+	rc = validate_partition(memdev, volatile_request);
+	if (rc)
+		return rc;
+
+	rc = cxl_memdev_set_partition_info(memdev, volatile_request,
+			!cxl_cmd_partition_info_flag_immediate());
+	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)
@@ -235,6 +321,11 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
 		}
 	}
 
+	if (action == action_set_partition && !param.volatile_size) {
+		usage_with_options(u, options);
+		return -EINVAL;
+	}
+
 	if (param.verbose)
 		cxl_set_log_priority(ctx, LOG_DEBUG);
 
@@ -323,3 +414,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 v3 1/6] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 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>

Looks good to me, you might want to also add a short note about the
"cxl_cmd_new_get_partition_info()" API in the "=== MEMDEV: Commands"
section of Documentation/cxl/lib/libcxl.txt that I started here:

https://lore.kernel.org/r/164298557771.3021641.14904324834528700206.stgit@dwillia2-desk3.amr.corp.intel.com

Note that I'm not adding every single API there, but I think each
cxl_cmd_new_<command_type>() API could use a short note.

That can be a follow on depending on whether Vishal merges this first
or the topology enumeration series.

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

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

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 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
> 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   | 29 +++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  3 +++
>  cxl/libcxl.h       |  3 +++
>  3 files changed, 35 insertions(+)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 58181c0..1fd584a 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1105,6 +1105,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)

This could probably just be 2 functions

cmd_to_identify()
capacity_to_bytes()

...and then the caller does:

return capactity_to_bytes(cmd_to_identify()->field);

...and skip the macro. Otherwise, a macro with a "return" statement
can be unwieldy unless that macro is defining an entire function.

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

[Alison Schofield]

On Wed, Jan 26, 2022 at 08:07:17AM -0800, Dan Williams wrote:
> On Tue, Jan 18, 2022 at 12:20 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>
> 
> Looks good to me, you might want to also add a short note about the
> "cxl_cmd_new_get_partition_info()" API in the "=== MEMDEV: Commands"
> section of Documentation/cxl/lib/libcxl.txt that I started here:
> 
> https://lore.kernel.org/r/164298557771.3021641.14904324834528700206.stgit@dwillia2-desk3.amr.corp.intel.com

Will do.

> 
> Note that I'm not adding every single API there, but I think each
> cxl_cmd_new_<command_type>() API could use a short note.
> 
> That can be a follow on depending on whether Vishal merges this first
> or the topology enumeration series.

Vishal - I think this should follow the topology enumeration series
because it wants to use the cxl_filter_walk() that the topo series
introduces. (to spit out the updated partition info upon completion
of the set-partition-info cmd.)

So, a v4 posting will apply after topo series.

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

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

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 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.
>
> 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 1fd584a..5b1fc32 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1089,7 +1089,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;
>  }
>

Looks ok, and likely no one has noticed the old behavior. If someone
does notice though we'll likely need to introduce a new
cxl_cmd_identify_get_partition_align_bytes() and support both styles
forever.

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

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

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
>
> 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 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_bytes":273535729664,
>       "active_persistent_bytes":0,
>       "next_volatile_bytes":0,
>       "next_persistent_bytes":0,
>       "total_bytes":273535729664,
>       "volatile_only_bytes":0,
>       "persistent_only_bytes":0,
>       "partition_alignment_bytes":268435456
>     }
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-list.txt |  23 +++++++
>  cxl/json.c                     | 114 +++++++++++++++++++++++++++++++++
>  cxl/list.c                     |   5 ++
>  util/json.h                    |   1 +
>  4 files changed, 143 insertions(+)
>
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index c8d10fb..912ac11 100644
> --- a/Documentation/cxl/cxl-list.txt
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -85,6 +85,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_bytes":273535729664,
> +      "active_persistent_bytes":0,
> +      "next_volatile_bytes":0,
> +      "next_persistent_bytes":0,
> +      "total_bytes":273535729664,
> +      "volatile_only_bytes":0,
> +      "persistent_only_bytes":0,
> +      "partition_alignment_bytes":268435456

I think it's confusing to include "_bytes" in the json listing as it's
not used in any of the other byte oriented output fields across 'cxl
list' and 'ndctl list'. "_size" would match similar fields in other
json objects.

Other than that,

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

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

[Verma, Vishal L]

On Wed, 2022-01-26 at 08:37 -0800, Alison Schofield wrote:
> On Wed, Jan 26, 2022 at 08:07:17AM -0800, Dan Williams wrote:
> > On Tue, Jan 18, 2022 at 12:20 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>
> > 
> > Looks good to me, you might want to also add a short note about the
> > "cxl_cmd_new_get_partition_info()" API in the "=== MEMDEV: Commands"
> > section of Documentation/cxl/lib/libcxl.txt that I started here:
> > 
> > https://lore.kernel.org/r/164298557771.3021641.14904324834528700206.stgit@dwillia2-desk3.amr.corp.intel.com
> 
> Will do.
> 
> > 
> > Note that I'm not adding every single API there, but I think each
> > cxl_cmd_new_<command_type>() API could use a short note.
> > 
> > That can be a follow on depending on whether Vishal merges this first
> > or the topology enumeration series.
> 
> Vishal - I think this should follow the topology enumeration series
> because it wants to use the cxl_filter_walk() that the topo series
> introduces. (to spit out the updated partition info upon completion
> of the set-partition-info cmd.)
> 
> So, a v4 posting will apply after topo series.

Yep that sounds good to me.

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

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

[Alison Schofield]

On Wed, Jan 26, 2022 at 09:23:23AM -0800, Dan Williams wrote:
> On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
> >
snip

> > +# cxl list -m mem0 -I
> > +[
> > +  {
> > +    "memdev":"mem0",
> > +    "pmem_size":0,
> > +    "ram_size":273535729664,
> > +    "partition_info":{
> > +      "active_volatile_bytes":273535729664,
> > +      "active_persistent_bytes":0,
> > +      "next_volatile_bytes":0,
> > +      "next_persistent_bytes":0,
> > +      "total_bytes":273535729664,
> > +      "volatile_only_bytes":0,
> > +      "persistent_only_bytes":0,
> > +      "partition_alignment_bytes":268435456
> 
> I think it's confusing to include "_bytes" in the json listing as it's
> not used in any of the other byte oriented output fields across 'cxl
> list' and 'ndctl list'. "_size" would match similar fields in other
> json objects.

Got it. Will drop the _bytes in v4. Thanks!
> 
> Other than that,
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

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

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 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 as defined in the CXL 2.0 specification.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  5 +++++
>  cxl/lib/private.h  |  8 ++++++++
>  cxl/libcxl.h       |  5 +++++
>  4 files changed, 68 insertions(+)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 5b1fc32..5a5b189 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1230,6 +1230,21 @@ cxl_cmd_partition_info_get_next_persistent_bytes(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;

->in.payload is a 'void *', no casting required.


> +       set_partition->volatile_capacity = cpu_to_le64(volatile_capacity);
> +       set_partition->flags = flags;

This has the potential to bite if users get accustomed passing in raw
values directly... more below.

> +       return cmd;
> +}
> +
>  CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
>  {
>         struct cxl_memdev *memdev = cmd->memdev;
> @@ -1428,3 +1443,38 @@ 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);

Given that the get-partition-info helpers emit 'bytes' I think it
would make sense for the set-partition-info to take bytes, i.e. move
the conversion to the 256MB shifted value internal to
xl_cmd_new_set_partition_info().


> +       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;
> +}
> +
> +CXL_EXPORT int cxl_cmd_partition_info_flag_immediate(void)
> +{
> +       return CXL_CMD_SET_PARTITION_INFO_FLAG_IMMEDIATE;
> +}

I don't understand what this is for?

Let's back up. In order to future proof against spec changes, and
endianness, struct packing and all other weird things that make struct
ABIs hard to maintain compatibility the ndctl project adopts the
libabc template of just not letting library consumers see any raw data
structures or bit fields by default [1]. For a situation like this
since the command only has one flag that affects the mode of operation
I would just go ahead and define an enum for that explicitly.

enum cxl_setpartition_mode {
    CXL_SETPART_NONE,
    CXL_SETPART_NEXTBOOT,
    CXL_SETPART_IMMEDIATE,
};

Then the main function prototype becomes:

int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long
long volatile_capacity);

...with a new:

int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum
cxl_setpartition_mode mode);

...and it becomes impossible for users to pass unsupported flag
values. If the specification later on adds more flags then we can add
more:

int cxl_cmd_setpartition_set_<X>(struct cxl_cmd *cmd, enum
cxl_setpartition_X x);

...style helpers.

Note, I was thinking CXL_SETPART_NONE is there to catch API users that
forget to set a mode, but I also don't mind skipping that and just
defaulting cxl_cmd_new_setpartition() to CXL_SETPART_NEXTBOOT, up to
you.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git/tree/README#n99

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

[Dan Williams]

On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> Users may want to change the partition layout of a memory
> device using the CXL command line tool.

How about:

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.

> Add a new CXL command,
> 'cxl set-partition-info', that operates on a CXL memdev, or a
> set of memdevs, and allows the user to change the partition
> layout of the device(s).
>
> Synopsis:
> Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]

It's unfortunate that the specification kept the word "info" in the
set-partition command name, let's leave it out of this name and just
call this 'cxl set-partition'.

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

Some users will come to this tool with the thought, "I want volatile
capacity X", others "I want pmem capacity Y". All but the most
advanced users will not understand how the volatile setting affects
the pmem and vice versa, nor will they understand that capacity might
be fixed and other capacity is dynamic. So how about a set of options
like:

-t, --type=<volatile,pmem>
-s,--size=<size>

...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic
capacity set to PMEM.

The tool can then help the user get what they want relative to
whatever frame of reference led them to read the set-partition man
page. The tool can figure out the details behind the scenes, or warn
them if they want something impossible like setting PMEM capacity to
1GB on a device where 2GB of PMEM is statically allocated.

This also helps with future proofing in case there are other types
that come along.

>
> The included MAN page explains how to find the partitioning
> capabilities and restrictions of a CXL memory device.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/cxl/cxl-set-partition-info.txt |  53 ++++++++++
>  Documentation/cxl/meson.build                |   1 +
>  cxl/builtin.h                                |   1 +
>  cxl/cxl.c                                    |   1 +
>  cxl/memdev.c                                 | 101 +++++++++++++++++++
>  5 files changed, 157 insertions(+)
>  create mode 100644 Documentation/cxl/cxl-set-partition-info.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..d99a1b9
> --- /dev/null
> +++ b/Documentation/cxl/cxl-set-partition-info.txt
> @@ -0,0 +1,53 @@
> +// 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>]'
> +
> +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[]
> +
> +-s::
> +--size=::

This is "--volatile_size" in the current patch, but that's moot with
feedback to move to a --type + --size scheme.

> +        Size in bytes of the volatile partition requested.
> +
> +        Size must align to the devices partition_alignment_bytes.
> +        Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes.

This a static 256M, that can just be documented here.

> +
> +        Size must be less than or equal to the device's partitionable bytes.
> +        Calculate partitionable bytes by subracting the volatile_only_bytes,

s/subracting/subtracting/

...but rather than teaching the user to input valid values, tell them
the validation and translation the tool will do on their behalf with
the input based on type and the details they don't need to worry
about.

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

[Alison Schofield]

On Wed, Jan 26, 2022 at 05:44:54PM -0800, Dan Williams wrote:
> On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users may want to change the partition layout of a memory
> > device using the CXL command line tool.
> 
> How about:
> 
> 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.

Thanks!
> 
> > Add a new CXL command,
> > 'cxl set-partition-info', that operates on a CXL memdev, or a
> > set of memdevs, and allows the user to change the partition
> > layout of the device(s).
> >
> > Synopsis:
> > Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
> 
> It's unfortunate that the specification kept the word "info" in the
> set-partition command name, let's leave it out of this name and just
> call this 'cxl set-partition'.
> 
Will do. 

> >
> >         -v, --verbose           turn on debug
> >         -s, --volatile_size <n>
> >                                 next volatile partition size in bytes
> 
> Some users will come to this tool with the thought, "I want volatile
> capacity X", others "I want pmem capacity Y". All but the most
> advanced users will not understand how the volatile setting affects
> the pmem and vice versa, nor will they understand that capacity might
> be fixed and other capacity is dynamic.

I went very by the spec here, with those advance users in mind. I do
think the user with limited knowledge may get frustrated by the rules.
ie. like finding that their total capacity is not all partitionable
capacity. I looked at ipmctl goal setting, where they do percentages,
and didn't pursue - stuck with the spec.

I like this below. If user wants 100% of any type, no math needed. But,
anything else the user will have to specify a byte value.

> So how about a set of options like:
> 
> -t, --type=<volatile,pmem>
> -s,--size=<size>
> 
> ...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic
> capacity set to PMEM.
>
I don't get the language "and -s is '0' for 100%", specifically, the
"-s is 0".

If -s is ommitted the default behavior will be to set 100% of the
partitionable capacity to type.

If both type and size are ommitted, 100% of partitionable capacity is
set to PMEM.

Humor me...are these right?  

set-partition mem0
	100% to pmem

set-partition mem0 -tpmem 
	100% to pmem

set-partition mem0 -tvolatile -s0
	100% to pmem          ^^ That's what I think a -s0 does?!?

set-partition mem0 -tvolatile
	100% to volatile

set-partition mem0 -tvolatile -s256MB
	256MB to volatile
	Remainder to pmem

set-partition mem0 -s256MB
	256MB to pmem
	Remainder to volatile

Thanks!

> The tool can then help the user get what they want relative to
> whatever frame of reference led them to read the set-partition man
> page. The tool can figure out the details behind the scenes, or warn
> them if they want something impossible like setting PMEM capacity to
> 1GB on a device where 2GB of PMEM is statically allocated.
> 
> This also helps with future proofing in case there are other types
> that come along.
> 
Got it.

> >
> > The included MAN page explains how to find the partitioning
> > capabilities and restrictions of a CXL memory device.
> >
snip

> > +-------
> > +<memory device(s)>::
> > +include::memdev-option.txt[]
> > +
> > +-s::
> > +--size=::
> 
> This is "--volatile_size" in the current patch, but that's moot with
> feedback to move to a --type + --size scheme.

Got it.

> 
> > +        Size in bytes of the volatile partition requested.
> > +
> > +        Size must align to the devices partition_alignment_bytes.
> > +        Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes.
> 
> This a static 256M, that can just be documented here.
> 
My read of the spec says partition alignment is "Expressed in multiples
of 256MB", not static. Where is it defined as static?

> > +
> > +        Size must be less than or equal to the device's partitionable bytes.
> > +        Calculate partitionable bytes by subracting the volatile_only_bytes,
> 
> s/subracting/subtracting/
> 
> ...but rather than teaching the user to input valid values, tell them
> the validation and translation the tool will do on their behalf with
> the input based on type and the details they don't need to worry
> about.

Got it.

Thanks Dan.

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

[Dan Williams]

On Wed, Jan 26, 2022 at 9:40 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> On Wed, Jan 26, 2022 at 05:44:54PM -0800, Dan Williams wrote:
> > On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
> > >
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > Users may want to change the partition layout of a memory
> > > device using the CXL command line tool.
> >
> > How about:
> >
> > 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.
>
> Thanks!
> >
> > > Add a new CXL command,
> > > 'cxl set-partition-info', that operates on a CXL memdev, or a
> > > set of memdevs, and allows the user to change the partition
> > > layout of the device(s).
> > >
> > > Synopsis:
> > > Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]
> >
> > It's unfortunate that the specification kept the word "info" in the
> > set-partition command name, let's leave it out of this name and just
> > call this 'cxl set-partition'.
> >
> Will do.
>
> > >
> > >         -v, --verbose           turn on debug
> > >         -s, --volatile_size <n>
> > >                                 next volatile partition size in bytes
> >
> > Some users will come to this tool with the thought, "I want volatile
> > capacity X", others "I want pmem capacity Y". All but the most
> > advanced users will not understand how the volatile setting affects
> > the pmem and vice versa, nor will they understand that capacity might
> > be fixed and other capacity is dynamic.
>
> I went very by the spec here, with those advance users in mind. I do
> think the user with limited knowledge may get frustrated by the rules.
> ie. like finding that their total capacity is not all partitionable
> capacity. I looked at ipmctl goal setting, where they do percentages,
> and didn't pursue - stuck with the spec.
>
> I like this below. If user wants 100% of any type, no math needed. But,
> anything else the user will have to specify a byte value.
>
> > So how about a set of options like:
> >
> > -t, --type=<volatile,pmem>
> > -s,--size=<size>
> >
> > ...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic
> > capacity set to PMEM.
> >
> I don't get the language "and -s is '0' for 100%", specifically, the
> "-s is 0".
>
> If -s is ommitted the default behavior will be to set 100% of the
> partitionable capacity to type.

Right, sorry, yes I should have been clear that "-s 0" on the command
line means zero-out that capacity, but no "-s" is a default value that
means 100% of the given type.

I forgot that NULL can be used to detect string options not specified.

>
> If both type and size are ommitted, 100% of partitionable capacity is
> set to PMEM.
>
> Humor me...are these right?
>
> set-partition mem0
>         100% to pmem

yup.

>
> set-partition mem0 -tpmem
>         100% to pmem

I thought you need a space after short options, but apparently not.

>
> set-partition mem0 -tvolatile -s0
>         100% to pmem          ^^ That's what I think a -s0 does?!?
>

Agree, looks good.

> set-partition mem0 -tvolatile
>         100% to volatile

yup.

>
> set-partition mem0 -tvolatile -s256MB
>         256MB to volatile
>         Remainder to pmem
>
> set-partition mem0 -s256MB
>         256MB to pmem
>         Remainder to volatile

Yup, thanks for going through the examples to clarify.

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

[Alison Schofield]

Hi Dan,
Thanks for the review. I'm still working thru this, but a clarifying
question below...

On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote:
> On Tue, Jan 18, 2022 at 12:20 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 as defined in the CXL 2.0 specification.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym |  5 +++++
> >  cxl/lib/private.h  |  8 ++++++++
> >  cxl/libcxl.h       |  5 +++++
> >  4 files changed, 68 insertions(+)
> >
snip
> 
> 
> I don't understand what this is for?
> 
> Let's back up. In order to future proof against spec changes, and
> endianness, struct packing and all other weird things that make struct
> ABIs hard to maintain compatibility the ndctl project adopts the
> libabc template of just not letting library consumers see any raw data
> structures or bit fields by default [1]. For a situation like this
> since the command only has one flag that affects the mode of operation
> I would just go ahead and define an enum for that explicitly.
> 
> enum cxl_setpartition_mode {
>     CXL_SETPART_NONE,
>     CXL_SETPART_NEXTBOOT,
>     CXL_SETPART_IMMEDIATE,
> };
> 
> Then the main function prototype becomes:
> 
> int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long
> long volatile_capacity);
> 
> ...with a new:
> 
> int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum
> cxl_setpartition_mode mode);
>

I don't understand setting of the mode separately. Can it be:

int cxl_cmd_new_setpartition(struct cxl_memdev *memdev,
			     unsigned long long volatile_capacity,
			     enum cxl_setpartition_mode mode);



> ...and it becomes impossible for users to pass unsupported flag
> values. If the specification later on adds more flags then we can add
> more:
> 
> int cxl_cmd_setpartition_set_<X>(struct cxl_cmd *cmd, enum
> cxl_setpartition_X x);
> 
> ...style helpers.
> 
> Note, I was thinking CXL_SETPART_NONE is there to catch API users that
> forget to set a mode, but I also don't mind skipping that and just
> defaulting cxl_cmd_new_setpartition() to CXL_SETPART_NEXTBOOT, up to
> you.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git/tree/README#n99

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

[Dan Williams]

On Thu, Jan 27, 2022 at 12:46 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> Hi Dan,
> Thanks for the review. I'm still working thru this, but a clarifying
> question below...
>
> On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote:
> > On Tue, Jan 18, 2022 at 12:20 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 as defined in the CXL 2.0 specification.
> > >
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  cxl/lib/libcxl.sym |  5 +++++
> > >  cxl/lib/private.h  |  8 ++++++++
> > >  cxl/libcxl.h       |  5 +++++
> > >  4 files changed, 68 insertions(+)
> > >
> snip
> >
> >
> > I don't understand what this is for?
> >
> > Let's back up. In order to future proof against spec changes, and
> > endianness, struct packing and all other weird things that make struct
> > ABIs hard to maintain compatibility the ndctl project adopts the
> > libabc template of just not letting library consumers see any raw data
> > structures or bit fields by default [1]. For a situation like this
> > since the command only has one flag that affects the mode of operation
> > I would just go ahead and define an enum for that explicitly.
> >
> > enum cxl_setpartition_mode {
> >     CXL_SETPART_NONE,
> >     CXL_SETPART_NEXTBOOT,
> >     CXL_SETPART_IMMEDIATE,
> > };
> >
> > Then the main function prototype becomes:
> >
> > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long
> > long volatile_capacity);
> >
> > ...with a new:
> >
> > int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum
> > cxl_setpartition_mode mode);
> >
>
> I don't understand setting of the mode separately. Can it be:
>
> int cxl_cmd_new_setpartition(struct cxl_memdev *memdev,
>                              unsigned long long volatile_capacity,
>                              enum cxl_setpartition_mode mode);

It could be, but what happens when the specification defines a new
flag for this command? Then we would have cxl_cmd_new_setpartition()
and cxl_cmd_new_setpartition2()  to add the new parameters. A helper
function after establishing the cxl_cmd context lets you have
flexibility to extend the base command by as many new flags and modes
that come along... hopefully none, but you never know.

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

[Alison Schofield]

Dan, One follow up below...

On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote:
> On Tue, Jan 18, 2022 at 12:20 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 as defined in the CXL 2.0 specification.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym |  5 +++++
> >  cxl/lib/private.h  |  8 ++++++++
> >  cxl/libcxl.h       |  5 +++++
> >  4 files changed, 68 insertions(+)
> >
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 5b1fc32..5a5b189 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1230,6 +1230,21 @@ cxl_cmd_partition_info_get_next_persistent_bytes(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;
> 
> ->in.payload is a 'void *', no casting required.
> 

send_cmd->in.payload is a __u64 so this cast is needed.

Of course, I wondered what Dan was thinking ;) and I see that struct
cxl_cmd's input_payload is indeed a 'void *'.

I believe this is the correct payload here, umm..  because it works ;)

But - let me know if you suspect something is off here.

Thanks!




snip
>

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

[Dan Williams]

On Mon, Jan 31, 2022 at 5:20 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> Dan, One follow up below...
>
> On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote:
> > On Tue, Jan 18, 2022 at 12:20 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 as defined in the CXL 2.0 specification.
> > >
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  cxl/lib/libcxl.sym |  5 +++++
> > >  cxl/lib/private.h  |  8 ++++++++
> > >  cxl/libcxl.h       |  5 +++++
> > >  4 files changed, 68 insertions(+)
> > >
> > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > > index 5b1fc32..5a5b189 100644
> > > --- a/cxl/lib/libcxl.c
> > > +++ b/cxl/lib/libcxl.c
> > > @@ -1230,6 +1230,21 @@ cxl_cmd_partition_info_get_next_persistent_bytes(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;
> >
> > ->in.payload is a 'void *', no casting required.
> >
>
> send_cmd->in.payload is a __u64 so this cast is needed.
>
> Of course, I wondered what Dan was thinking ;) and I see that struct
> cxl_cmd's input_payload is indeed a 'void *'.
>
> I believe this is the correct payload here, umm..  because it works ;)
>
> But - let me know if you suspect something is off here.

You're right...

...however :), I think this casting is what the core
cxl_cmd_alloc_send() helper is doing for everyone when it does:

cmd->send_cmd->in.payload = (u64)cmd->input_payload;

...so cxl_cmd_new_set_partition_info() can safely switch to the
shorter cmd->input_payload and drop its own redundant local casting.

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

[Alison Schofield]

> > snip
> > >
> > >
> > > I don't understand what this is for?
> > >
> > > Let's back up. In order to future proof against spec changes, and
> > > endianness, struct packing and all other weird things that make struct
> > > ABIs hard to maintain compatibility the ndctl project adopts the
> > > libabc template of just not letting library consumers see any raw data
> > > structures or bit fields by default [1]. For a situation like this
> > > since the command only has one flag that affects the mode of operation
> > > I would just go ahead and define an enum for that explicitly.
> > >
> > > enum cxl_setpartition_mode {
> > >     CXL_SETPART_NONE,
> > >     CXL_SETPART_NEXTBOOT,
> > >     CXL_SETPART_IMMEDIATE,
> > > };
> > >
> > > Then the main function prototype becomes:
> > >
> > > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long
> > > long volatile_capacity);
> > >
> > > ...with a new:
> > >
> > > int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum
> > > cxl_setpartition_mode mode);
> > >
> >
> > I don't understand setting of the mode separately. Can it be:
> >
> > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev,
> >                              unsigned long long volatile_capacity,
> >                              enum cxl_setpartition_mode mode);
> 
> It could be, but what happens when the specification defines a new
> flag for this command? Then we would have cxl_cmd_new_setpartition()
> and cxl_cmd_new_setpartition2()  to add the new parameters. A helper
> function after establishing the cxl_cmd context lets you have
> flexibility to extend the base command by as many new flags and modes
> that come along... hopefully none, but you never know.

Got it. Doing the 'new' followed by 'mode' set up you suggested.
(Sorry, I didn't update this thread after our offline chat.)