[ndctl PATCH v2 00/10] cxl: add region management

[ndctl PATCH v2 00/10] cxl: add region management

[Vishal Verma]

Changes since v1[1]:
- Collect review tags
- Fix order of setting region size vs UUID (Dan)
- Rename region_{commit,decommit} to region_decode{commit,reset} everywhere (Dan)
- Add a -b <bus> option to all region management commands
- Add a cxl-create-region unit test
- Use walk_children pattern for root decoders vs. regions (Dan)
- Add bus, port, and (root) decoder based filters for regions (Dan)
- Add a 'REGIONS' section to the libcxl(3) documentation (Dan)
- s/--interleave-ways/--ways/ and
  s/interleave-granularity/--granularity/ (Dan)
- Rename and clarify memdev_match_set_size() with a comment (Dan)
- Remove a double init in memdev_match_set_size() (Dan)
- Rework granularity determination (Dan)
- Ensure ep_decoder is free before changing its mode (Dan)
- Fix a mid-series compile breakage in region_action() (Dan)
- Fix missing whitespace in region_action() (Dan)
- Refactor region_action for too much indentation (Dan)
- Save error state from multi-region operations in region_action()
- Free leaky region objects (when the parent decoder is freed)
- Consider root decoder's available size before creating a region (Dan)


[1]: https://lore.kernel.org/linux-cxl/20220715062550.789736-1-vishal.l.verma@intel.com/

---

Introduce the first cut at a 'cxl create-region' command, which uses the
ABI merged in the kernel in [2], and builds on the create-region foundation
laid out in [3].

This introduces libcxl objects for regions and their memdev mappings,
adds listing support for them, and libcxl APIs to create, enable,
disable, and destroy regions. These are consumed by respective cxl-cli
commands to perform the same region management operations.

The 'cxl create-region' command deserves a bit more detail. Its current
version can be considered a fundamental 'plumbing' implementation. It
allows, and in some cases, requires the user to specify the parameters
for the region being created. Future enhancements to this will allow a
simple 'cxl create-region' invocation to automatically pick suitable
root decoders, memdev endpoints, and interleave settings to create a
maximally sized and interleaved region.

Today, the create-region command requires the following from the user:

  -d <root decoder> : the root decoder (CFMWS window) under which to
  create the region

  -m or -e <targets>: the memdev or endpoint decoder targets that will
  form the new region

It can pick the following settings automatically (though the user can
also override these if desired):

  -w : interleave ways - picked based on the number of memdev / endpoint
  decoder targets supplied

  -g : interleave granularity - selects the interleave granularity
  advertised by the root decoder

  -t : type of region - defaults to pmem. The ram (volatile) type is not
  supported yet.

  -s : size of region - deduced based on sizes of the specified targets

[2]: https://git.kernel.org/torvalds/c/c235698355fa94df7073b51befda7d4be00a0e23
[3]: https://lore.kernel.org/linux-cxl/165781810717.1555691.1411727384567016588.stgit@dwillia2-xfh.jf.intel.com/


Vishal Verma (10):
  libcxl: add a depth attribute to cxl_port
  cxl/port: Consolidate the debug option in cxl-port man pages
  libcxl: Introduce libcxl region and mapping objects
  cxl-cli: add region listing support
  libcxl: add low level APIs for region creation
  cxl: add a 'create-region' command
  cxl: add commands to {enable,disable,destroy}-region
  cxl/list: make memdevs and regions the default listing
  test: add a cxl-create-region test
  cxl/decoder: add a max_available_extent attribute

 Documentation/cxl/bus-option.txt         |   5 +
 Documentation/cxl/cxl-create-region.txt  | 114 +++
 Documentation/cxl/cxl-destroy-region.txt |  41 ++
 Documentation/cxl/cxl-disable-port.txt   |   5 +-
 Documentation/cxl/cxl-disable-region.txt |  36 +
 Documentation/cxl/cxl-enable-port.txt    |   5 +-
 Documentation/cxl/cxl-enable-region.txt  |  36 +
 Documentation/cxl/cxl-list.txt           |  13 +-
 Documentation/cxl/debug-option.txt       |   4 +
 Documentation/cxl/decoder-option.txt     |   6 +
 Documentation/cxl/lib/libcxl.txt         |  69 ++
 Documentation/cxl/region-description.txt |   7 +
 cxl/lib/private.h                        |  38 +
 cxl/lib/libcxl.c                         | 889 ++++++++++++++++++++++-
 cxl/builtin.h                            |   4 +
 cxl/filter.h                             |   6 +
 cxl/json.h                               |   5 +
 cxl/libcxl.h                             |  67 +-
 cxl/cxl.c                                |   4 +
 cxl/filter.c                             | 158 +++-
 cxl/json.c                               | 131 ++++
 cxl/list.c                               |  26 +-
 cxl/region.c                             | 797 ++++++++++++++++++++
 .clang-format                            |   2 +
 Documentation/cxl/meson.build            |   7 +
 cxl/lib/libcxl.sym                       |  37 +
 cxl/meson.build                          |   1 +
 test/cxl-create-region.sh                | 126 ++++
 test/meson.build                         |   2 +
 29 files changed, 2598 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/cxl/bus-option.txt
 create mode 100644 Documentation/cxl/cxl-create-region.txt
 create mode 100644 Documentation/cxl/cxl-destroy-region.txt
 create mode 100644 Documentation/cxl/cxl-disable-region.txt
 create mode 100644 Documentation/cxl/cxl-enable-region.txt
 create mode 100644 Documentation/cxl/debug-option.txt
 create mode 100644 Documentation/cxl/decoder-option.txt
 create mode 100644 Documentation/cxl/region-description.txt
 create mode 100644 cxl/region.c
 create mode 100644 test/cxl-create-region.sh

-- 
2.37.1

[ndctl PATCH v2 01/10] libcxl: add a depth attribute to cxl_port

[Vishal Verma]

Add a depth attribute to the cxl_port structure, that can be used for
calculating its distance from the root port, and will be needed for
interleave granularity calculations during region creation.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/lib/private.h | 1 +
 cxl/lib/libcxl.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index f6d4573..832a815 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -66,6 +66,7 @@ struct cxl_port {
 	int decoders_init;
 	int dports_init;
 	int nr_dports;
+	int depth;
 	struct cxl_ctx *ctx;
 	struct cxl_bus *bus;
 	enum cxl_port_type type;
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index be6bc2c..946cd4b 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -744,6 +744,7 @@ static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port,
 	port->type = type;
 	port->parent = parent_port;
 	port->type = type;
+	port->depth = parent_port ? parent_port->depth + 1 : 0;
 
 	list_head_init(&port->child_ports);
 	list_head_init(&port->endpoints);
-- 
2.37.1

[ndctl PATCH v2 02/10] cxl/port: Consolidate the debug option in cxl-port man pages

[Vishal Verma]

In preparation for additional commands that implement the --debug
option, consolidate the option description from the cxl-port man pages
into an include.

The port man pages also mentioned the debug option requiring a build
with debug enabled, which wasn't true - so remove that part.

Cc: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-disable-port.txt | 5 +----
 Documentation/cxl/cxl-enable-port.txt  | 5 +----
 Documentation/cxl/debug-option.txt     | 4 ++++
 Documentation/cxl/meson.build          | 1 +
 4 files changed, 7 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/cxl/debug-option.txt

diff --git a/Documentation/cxl/cxl-disable-port.txt b/Documentation/cxl/cxl-disable-port.txt
index ac56f20..7a22efc 100644
--- a/Documentation/cxl/cxl-disable-port.txt
+++ b/Documentation/cxl/cxl-disable-port.txt
@@ -30,10 +30,7 @@ OPTIONS
 	firmware and disabling an active device is akin to force removing memory
 	from a running system.
 
---debug::
-	If the cxl tool was built with debug disabled, turn on debug
-	messages.
-
+include::debug-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/cxl/cxl-enable-port.txt b/Documentation/cxl/cxl-enable-port.txt
index 9a37cef..50b53d1 100644
--- a/Documentation/cxl/cxl-enable-port.txt
+++ b/Documentation/cxl/cxl-enable-port.txt
@@ -31,10 +31,7 @@ OPTIONS
 	memdev is only enabled after all CXL ports in its device topology
 	ancestry are enabled.
 
---debug::
-	If the cxl tool was built with debug enabled, turn on debug
-	messages.
-
+include::debug-option.txt[]
 
 include::../copyright.txt[]
 
diff --git a/Documentation/cxl/debug-option.txt b/Documentation/cxl/debug-option.txt
new file mode 100644
index 0000000..70b922f
--- /dev/null
+++ b/Documentation/cxl/debug-option.txt
@@ -0,0 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
+
+--debug::
+	Turn on additional debug messages including library debug.
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index d019dfc..423be90 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -22,6 +22,7 @@ filedeps = [
   '../copyright.txt',
   'memdev-option.txt',
   'labels-options.txt',
+  'debug-option.txt',
 ]
 
 cxl_manpages = [
-- 
2.37.1

[ndctl PATCH v2 03/10] libcxl: Introduce libcxl region and mapping objects

[Vishal Verma]

Add a cxl_region object to libcxl that represents a CXL region. CXL
regions are made up of one or more cxl_memdev 'targets'. The
relationship between a target and a region is conveyed with a
cxl_memdev_mapping object.

CXL regions are childeren of root decoders, and are organized as such.
Mapping objects are childeren of a CXL region.  Introduce the two
classes of objects themselves, and common accessors related to them.

Cc: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/lib/private.h  |  34 ++++
 cxl/lib/libcxl.c   | 431 +++++++++++++++++++++++++++++++++++++++++++--
 cxl/libcxl.h       |  41 +++++
 .clang-format      |   2 +
 cxl/lib/libcxl.sym |  20 +++
 5 files changed, 518 insertions(+), 10 deletions(-)

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 832a815..da49a6c 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -116,7 +116,41 @@ struct cxl_decoder {
 	bool accelmem_capable;
 	bool locked;
 	enum cxl_decoder_target_type target_type;
+	int regions_init;
 	struct list_head targets;
+	struct list_head regions;
+};
+
+enum cxl_decode_state {
+	CXL_DECODE_UNKNOWN = -1,
+	CXL_DECODE_RESET = 0,
+	CXL_DECODE_COMMIT,
+};
+
+struct cxl_region {
+	struct cxl_decoder *decoder;
+	struct list_node list;
+	int mappings_init;
+	struct cxl_ctx *ctx;
+	void *dev_buf;
+	size_t buf_len;
+	char *dev_path;
+	int id;
+	uuid_t uuid;
+	u64 start;
+	u64 size;
+	unsigned int interleave_ways;
+	unsigned int interleave_granularity;
+	enum cxl_decode_state decode_state;
+	struct kmod_module *module;
+	struct list_head mappings;
+};
+
+struct cxl_memdev_mapping {
+	struct cxl_region *region;
+	struct cxl_decoder *decoder;
+	unsigned int position;
+	struct list_node list;
 };
 
 enum cxl_cmd_query_status {
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 946cd4b..8dd804c 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -79,6 +79,31 @@ static void free_target(struct cxl_target *target, struct list_head *head)
 	free(target);
 }
 
+static void free_region(struct cxl_region *region)
+{
+	struct cxl_decoder *decoder = region->decoder;
+	struct cxl_memdev_mapping *mapping, *_m;
+
+	list_for_each_safe(&region->mappings, mapping, _m, list) {
+		list_del_from(&region->mappings, &mapping->list);
+		free(mapping);
+	}
+	list_del_from(&decoder->regions, &region->list);
+	kmod_module_unref(region->module);
+	free(region->dev_buf);
+	free(region->dev_path);
+	free(region);
+}
+
+static void free_regions(struct cxl_decoder *decoder)
+{
+	struct cxl_region *region, *_r;
+
+	list_for_each_safe(&decoder->regions, region, _r, list)
+		free_region(region);
+	decoder->regions_init = 0;
+}
+
 static void free_decoder(struct cxl_decoder *decoder, struct list_head *head)
 {
 	struct cxl_target *target, *_t;
@@ -87,6 +112,7 @@ static void free_decoder(struct cxl_decoder *decoder, struct list_head *head)
 		list_del_from(head, &decoder->list);
 	list_for_each_safe(&decoder->targets, target, _t, list)
 		free_target(target, &decoder->targets);
+	free_regions(decoder);
 	free(decoder->dev_buf);
 	free(decoder->dev_path);
 	free(decoder);
@@ -304,6 +330,400 @@ CXL_EXPORT void cxl_set_log_priority(struct cxl_ctx *ctx, int priority)
 	ctx->ctx.log_priority = priority;
 }
 
+static int is_enabled(const char *drvpath)
+{
+	struct stat st;
+
+	if (lstat(drvpath, &st) < 0 || !S_ISLNK(st.st_mode))
+		return 0;
+	else
+		return 1;
+}
+
+CXL_EXPORT int cxl_region_is_enabled(struct cxl_region *region)
+{
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	char *path = region->dev_buf;
+	int len = region->buf_len;
+
+	if (snprintf(path, len, "%s/driver", region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", cxl_region_get_devname(region));
+		return 0;
+	}
+
+	return is_enabled(path);
+}
+
+CXL_EXPORT int cxl_region_disable(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+
+	util_unbind(region->dev_path, ctx);
+
+	if (cxl_region_is_enabled(region)) {
+		err(ctx, "%s: failed to disable\n", devname);
+		return -EBUSY;
+	}
+
+	dbg(ctx, "%s: disabled\n", devname);
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_enable(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	char *path = region->dev_buf;
+	int len = region->buf_len;
+	char buf[SYSFS_ATTR_SIZE];
+	u64 resource = ULLONG_MAX;
+
+	if (cxl_region_is_enabled(region))
+		return 0;
+
+	util_bind(devname, region->module, "cxl", ctx);
+
+	if (!cxl_region_is_enabled(region)) {
+		err(ctx, "%s: failed to enable\n", devname);
+		return -ENXIO;
+	}
+
+	/*
+	 * Currently 'resource' is the only attr that may change after enabling.
+	 * Just refresh it here. If there are additional resources that need
+	 * to be refreshed here later, split these out into a common helper
+	 * for this and add_cxl_region()
+	 */
+	if (snprintf(path, len, "%s/resource", region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return 0;
+	}
+
+	if (sysfs_read_attr(ctx, path, buf) == 0)
+		resource = strtoull(buf, NULL, 0);
+
+	if (resource < ULLONG_MAX)
+		region->start = resource;
+
+	dbg(ctx, "%s: enabled\n", devname);
+
+	return 0;
+}
+
+static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
+{
+	const char *devname = devpath_to_devname(cxlregion_base);
+	char *path = calloc(1, strlen(cxlregion_base) + 100);
+	struct cxl_region *region, *region_dup;
+	struct cxl_decoder *decoder = parent;
+	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
+	char buf[SYSFS_ATTR_SIZE];
+	u64 resource = ULLONG_MAX;
+
+	dbg(ctx, "%s: base: \'%s\'\n", devname, cxlregion_base);
+
+	if (!path)
+		return NULL;
+
+	region = calloc(1, sizeof(*region));
+	if (!region)
+		goto err;
+
+	region->id = id;
+	region->ctx = ctx;
+	region->decoder = decoder;
+	list_head_init(&region->mappings);
+
+	region->dev_path = strdup(cxlregion_base);
+	if (!region->dev_path)
+		goto err;
+
+	region->dev_buf = calloc(1, strlen(cxlregion_base) + 50);
+	if (!region->dev_buf)
+		goto err;
+	region->buf_len = strlen(cxlregion_base) + 50;
+
+	sprintf(path, "%s/size", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		region->size = ULLONG_MAX;
+	else
+		region->size = strtoull(buf, NULL, 0);
+
+	sprintf(path, "%s/resource", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) == 0)
+		resource = strtoull(buf, NULL, 0);
+
+	if (resource < ULLONG_MAX)
+		region->start = resource;
+
+	sprintf(path, "%s/uuid", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		goto err;
+	if (strlen(buf) && uuid_parse(buf, region->uuid) < 0) {
+		dbg(ctx, "%s:%s\n", path, buf);
+		goto err;
+	}
+
+	sprintf(path, "%s/interleave_granularity", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		region->interleave_granularity = UINT_MAX;
+	else
+		region->interleave_granularity = strtoul(buf, NULL, 0);
+
+	sprintf(path, "%s/interleave_ways", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		region->interleave_ways = UINT_MAX;
+	else
+		region->interleave_ways = strtoul(buf, NULL, 0);
+
+	sprintf(path, "%s/commit", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		region->decode_state = CXL_DECODE_UNKNOWN;
+	else
+		region->decode_state = strtoul(buf, NULL, 0);
+
+	sprintf(path, "%s/modalias", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) == 0)
+		region->module = util_modalias_to_module(ctx, buf);
+
+	cxl_region_foreach(decoder, region_dup)
+		if (region_dup->id == region->id) {
+			free_region(region);
+			return region_dup;
+		}
+
+	list_add(&decoder->regions, &region->list);
+
+	return region;
+err:
+	free(region->dev_path);
+	free(region->dev_buf);
+	free(region);
+	free(path);
+	return NULL;
+}
+
+static void cxl_regions_init(struct cxl_decoder *decoder)
+{
+	struct cxl_port *port = cxl_decoder_get_port(decoder);
+	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
+
+	if (decoder->regions_init)
+		return;
+
+	/* Only root port decoders may have child regions */
+	if (!cxl_port_is_root(port))
+		return;
+
+	decoder->regions_init = 1;
+
+	sysfs_device_parse(ctx, decoder->dev_path, "region", decoder,
+			   add_cxl_region);
+}
+
+CXL_EXPORT struct cxl_region *cxl_region_get_first(struct cxl_decoder *decoder)
+{
+	cxl_regions_init(decoder);
+
+	return list_top(&decoder->regions, struct cxl_region, list);
+}
+
+CXL_EXPORT struct cxl_region *cxl_region_get_next(struct cxl_region *region)
+{
+	struct cxl_decoder *decoder = region->decoder;
+
+	return list_next(&decoder->regions, region, list);
+}
+
+CXL_EXPORT struct cxl_ctx *cxl_region_get_ctx(struct cxl_region *region)
+{
+	return region->ctx;
+}
+
+CXL_EXPORT struct cxl_decoder *cxl_region_get_decoder(struct cxl_region *region)
+{
+	return region->decoder;
+}
+
+CXL_EXPORT int cxl_region_get_id(struct cxl_region *region)
+{
+	return region->id;
+}
+
+CXL_EXPORT const char *cxl_region_get_devname(struct cxl_region *region)
+{
+	return devpath_to_devname(region->dev_path);
+}
+
+CXL_EXPORT void cxl_region_get_uuid(struct cxl_region *region, uuid_t uu)
+{
+	memcpy(uu, region->uuid, sizeof(uuid_t));
+}
+
+CXL_EXPORT unsigned long long cxl_region_get_size(struct cxl_region *region)
+{
+	return region->size;
+}
+
+CXL_EXPORT unsigned long long cxl_region_get_resource(struct cxl_region *region)
+{
+	return region->start;
+}
+
+CXL_EXPORT unsigned int
+cxl_region_get_interleave_ways(struct cxl_region *region)
+{
+	return region->interleave_ways;
+}
+
+CXL_EXPORT int cxl_region_decode_is_committed(struct cxl_region *region)
+{
+	return (region->decode_state == CXL_DECODE_COMMIT) ? 1 : 0;
+}
+
+CXL_EXPORT unsigned int
+cxl_region_get_interleave_granularity(struct cxl_region *region)
+{
+	return region->interleave_granularity;
+}
+
+static struct cxl_decoder *__cxl_port_match_decoder(struct cxl_port *port,
+						    const char *ident)
+{
+	struct cxl_decoder *decoder;
+
+	cxl_decoder_foreach(port, decoder)
+		if (strcmp(cxl_decoder_get_devname(decoder), ident) == 0)
+			return decoder;
+
+	return NULL;
+}
+
+static struct cxl_decoder *cxl_port_find_decoder(struct cxl_port *port,
+						 const char *ident)
+{
+	struct cxl_decoder *decoder;
+	struct cxl_endpoint *ep;
+
+	/* First, check decoders directly under @port */
+	decoder = __cxl_port_match_decoder(port, ident);
+	if (decoder)
+		return decoder;
+
+	/* Next, iterate over the endpoints under @port */
+	cxl_endpoint_foreach(port, ep) {
+		decoder = __cxl_port_match_decoder(cxl_endpoint_get_port(ep),
+						   ident);
+		if (decoder)
+			return decoder;
+	}
+
+	return NULL;
+}
+
+static struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
+						   const char *ident)
+{
+	struct cxl_bus *bus;
+
+	cxl_bus_foreach(ctx, bus) {
+		struct cxl_decoder *decoder;
+		struct cxl_port *port, *top;
+
+		port = cxl_bus_get_port(bus);
+		decoder = cxl_port_find_decoder(port, ident);
+		if (decoder)
+			return decoder;
+
+		top = port;
+		cxl_port_foreach_all (top, port) {
+			decoder = cxl_port_find_decoder(port, ident);
+			if (decoder)
+				return decoder;
+		}
+	}
+
+	return NULL;
+}
+
+static void cxl_mappings_init(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	char *mapping_path, buf[SYSFS_ATTR_SIZE];
+	unsigned int i;
+
+	if (region->mappings_init)
+		return;
+	region->mappings_init = 1;
+
+	mapping_path = calloc(1, strlen(region->dev_path) + 100);
+	if (!mapping_path) {
+		err(ctx, "%s: allocation failure\n", devname);
+		return;
+	}
+
+	for (i = 0; i < region->interleave_ways; i++) {
+		struct cxl_memdev_mapping *mapping;
+		struct cxl_decoder *decoder;
+
+		sprintf(mapping_path, "%s/target%d", region->dev_path, i);
+		if (sysfs_read_attr(ctx, mapping_path, buf) < 0) {
+			err(ctx, "%s: failed to read target%d\n", devname, i);
+			continue;
+		}
+
+		decoder = cxl_decoder_get_by_name(ctx, buf);
+		if (!decoder) {
+			err(ctx, "%s target%d: %s lookup failure\n",
+			    devname, i, buf);
+			continue;
+		}
+
+		mapping = calloc(1, sizeof(*mapping));
+		if (!mapping) {
+			err(ctx, "%s target%d: allocation failure\n", devname, i);
+			continue;
+		}
+
+		mapping->region = region;
+		mapping->decoder = decoder;
+		mapping->position = i;
+		list_add(&region->mappings, &mapping->list);
+	}
+	free(mapping_path);
+}
+
+CXL_EXPORT struct cxl_memdev_mapping *
+cxl_mapping_get_first(struct cxl_region *region)
+{
+	cxl_mappings_init(region);
+
+	return list_top(&region->mappings, struct cxl_memdev_mapping, list);
+}
+
+CXL_EXPORT struct cxl_memdev_mapping *
+cxl_mapping_get_next(struct cxl_memdev_mapping *mapping)
+{
+	struct cxl_region *region = mapping->region;
+
+	return list_next(&region->mappings, mapping, list);
+}
+
+CXL_EXPORT struct cxl_decoder *
+cxl_mapping_get_decoder(struct cxl_memdev_mapping *mapping)
+{
+	return mapping->decoder;
+}
+
+CXL_EXPORT unsigned int
+cxl_mapping_get_position(struct cxl_memdev_mapping *mapping)
+{
+	return mapping->position;
+}
+
 static void *add_cxl_pmem(void *parent, int id, const char *br_base)
 {
 	const char *devname = devpath_to_devname(br_base);
@@ -681,16 +1101,6 @@ CXL_EXPORT size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev)
 	return memdev->lsa_size;
 }
 
-static int is_enabled(const char *drvpath)
-{
-	struct stat st;
-
-	if (lstat(drvpath, &st) < 0 || !S_ISLNK(st.st_mode))
-		return 0;
-	else
-		return 1;
-}
-
 CXL_EXPORT int cxl_memdev_is_enabled(struct cxl_memdev *memdev)
 {
 	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
@@ -939,6 +1349,7 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
 	decoder->ctx = ctx;
 	decoder->port = port;
 	list_head_init(&decoder->targets);
+	list_head_init(&decoder->regions);
 
 	decoder->dev_path = strdup(cxldecoder_base);
 	if (!decoder->dev_path)
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 33a216e..c77a7b9 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -224,6 +224,47 @@ int cxl_memdev_is_enabled(struct cxl_memdev *memdev);
 	for (endpoint = cxl_endpoint_get_first(port); endpoint != NULL;        \
 	     endpoint = cxl_endpoint_get_next(endpoint))
 
+struct cxl_region;
+struct cxl_region *cxl_region_get_first(struct cxl_decoder *decoder);
+struct cxl_region *cxl_region_get_next(struct cxl_region *region);
+int cxl_region_decode_is_committed(struct cxl_region *region);
+int cxl_region_is_enabled(struct cxl_region *region);
+int cxl_region_disable(struct cxl_region *region);
+int cxl_region_enable(struct cxl_region *region);
+struct cxl_ctx *cxl_region_get_ctx(struct cxl_region *region);
+struct cxl_decoder *cxl_region_get_decoder(struct cxl_region *region);
+int cxl_region_get_id(struct cxl_region *region);
+const char *cxl_region_get_devname(struct cxl_region *region);
+void cxl_region_get_uuid(struct cxl_region *region, uuid_t uu);
+unsigned long long cxl_region_get_size(struct cxl_region *region);
+unsigned long long cxl_region_get_resource(struct cxl_region *region);
+unsigned int cxl_region_get_interleave_ways(struct cxl_region *region);
+unsigned int cxl_region_get_interleave_granularity(struct cxl_region *region);
+
+#define cxl_region_foreach(decoder, region)                                    \
+	for (region = cxl_region_get_first(decoder); region != NULL;           \
+	     region = cxl_region_get_next(region))
+
+#define cxl_region_foreach_safe(decoder, region, _region)                      \
+	for (region = cxl_region_get_first(decoder),                           \
+	     _region = region ? cxl_region_get_next(region) : NULL;            \
+	     region != NULL;                                                   \
+	     region = _region,                                                 \
+	     _region = _region ? cxl_region_get_next(_region) : NULL)
+
+struct cxl_memdev_mapping;
+struct cxl_memdev_mapping *cxl_mapping_get_first(struct cxl_region *region);
+struct cxl_memdev_mapping *
+cxl_mapping_get_next(struct cxl_memdev_mapping *mapping);
+struct cxl_decoder *cxl_mapping_get_decoder(struct cxl_memdev_mapping *mapping);
+struct cxl_region *cxl_mapping_get_region(struct cxl_memdev_mapping *mapping);
+unsigned int cxl_mapping_get_position(struct cxl_memdev_mapping *mapping);
+
+#define cxl_mapping_foreach(region, mapping) \
+        for (mapping = cxl_mapping_get_first(region); \
+             mapping != NULL; \
+             mapping = cxl_mapping_get_next(mapping))
+
 struct cxl_cmd;
 const char *cxl_cmd_get_devname(struct cxl_cmd *cmd);
 struct cxl_cmd *cxl_cmd_new_raw(struct cxl_memdev *memdev, int opcode);
diff --git a/.clang-format b/.clang-format
index 7254a1b..b6169e1 100644
--- a/.clang-format
+++ b/.clang-format
@@ -86,6 +86,8 @@ ForEachMacros:
   - 'cxl_dport_foreach'
   - 'cxl_endpoint_foreach'
   - 'cxl_port_foreach_all'
+  - 'cxl_region_foreach'
+  - 'cxl_region_foreach_safe'
   - 'daxctl_dev_foreach'
   - 'daxctl_mapping_foreach'
   - 'daxctl_region_foreach'
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 7712de0..e410298 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -177,4 +177,24 @@ global:
 	cxl_decoder_get_prev;
 	cxl_decoder_set_dpa_size;
 	cxl_decoder_set_mode;
+	cxl_region_get_first;
+	cxl_region_get_next;
+	cxl_region_decode_is_committed;
+	cxl_region_is_enabled;
+	cxl_region_disable;
+	cxl_region_enable;
+	cxl_region_get_ctx;
+	cxl_region_get_decoder;
+	cxl_region_get_id;
+	cxl_region_get_devname;
+	cxl_region_get_uuid;
+	cxl_region_get_size;
+	cxl_region_get_resource;
+	cxl_region_get_interleave_ways;
+	cxl_region_get_interleave_granularity;
+	cxl_mapping_get_first;
+	cxl_mapping_get_next;
+	cxl_mapping_get_decoder;
+	cxl_mapping_get_region;
+	cxl_mapping_get_position;
 } LIBCXL_2;
-- 
2.37.1

[ndctl PATCH v2 04/10] cxl-cli: add region listing support

[Vishal Verma]

Add cxl_region -> json and cxl_mapping -> json emitter helpers, and
teach cxl_filter_walk about cxl_regions. With these in place, 'cxl-list'
can now emit json objects for CXL regions. They can be top-level objects
if requested by themselves, or nested under root-decoders, if listed
along with decoders. Allow a plain 'cxl list' command to imply
'--regions'.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-list.txt |  13 ++-
 cxl/filter.h                   |   4 +
 cxl/json.h                     |   5 ++
 cxl/filter.c                   | 158 +++++++++++++++++++++++++++++++--
 cxl/json.c                     | 123 +++++++++++++++++++++++++
 cxl/list.c                     |  25 +++---
 6 files changed, 304 insertions(+), 24 deletions(-)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index f6aba0c..2906c2f 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -309,8 +309,9 @@ OPTIONS
 
 -T::
 --targets::
-	Extend decoder listings with downstream port target information, and /
-	or port and bus listings with the downstream port information.
+	Extend decoder listings with downstream port target information, port
+	and bus listings with the downstream port information, and / or regions
+	with mapping information.
 ----
 # cxl list -BTu -b ACPI.CXL
 {
@@ -327,6 +328,14 @@ OPTIONS
 }
 ----
 
+-R::
+--regions::
+	Include region objects in the listing.
+
+-r::
+--region::
+	Specify the region name to filter the emitted regions.
+
 --debug::
 	If the cxl tool was built with debug enabled, turn on debug
 	messages.
diff --git a/cxl/filter.h b/cxl/filter.h
index c913daf..609433c 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -13,9 +13,11 @@ struct cxl_filter_params {
 	const char *port_filter;
 	const char *endpoint_filter;
 	const char *decoder_filter;
+	const char *region_filter;
 	bool single;
 	bool endpoints;
 	bool decoders;
+	bool regions;
 	bool targets;
 	bool memdevs;
 	bool ports;
@@ -33,6 +35,8 @@ struct cxl_memdev *util_cxl_memdev_filter(struct cxl_memdev *memdev,
 struct cxl_port *util_cxl_port_filter_by_memdev(struct cxl_port *port,
 						const char *ident,
 						const char *serial);
+struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
+					    const char *__ident);
 
 enum cxl_port_filter_mode {
 	CXL_PF_SINGLE,
diff --git a/cxl/json.h b/cxl/json.h
index 9a5a845..eb7572b 100644
--- a/cxl/json.h
+++ b/cxl/json.h
@@ -15,6 +15,11 @@ struct json_object *util_cxl_endpoint_to_json(struct cxl_endpoint *endpoint,
 					      unsigned long flags);
 struct json_object *util_cxl_decoder_to_json(struct cxl_decoder *decoder,
 					     unsigned long flags);
+struct json_object *util_cxl_region_to_json(struct cxl_region *region,
+					     unsigned long flags);
+void util_cxl_mappings_append_json(struct json_object *jregion,
+				  struct cxl_region *region,
+				  unsigned long flags);
 void util_cxl_targets_append_json(struct json_object *jdecoder,
 				  struct cxl_decoder *decoder,
 				  const char *ident, const char *serial,
diff --git a/cxl/filter.c b/cxl/filter.c
index e5fab19..38ece55 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -585,6 +585,73 @@ util_cxl_memdev_filter_by_port(struct cxl_memdev *memdev, const char *bus_ident,
 	return NULL;
 }
 
+static struct cxl_region *
+util_cxl_region_filter_by_bus(struct cxl_region *region, const char *__ident)
+{
+	struct cxl_decoder *decoder = cxl_region_get_decoder(region);
+
+	if (!util_cxl_decoder_filter_by_bus(decoder, __ident))
+		return NULL;
+	return region;
+}
+
+static struct cxl_region *
+util_cxl_region_filter_by_port(struct cxl_region *region, const char *__ident)
+{
+	struct cxl_decoder *decoder = cxl_region_get_decoder(region);
+	struct cxl_port *port = cxl_decoder_get_port(decoder);
+
+	if (!util_cxl_port_filter(port, __ident ,CXL_PF_ANCESTRY))
+		return NULL;
+	return region;
+}
+
+static struct cxl_region *
+util_cxl_region_filter_by_decoder(struct cxl_region *region,
+				  const char *__ident)
+{
+	struct cxl_decoder *decoder = cxl_region_get_decoder(region);
+
+	if (!util_cxl_decoder_filter(decoder, __ident))
+		return NULL;
+	return region;
+}
+
+struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
+					    const char *__ident)
+{
+	char *ident, *save;
+	const char *name;
+	int id;
+
+	if (!__ident)
+		return region;
+
+	ident = strdup(__ident);
+	if (!ident)
+		return NULL;
+
+	for (name = strtok_r(ident, which_sep(__ident), &save); name;
+	     name = strtok_r(NULL, which_sep(__ident), &save)) {
+		if (strcmp(name, "all") == 0)
+			break;
+
+		if ((sscanf(name, "%d", &id) == 1 ||
+		     sscanf(name, "region%d", &id) == 1) &&
+		    cxl_region_get_id(region) == id)
+			break;
+
+		if (strcmp(name, cxl_region_get_devname(region)) == 0)
+			break;
+	}
+
+	free(ident);
+	if (name)
+		return region;
+	return NULL;
+
+}
+
 static unsigned long params_to_flags(struct cxl_filter_params *param)
 {
 	unsigned long flags = 0;
@@ -672,26 +739,57 @@ static struct json_object *pick_array(struct json_object *child,
 	return NULL;
 }
 
+static void walk_regions(struct cxl_decoder *decoder,
+			 struct json_object *jregions,
+			 struct cxl_filter_params *p,
+			 unsigned long flags)
+{
+	struct json_object *jregion;
+	struct cxl_region *region;
+
+	cxl_region_foreach(decoder, region) {
+		if (!util_cxl_region_filter(region, p->region_filter))
+			continue;
+		if (!util_cxl_region_filter_by_bus(region, p->bus_filter))
+			continue;
+		if (!util_cxl_region_filter_by_port(region, p->port_filter))
+			continue;
+		if (!util_cxl_region_filter_by_decoder(region, p->decoder_filter))
+			continue;
+		if (!p->idle && !cxl_region_is_enabled(region))
+			continue;
+		jregion = util_cxl_region_to_json(region, flags);
+		if (!jregion)
+			continue;
+		json_object_array_add(jregions, jregion);
+	}
+
+	return;
+}
+
 static void walk_decoders(struct cxl_port *port, struct cxl_filter_params *p,
-			  struct json_object *jdecoders, unsigned long flags)
+			  struct json_object *jdecoders,
+			  struct json_object *jregions, unsigned long flags)
 {
 	struct cxl_decoder *decoder;
 
 	cxl_decoder_foreach(port, decoder) {
+		const char *devname = cxl_decoder_get_devname(decoder);
+		struct json_object *jchildregions = NULL;
 		struct json_object *jdecoder;
 
 		if (!p->decoders)
-			continue;
+			goto walk_children;
 		if (!util_cxl_decoder_filter(decoder, p->decoder_filter))
-			continue;
+			goto walk_children;
 		if (!util_cxl_decoder_filter_by_bus(decoder, p->bus_filter))
-			continue;
+			goto walk_children;
 		if (!util_cxl_decoder_filter_by_port(decoder, p->port_filter,
 						     pf_mode(p)))
-			continue;
+			goto walk_children;
 		if (!util_cxl_decoder_filter_by_memdev(
 			    decoder, p->memdev_filter, p->serial_filter))
-			continue;
+			goto walk_children;
 		if (!p->idle && cxl_decoder_get_size(decoder) == 0)
 			continue;
 		jdecoder = util_cxl_decoder_to_json(decoder, flags);
@@ -702,7 +800,27 @@ static void walk_decoders(struct cxl_port *port, struct cxl_filter_params *p,
 		util_cxl_targets_append_json(jdecoder, decoder,
 					     p->memdev_filter, p->serial_filter,
 					     flags);
+
+		if (p->regions) {
+			jchildregions = json_object_new_array();
+			if (!jchildregions) {
+				err(p, "failed to allocate region object\n");
+				return;
+			}
+		}
+
 		json_object_array_add(jdecoders, jdecoder);
+
+walk_children:
+		if (!p->regions)
+			continue;
+		if (!cxl_port_is_root(port))
+			continue;
+		walk_regions(decoder,
+			     pick_array(jchildregions, jregions),
+			     p, flags);
+		cond_add_put_array_suffix(jdecoder, "regions", devname,
+					  jchildregions);
 	}
 }
 
@@ -782,7 +900,7 @@ static void walk_endpoints(struct cxl_port *port, struct cxl_filter_params *p,
 		if (!p->decoders)
 			continue;
 		walk_decoders(cxl_endpoint_get_port(endpoint), p,
-			      pick_array(jchilddecoders, jdecoders), flags);
+			      pick_array(jchilddecoders, jdecoders), NULL, flags);
 		cond_add_put_array_suffix(jendpoint, "decoders", devname,
 					  jchilddecoders);
 	}
@@ -869,7 +987,8 @@ walk_children:
 				       flags);
 
 		walk_decoders(port, p,
-			      pick_array(jchilddecoders, jportdecoders), flags);
+			      pick_array(jchilddecoders, jportdecoders), NULL,
+			      flags);
 		walk_child_ports(port, p, pick_array(jchildports, jports),
 				 pick_array(jchilddecoders, jportdecoders),
 				 pick_array(jchildeps, jeps),
@@ -894,6 +1013,7 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 	struct json_object *jbusdecoders = NULL;
 	struct json_object *jepdecoders = NULL;
 	struct json_object *janondevs = NULL;
+	struct json_object *jregions = NULL;
 	struct json_object *jeps = NULL;
 	struct cxl_memdev *memdev;
 	int top_level_objs = 0;
@@ -936,6 +1056,10 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 	if (!jepdecoders)
 		goto err;
 
+	jregions = json_object_new_array();
+	if (!jregions)
+		goto err;
+
 	dbg(p, "walk memdevs\n");
 	cxl_memdev_foreach(ctx, memdev) {
 		struct json_object *janondev;
@@ -964,6 +1088,7 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 		struct json_object *jchildports = NULL;
 		struct json_object *jchilddevs = NULL;
 		struct json_object *jchildeps = NULL;
+		struct json_object *jchildregions = NULL;
 		struct cxl_port *port = cxl_bus_get_port(bus);
 		const char *devname = cxl_bus_get_devname(bus);
 
@@ -1021,11 +1146,20 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 					continue;
 				}
 			}
+			if (p->regions && !p->decoders) {
+				jchildregions = json_object_new_array();
+				if (!jchildregions) {
+					err(p,
+					    "%s: failed to enumerate child regions\n",
+					    devname);
+					continue;
+				}
+			}
 		}
 walk_children:
 		dbg(p, "walk decoders\n");
 		walk_decoders(port, p, pick_array(jchilddecoders, jbusdecoders),
-			      flags);
+			      pick_array(jchildregions, jregions), flags);
 
 		dbg(p, "walk ports\n");
 		walk_child_ports(port, p, pick_array(jchildports, jports),
@@ -1038,6 +1172,8 @@ walk_children:
 					  jchildeps);
 		cond_add_put_array_suffix(jbus, "decoders", devname,
 					  jchilddecoders);
+		cond_add_put_array_suffix(jbus, "regions", devname,
+					  jchildregions);
 		cond_add_put_array_suffix(jbus, "memdevs", devname, jchilddevs);
 	}
 
@@ -1057,6 +1193,8 @@ walk_children:
 		top_level_objs++;
 	if (json_object_array_length(jepdecoders))
 		top_level_objs++;
+	if (json_object_array_length(jregions))
+		top_level_objs++;
 
 	splice_array(p, janondevs, jplatform, "anon memdevs", top_level_objs > 1);
 	splice_array(p, jbuses, jplatform, "buses", top_level_objs > 1);
@@ -1069,6 +1207,7 @@ walk_children:
 		     top_level_objs > 1);
 	splice_array(p, jepdecoders, jplatform, "endpoint decoders",
 		     top_level_objs > 1);
+	splice_array(p, jregions, jplatform, "regions", top_level_objs > 1);
 
 	util_display_json_array(stdout, jplatform, flags);
 
@@ -1082,6 +1221,7 @@ err:
 	json_object_put(jbusdecoders);
 	json_object_put(jportdecoders);
 	json_object_put(jepdecoders);
+	json_object_put(jregions);
 	json_object_put(jplatform);
 	return -ENOMEM;
 }
diff --git a/cxl/json.c b/cxl/json.c
index ae9c812..ad93413 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -524,6 +524,129 @@ struct json_object *util_cxl_decoder_to_json(struct cxl_decoder *decoder,
 	return jdecoder;
 }
 
+void util_cxl_mappings_append_json(struct json_object *jregion,
+				  struct cxl_region *region,
+				  unsigned long flags)
+{
+	struct json_object *jobj, *jmappings;
+	struct cxl_memdev_mapping *mapping;
+	unsigned int val, nr_mappings;
+	const char *devname;
+
+	nr_mappings = cxl_region_get_interleave_ways(region);
+	if (!nr_mappings || (nr_mappings == UINT_MAX))
+		return;
+
+	if (!(flags & UTIL_JSON_TARGETS))
+		return;
+
+	jmappings = json_object_new_array();
+	if (!jmappings)
+		return;
+
+	cxl_mapping_foreach(region, mapping) {
+		struct json_object *jmapping;
+		struct cxl_decoder *decoder;
+		struct cxl_memdev *memdev;
+
+		jmapping = json_object_new_object();
+		if (!jmapping)
+			continue;
+
+		val = cxl_mapping_get_position(mapping);
+		if (val < UINT_MAX) {
+			jobj = json_object_new_int(val);
+			if (jobj)
+				json_object_object_add(jmapping, "position",
+						       jobj);
+		}
+
+		decoder = cxl_mapping_get_decoder(mapping);
+		if (!decoder)
+			continue;
+
+		memdev = cxl_ep_decoder_get_memdev(decoder);
+		if (memdev) {
+			devname = cxl_memdev_get_devname(memdev);
+			jobj = json_object_new_string(devname);
+			if (jobj)
+				json_object_object_add(jmapping, "memdev", jobj);
+		}
+
+		devname = cxl_decoder_get_devname(decoder);
+		jobj = json_object_new_string(devname);
+		if (jobj)
+			json_object_object_add(jmapping, "decoder", jobj);
+
+		json_object_array_add(jmappings, jmapping);
+	}
+
+	json_object_object_add(jregion, "mappings", jmappings);
+}
+
+struct json_object *util_cxl_region_to_json(struct cxl_region *region,
+					     unsigned long flags)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct json_object *jregion, *jobj;
+	u64 val;
+
+	jregion = json_object_new_object();
+	if (!jregion)
+		return NULL;
+
+	jobj = json_object_new_string(devname);
+	if (jobj)
+		json_object_object_add(jregion, "region", jobj);
+
+	val = cxl_region_get_resource(region);
+	if (val < ULLONG_MAX) {
+		jobj = util_json_object_hex(val, flags);
+		if (jobj)
+			json_object_object_add(jregion, "resource", jobj);
+	}
+
+	val = cxl_region_get_size(region);
+	if (val < ULLONG_MAX) {
+		jobj = util_json_object_size(val, flags);
+		if (jobj)
+			json_object_object_add(jregion, "size", jobj);
+	}
+
+	val = cxl_region_get_interleave_ways(region);
+	if (val < INT_MAX) {
+		jobj = json_object_new_int(val);
+		if (jobj)
+			json_object_object_add(jregion,
+					       "interleave_ways", jobj);
+	}
+
+	val = cxl_region_get_interleave_granularity(region);
+	if (val < INT_MAX) {
+		jobj = json_object_new_int(val);
+		if (jobj)
+			json_object_object_add(jregion,
+					       "interleave_granularity", jobj);
+	}
+
+	if (cxl_region_decode_is_committed(region))
+		jobj = json_object_new_string("commit");
+	else
+		jobj = json_object_new_string("reset");
+	if (jobj)
+		json_object_object_add(jregion, "decode_state", jobj);
+
+	if (!cxl_region_is_enabled(region)) {
+		jobj = json_object_new_string("disabled");
+		if (jobj)
+			json_object_object_add(jregion, "state", jobj);
+	}
+
+	util_cxl_mappings_append_json(jregion, region, flags);
+
+	return jregion;
+}
+
 void util_cxl_targets_append_json(struct json_object *jdecoder,
 				  struct cxl_decoder *decoder,
 				  const char *ident, const char *serial,
diff --git a/cxl/list.c b/cxl/list.c
index 1b5f583..88ca9d9 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -41,7 +41,10 @@ static const struct option options[] = {
 	OPT_BOOLEAN('D', "decoders", &param.decoders,
 		    "include CXL decoder info"),
 	OPT_BOOLEAN('T', "targets", &param.targets,
-		    "include CXL target data with decoders or ports"),
+		    "include CXL target data with decoders, ports, or regions"),
+	OPT_STRING('r', "region", &param.region_filter, "region name",
+		   "filter by CXL region name(s)"),
+	OPT_BOOLEAN('R', "regions", &param.regions, "include CXL regions"),
 	OPT_BOOLEAN('i', "idle", &param.idle, "include disabled devices"),
 	OPT_BOOLEAN('u', "human", &param.human,
 		    "use human friendly number formats"),
@@ -58,7 +61,7 @@ static const struct option options[] = {
 static int num_list_flags(void)
 {
 	return !!param.memdevs + !!param.buses + !!param.ports +
-	       !!param.endpoints + !!param.decoders;
+	       !!param.endpoints + !!param.decoders + !!param.regions;
 }
 
 int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
@@ -92,18 +95,14 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 			param.endpoints = true;
 		if (param.decoder_filter)
 			param.decoders = true;
-		if (num_list_flags() == 0) {
-			/*
-			 * TODO: We likely want to list regions by default if
-			 * nothing was explicitly asked for. But until we have
-			 * region support, print this error asking for devices
-			 * explicitly.  Once region support is added, this TODO
-			 * can be removed.
-			 */
-			error("please specify entities to list, e.g. using -m/-M\n");
-			usage_with_options(u, options);
-		}
 		param.single = true;
+		if (param.region_filter)
+			param.regions = true;
+	}
+
+	/* List regions by default */
+	if (num_list_flags() == 0) {
+		param.regions = true;
 	}
 
 	log_init(&param.ctx, "cxl list", "CXL_LIST_LOG");
-- 
2.37.1

[ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

[Vishal Verma]

Add libcxl APIs to create a region under a given root decoder, and to
set different attributes for the new region. These allow setting the
size, interleave_ways, interleave_granularity, uuid, and the target
devices for the newly minted cxl_region object.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/lib/libcxl.txt |  69 ++++++
 cxl/lib/private.h                |   2 +
 cxl/lib/libcxl.c                 | 377 ++++++++++++++++++++++++++++++-
 cxl/libcxl.h                     |  23 +-
 cxl/lib/libcxl.sym               |  16 ++
 5 files changed, 484 insertions(+), 3 deletions(-)

diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index 7a38ce4..c3a8f36 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -508,6 +508,75 @@ device to represent the root of a PCI device hierarchy. The
 cxl_target_get_physical_node() helper returns the device name of that
 companion object in the PCI hierarchy.
 
+==== REGIONS
+A CXL region is composed of one or more slices of CXL memdevs, with configurable
+interleave settings - both the number of interleave ways, and the interleave
+granularity. In terms of hierarchy, it is the child of a CXL root decoder. A root
+decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), may have
+multiple chile regions, but a region is strictly tied to one root decoder.
+
+A region also defines a set of mappings which are slices of capacity on a memdev,
+each represented by an endpoint decoder.
+
+===== REGION: Enumeration
+----
+struct cxl_region *cxl_region_get_first(struct cxl_decoder *decoder);
+struct cxl_region *cxl_region_get_next(struct cxl_region *region);
+
+#define cxl_region_foreach(decoder, region)                                    \
+	for (region = cxl_region_get_first(decoder); region != NULL;           \
+	     region = cxl_region_get_next(region))
+
+#define cxl_region_foreach_safe(decoder, region, _region)                      \
+	for (region = cxl_region_get_first(decoder),                           \
+	     _region = region ? cxl_region_get_next(region) : NULL;            \
+	     region != NULL;                                                   \
+	     region = _region,                                                 \
+	     _region = _region ? cxl_region_get_next(_region) : NULL)
+----
+
+===== REGION: Attributes
+----
+int cxl_region_get_id(struct cxl_region *region);
+const char *cxl_region_get_devname(struct cxl_region *region);
+void cxl_region_get_uuid(struct cxl_region *region, uuid_t uu);
+unsigned long long cxl_region_get_size(struct cxl_region *region);
+unsigned long long cxl_region_get_resource(struct cxl_region *region);
+unsigned int cxl_region_get_interleave_ways(struct cxl_region *region);
+unsigned int cxl_region_get_interleave_granularity(struct cxl_region *region);
+struct cxl_decoder *cxl_region_get_target_decoder(struct cxl_region *region,
+						  int position);
+int cxl_region_set_size(struct cxl_region *region, unsigned long long size);
+int cxl_region_set_uuid(struct cxl_region *region, uuid_t uu);
+int cxl_region_set_interleave_ways(struct cxl_region *region,
+				   unsigned int ways);
+int cxl_region_set_interleave_granularity(struct cxl_region *region,
+					  unsigned int granularity);
+int cxl_region_set_target(struct cxl_region *region, int position,
+			  struct cxl_decoder *decoder);
+int cxl_region_clear_target(struct cxl_region *region, int position);
+int cxl_region_clear_all_targets(struct cxl_region *region);
+int cxl_region_decode_commit(struct cxl_region *region);
+int cxl_region_decode_reset(struct cxl_region *region);
+----
+
+A region's resource attribute is the Host Physical Address at which the region's
+address space starts. The region's address space is a subset of the parent root
+decoder's address space.
+
+The interleave ways is the number of component memdevs participating in the
+region.
+
+The interleave granularity depends on the root decoder's granularity, and must
+follow the interleave math rules defined in the CXL spec.
+
+Regions have a list of targets 0..N, which are programmed with the name of an
+endpoint decoder under each participating memdev.
+
+The 'decode_commit' and 'decode_reset' attributes reserve and free DPA space
+on a given memdev by allocating an endpoint decoder, and programming it based
+on the region's interleave geometry.
+
 include::../../copyright.txt[]
 
 SEE ALSO
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index da49a6c..8619bb1 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -110,6 +110,8 @@ struct cxl_decoder {
 	int nr_targets;
 	int id;
 	enum cxl_decoder_mode mode;
+	unsigned int interleave_ways;
+	unsigned int interleave_granularity;
 	bool pmem_capable;
 	bool volatile_capable;
 	bool mem_capable;
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 8dd804c..b4d7890 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -17,6 +17,7 @@
 #include <ccan/minmax/minmax.h>
 #include <ccan/array_size/array_size.h>
 #include <ccan/short_types/short_types.h>
+#include <ccan/container_of/container_of.h>
 
 #include <util/log.h>
 #include <util/list.h>
@@ -412,6 +413,39 @@ CXL_EXPORT int cxl_region_enable(struct cxl_region *region)
 	return 0;
 }
 
+static int cxl_region_delete_name(struct cxl_decoder *decoder,
+				  const char *devname)
+{
+	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
+	char *path = decoder->dev_buf;
+	int rc;
+
+	sprintf(path, "%s/delete_region", decoder->dev_path);
+	rc = sysfs_write_attr(ctx, path, devname);
+	if (rc != 0) {
+		err(ctx, "error deleting region: %s\n", strerror(-rc));
+		return rc;
+	}
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
+{
+	struct cxl_decoder *decoder = cxl_region_get_decoder(region);
+	const char *devname = cxl_region_get_devname(region);
+	int rc;
+
+	if (cxl_region_is_enabled(region))
+		return -EBUSY;
+
+	rc = cxl_region_delete_name(decoder, devname);
+	if (rc != 0)
+		return rc;
+
+	free_region(region);
+	return 0;
+}
+
 static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 {
 	const char *devname = devpath_to_devname(cxlregion_base);
@@ -589,6 +623,258 @@ cxl_region_get_interleave_granularity(struct cxl_region *region)
 	return region->interleave_granularity;
 }
 
+CXL_EXPORT struct cxl_decoder *
+cxl_region_get_target_decoder(struct cxl_region *region, int position)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	struct cxl_decoder *decoder;
+	char buf[SYSFS_ATTR_SIZE];
+
+	if (snprintf(path, len, "%s/target%d", region->dev_path, position) >=
+	    len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return NULL;
+	}
+
+	rc = sysfs_read_attr(ctx, path, buf);
+	if (rc < 0) {
+		err(ctx, "%s: error reading target%d: %s\n", devname,
+		    position, strerror(-rc));
+		return NULL;
+	}
+
+	decoder = cxl_decoder_get_by_name(ctx, buf);
+	if (!decoder) {
+		err(ctx, "%s: error locating decoder for target%d\n", devname,
+		    position);
+		return NULL;
+	}
+	return decoder;
+}
+
+CXL_EXPORT int cxl_region_set_size(struct cxl_region *region,
+				   unsigned long long size)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	char buf[SYSFS_ATTR_SIZE];
+
+	if (size == 0) {
+		dbg(ctx, "%s: cannot use %s to delete a region\n", __func__,
+		    devname);
+		return -EINVAL;
+	}
+
+	if (snprintf(path, len, "%s/size", region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	sprintf(buf, "%#llx\n", size);
+	rc = sysfs_write_attr(ctx, path, buf);
+	if (rc < 0)
+		return rc;
+
+	region->size = size;
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_set_uuid(struct cxl_region *region, uuid_t uu)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	char uuid[SYSFS_ATTR_SIZE];
+
+	if (snprintf(path, len, "%s/uuid", region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	uuid_unparse(uu, uuid);
+	rc = sysfs_write_attr(ctx, path, uuid);
+	if (rc != 0)
+		return rc;
+	memcpy(region->uuid, uu, sizeof(uuid_t));
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_set_interleave_ways(struct cxl_region *region,
+					      unsigned int ways)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	char buf[SYSFS_ATTR_SIZE];
+
+	if (snprintf(path, len, "%s/interleave_ways",
+		     region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	sprintf(buf, "%u\n", ways);
+	rc = sysfs_write_attr(ctx, path, buf);
+	if (rc < 0)
+		return rc;
+
+	region->interleave_ways = ways;
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_set_interleave_granularity(struct cxl_region *region,
+						     unsigned int granularity)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	char buf[SYSFS_ATTR_SIZE];
+
+	if (snprintf(path, len, "%s/interleave_granularity",
+		     region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	sprintf(buf, "%u\n", granularity);
+	rc = sysfs_write_attr(ctx, path, buf);
+	if (rc < 0)
+		return rc;
+
+	region->interleave_granularity = granularity;
+
+	return 0;
+}
+
+static int region_write_target(struct cxl_region *region, int position,
+			       struct cxl_decoder *decoder)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	const char *dec_name = "";
+
+	if (decoder)
+		dec_name = cxl_decoder_get_devname(decoder);
+
+	if (snprintf(path, len, "%s/target%d", region->dev_path, position) >=
+	    len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	rc = sysfs_write_attr(ctx, path, dec_name);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_set_target(struct cxl_region *region, int position,
+				     struct cxl_decoder *decoder)
+{
+	if (!decoder)
+		return -ENXIO;
+
+	return region_write_target(region, position, decoder);
+}
+
+CXL_EXPORT int cxl_region_clear_target(struct cxl_region *region, int position)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int rc;
+
+	if (cxl_region_is_enabled(region)) {
+		err(ctx, "%s: can't clear targets on an active region\n",
+		    devname);
+		return -EBUSY;
+	}
+
+	rc = region_write_target(region, position, NULL);
+	if (rc) {
+		err(ctx, "%s: error clearing target%d: %s\n",
+		    devname, position, strerror(-rc));
+		return rc;
+	}
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_clear_all_targets(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	unsigned int ways, i;
+	int rc;
+
+	if (cxl_region_is_enabled(region)) {
+		err(ctx, "%s: can't clear targets on an active region\n",
+		    devname);
+		return -EBUSY;
+	}
+
+	ways = cxl_region_get_interleave_ways(region);
+	if (ways == 0 || ways == UINT_MAX)
+		return -ENXIO;
+
+	for (i = 0; i < ways; i++) {
+		rc = region_write_target(region, i, NULL);
+		if (rc) {
+			err(ctx, "%s: error clearing target%d: %s\n",
+			    devname, i, strerror(-rc));
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int set_region_decode(struct cxl_region *region,
+			     enum cxl_decode_state decode_state)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
+	int len = region->buf_len, rc;
+	char *path = region->dev_buf;
+	char buf[SYSFS_ATTR_SIZE];
+
+	if (snprintf(path, len, "%s/commit", region->dev_path) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return -ENXIO;
+	}
+
+	sprintf(buf, "%d\n", decode_state);
+	rc = sysfs_write_attr(ctx, path, buf);
+	if (rc < 0)
+		return rc;
+
+	region->decode_state = decode_state;
+
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_decode_commit(struct cxl_region *region)
+{
+	return set_region_decode(region, CXL_DECODE_COMMIT);
+}
+
+CXL_EXPORT int cxl_region_decode_reset(struct cxl_region *region)
+{
+	return set_region_decode(region, CXL_DECODE_RESET);
+}
+
 static struct cxl_decoder *__cxl_port_match_decoder(struct cxl_port *port,
 						    const char *ident)
 {
@@ -623,8 +909,8 @@ static struct cxl_decoder *cxl_port_find_decoder(struct cxl_port *port,
 	return NULL;
 }
 
-static struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
-						   const char *ident)
+CXL_EXPORT struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
+						       const char *ident)
 {
 	struct cxl_bus *bus;
 
@@ -1387,6 +1673,18 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
 	} else
 		decoder->mode = CXL_DECODER_MODE_NONE;
 
+	sprintf(path, "%s/interleave_granularity", cxldecoder_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		decoder->interleave_granularity = UINT_MAX;
+	else
+		decoder->interleave_granularity = strtoul(buf, NULL, 0);
+
+	sprintf(path, "%s/interleave_ways", cxldecoder_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		decoder->interleave_ways = UINT_MAX;
+	else
+		decoder->interleave_ways = strtoul(buf, NULL, 0);
+
 	switch (port->type) {
 	case CXL_PORT_ENDPOINT:
 		sprintf(path, "%s/dpa_resource", cxldecoder_base);
@@ -1719,6 +2017,63 @@ CXL_EXPORT bool cxl_decoder_is_locked(struct cxl_decoder *decoder)
 	return decoder->locked;
 }
 
+CXL_EXPORT unsigned int
+cxl_decoder_get_interleave_granularity(struct cxl_decoder *decoder)
+{
+	return decoder->interleave_granularity;
+}
+
+CXL_EXPORT unsigned int
+cxl_decoder_get_interleave_ways(struct cxl_decoder *decoder)
+{
+	return decoder->interleave_ways;
+}
+
+CXL_EXPORT struct cxl_region *
+cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
+{
+	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
+	char *path = decoder->dev_buf;
+	char buf[SYSFS_ATTR_SIZE];
+	struct cxl_region *region;
+	int rc;
+
+	sprintf(path, "%s/create_pmem_region", decoder->dev_path);
+	rc = sysfs_read_attr(ctx, path, buf);
+	if (rc < 0) {
+		err(ctx, "failed to read new region name: %s\n",
+		    strerror(-rc));
+		return NULL;
+	}
+
+	rc = sysfs_write_attr(ctx, path, buf);
+	if (rc < 0) {
+		err(ctx, "failed to write new region name: %s\n",
+		    strerror(-rc));
+		return NULL;
+	}
+
+	/* create_region was successful, walk to the new region */
+	cxl_region_foreach(decoder, region) {
+		const char *devname = cxl_region_get_devname(region);
+
+		if (strcmp(devname, buf) == 0)
+			goto found;
+	}
+
+	/*
+	 * If walking to the region we just created failed, something has gone
+	 * very wrong. Attempt to delete it to avoid leaving a dangling region
+	 * id behind.
+	 */
+	err(ctx, "failed to add new region to libcxl\n");
+	cxl_region_delete_name(decoder, buf);
+	return NULL;
+
+ found:
+	return region;
+}
+
 CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
 {
 	return decoder->nr_targets;
@@ -1729,6 +2084,24 @@ CXL_EXPORT const char *cxl_decoder_get_devname(struct cxl_decoder *decoder)
 	return devpath_to_devname(decoder->dev_path);
 }
 
+CXL_EXPORT struct cxl_memdev *
+cxl_ep_decoder_get_memdev(struct cxl_decoder *decoder)
+{
+	struct cxl_port *port = cxl_decoder_get_port(decoder);
+	struct cxl_endpoint *ep;
+
+	if (!port)
+		return NULL;
+	if (!cxl_port_is_endpoint(port))
+		return NULL;
+
+	ep = container_of(port, struct cxl_endpoint, port);
+	if (!ep)
+		return NULL;
+
+	return cxl_endpoint_get_memdev(ep);
+}
+
 CXL_EXPORT struct cxl_target *cxl_target_get_first(struct cxl_decoder *decoder)
 {
 	return list_top(&decoder->targets, struct cxl_target, list);
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index c77a7b9..0b84977 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -182,7 +182,13 @@ bool cxl_decoder_is_volatile_capable(struct cxl_decoder *decoder);
 bool cxl_decoder_is_mem_capable(struct cxl_decoder *decoder);
 bool cxl_decoder_is_accelmem_capable(struct cxl_decoder *decoder);
 bool cxl_decoder_is_locked(struct cxl_decoder *decoder);
-
+unsigned int
+cxl_decoder_get_interleave_granularity(struct cxl_decoder *decoder);
+unsigned int cxl_decoder_get_interleave_ways(struct cxl_decoder *decoder);
+struct cxl_region *cxl_decoder_create_pmem_region(struct cxl_decoder *decoder);
+struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
+					    const char *ident);
+struct cxl_memdev *cxl_ep_decoder_get_memdev(struct cxl_decoder *decoder);
 #define cxl_decoder_foreach(port, decoder)                                     \
 	for (decoder = cxl_decoder_get_first(port); decoder != NULL;           \
 	     decoder = cxl_decoder_get_next(decoder))
@@ -231,6 +237,7 @@ int cxl_region_decode_is_committed(struct cxl_region *region);
 int cxl_region_is_enabled(struct cxl_region *region);
 int cxl_region_disable(struct cxl_region *region);
 int cxl_region_enable(struct cxl_region *region);
+int cxl_region_delete(struct cxl_region *region);
 struct cxl_ctx *cxl_region_get_ctx(struct cxl_region *region);
 struct cxl_decoder *cxl_region_get_decoder(struct cxl_region *region);
 int cxl_region_get_id(struct cxl_region *region);
@@ -240,6 +247,20 @@ unsigned long long cxl_region_get_size(struct cxl_region *region);
 unsigned long long cxl_region_get_resource(struct cxl_region *region);
 unsigned int cxl_region_get_interleave_ways(struct cxl_region *region);
 unsigned int cxl_region_get_interleave_granularity(struct cxl_region *region);
+struct cxl_decoder *cxl_region_get_target_decoder(struct cxl_region *region,
+						  int position);
+int cxl_region_set_size(struct cxl_region *region, unsigned long long size);
+int cxl_region_set_uuid(struct cxl_region *region, uuid_t uu);
+int cxl_region_set_interleave_ways(struct cxl_region *region,
+				   unsigned int ways);
+int cxl_region_set_interleave_granularity(struct cxl_region *region,
+					  unsigned int granularity);
+int cxl_region_set_target(struct cxl_region *region, int position,
+			  struct cxl_decoder *decoder);
+int cxl_region_clear_target(struct cxl_region *region, int position);
+int cxl_region_clear_all_targets(struct cxl_region *region);
+int cxl_region_decode_commit(struct cxl_region *region);
+int cxl_region_decode_reset(struct cxl_region *region);
 
 #define cxl_region_foreach(decoder, region)                                    \
 	for (region = cxl_region_get_first(decoder); region != NULL;           \
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index e410298..6bf3e91 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -140,6 +140,7 @@ global:
 	cxl_decoder_is_mem_capable;
 	cxl_decoder_is_accelmem_capable;
 	cxl_decoder_is_locked;
+	cxl_decoder_create_pmem_region;
 	cxl_target_get_first;
 	cxl_target_get_next;
 	cxl_target_get_decoder;
@@ -183,6 +184,7 @@ global:
 	cxl_region_is_enabled;
 	cxl_region_disable;
 	cxl_region_enable;
+	cxl_region_delete;
 	cxl_region_get_ctx;
 	cxl_region_get_decoder;
 	cxl_region_get_id;
@@ -192,9 +194,23 @@ global:
 	cxl_region_get_resource;
 	cxl_region_get_interleave_ways;
 	cxl_region_get_interleave_granularity;
+	cxl_region_get_target_decoder;
+	cxl_region_set_size;
+	cxl_region_set_uuid;
+	cxl_region_set_interleave_ways;
+	cxl_region_set_interleave_granularity;
+	cxl_region_set_target;
+	cxl_region_clear_target;
+	cxl_region_clear_all_targets;
+	cxl_region_decode_commit;
+	cxl_region_decode_reset;
 	cxl_mapping_get_first;
 	cxl_mapping_get_next;
 	cxl_mapping_get_decoder;
 	cxl_mapping_get_region;
 	cxl_mapping_get_position;
+	cxl_decoder_get_by_name;
+	cxl_ep_decoder_get_memdev;
+	cxl_decoder_get_interleave_granularity;
+	cxl_decoder_get_interleave_ways;
 } LIBCXL_2;
-- 
2.37.1

[ndctl PATCH v2 06/10] cxl: add a 'create-region' command

[Vishal Verma]

Add a 'create-region' command to cxl-cli that walks the platform's CXL
hierarchy to find an appropriate root decoder based on any options
provided, and uses libcxl APIs to create a 'region' that is comprehended
by libnvdimm and ndctl.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/bus-option.txt         |   5 +
 Documentation/cxl/cxl-create-region.txt  | 114 +++++
 Documentation/cxl/region-description.txt |   7 +
 cxl/builtin.h                            |   1 +
 cxl/filter.h                             |   4 +-
 cxl/cxl.c                                |   1 +
 cxl/region.c                             | 594 +++++++++++++++++++++++
 Documentation/cxl/meson.build            |   2 +
 cxl/meson.build                          |   1 +
 9 files changed, 728 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/bus-option.txt
 create mode 100644 Documentation/cxl/cxl-create-region.txt
 create mode 100644 Documentation/cxl/region-description.txt
 create mode 100644 cxl/region.c

diff --git a/Documentation/cxl/bus-option.txt b/Documentation/cxl/bus-option.txt
new file mode 100644
index 0000000..02e2f08
--- /dev/null
+++ b/Documentation/cxl/bus-option.txt
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-b::
+--bus=::
+	Restrict the operation to the specified bus.
diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
new file mode 100644
index 0000000..15dc742
--- /dev/null
+++ b/Documentation/cxl/cxl-create-region.txt
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-create-region(1)
+====================
+
+NAME
+----
+cxl-create-region - Assemble a CXL region by setting up attributes of its
+constituent CXL memdevs.
+
+SYNOPSIS
+--------
+[verse]
+'cxl create-region [<options>]'
+
+include::region-description.txt[]
+
+For create-region, a size can optionally be specified, but if not, the maximum
+possible size for each memdev will be used up to the available decode capacity
+in the system for the given memory type. For persistent regions a UUID can
+optionally be specified, but if not, one will be generated.
+
+If the region-creation operation is successful, a region object will be
+emitted on stdout in JSON format (see examples). If the specified arguments
+cannot be satisfied with a legal configuration, then an appropriate error will
+be emitted on stderr.
+
+EXAMPLE
+-------
+----
+# cxl create-region -m -d decoder0.1 -w 2 -g 1024 mem0 mem1
+{
+  "region":"region0",
+  "resource":"0xc90000000",
+  "size":"512.00 MiB (536.87 MB)",
+  "interleave_ways":2,
+  "interleave_granularity":1024,
+  "mappings":[
+    {
+      "position":1,
+      "decoder":"decoder4.0"
+    },
+    {
+      "position":0,
+      "decoder":"decoder3.0"
+    }
+  ]
+}
+created 1 region
+----
+
+OPTIONS
+-------
+<target(s)>::
+The CXL targets that should be used to form the region. This is optional,
+as they can be chosen automatically based on other options chosen. The number of
+'target' arguments must match the '--ways' option (if provided). The
+targets may be memdevs, or endpoints. The options below control what type of
+targets are being used.
+
+include::bus-option.txt[]
+
+-m::
+--memdevs::
+	Indicate that the non-option arguments for 'target(s)' refer to memdev
+	names.
+
+-e::
+--ep-decoders::
+	Indicate that the non-option arguments for 'target(s)' refer to endpoint
+	decoder names.
+
+-s::
+--size=::
+	Specify the total size for the new region. This is optional, and by
+	default, the maximum possible size will be used.
+
+-t::
+--type=::
+	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
+
+-U::
+--uuid=::
+	Specify a UUID for the new region. This shouldn't usually need to be
+	specified, as one will be generated by default.
+
+-w::
+--ways=::
+	The number of interleave ways for the new region's interleave. This
+	should be equal to the number of memdevs specified in --memdevs, if
+	--memdevs is being supplied. If --memdevs is not specified, an
+	appropriate number of memdevs will be chosen based on the number of
+	ways specified.
+
+-g::
+--granularity=::
+	The interleave granularity for the new region. Must match the selected
+	root decoder's (if provided) granularity.
+
+-d::
+--decoder=::
+	The root decoder that the region should be created under. If not
+	supplied, the first cross-host bridge (if available), decoder that
+	supports the largest interleave will be chosen.
+
+include::human-option.txt[]
+
+include::debug-option.txt[]
+
+include::../copyright.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1],
diff --git a/Documentation/cxl/region-description.txt b/Documentation/cxl/region-description.txt
new file mode 100644
index 0000000..d7e3077
--- /dev/null
+++ b/Documentation/cxl/region-description.txt
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+
+DESCRIPTION
+-----------
+A CXL region is composed of one or more slices of CXL memdevs, with configurable
+interleave settings - both the number of interleave ways, and the interleave
+granularity.
diff --git a/cxl/builtin.h b/cxl/builtin.h
index 9e6fc62..843bada 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -18,4 +18,5 @@ 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);
 int cmd_disable_bus(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/filter.h b/cxl/filter.h
index 609433c..d22d8b1 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -35,8 +35,10 @@ struct cxl_memdev *util_cxl_memdev_filter(struct cxl_memdev *memdev,
 struct cxl_port *util_cxl_port_filter_by_memdev(struct cxl_port *port,
 						const char *ident,
 						const char *serial);
-struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
+struct cxl_decoder *util_cxl_decoder_filter(struct cxl_decoder *decoder,
 					    const char *__ident);
+struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
+					  const char *__ident);
 
 enum cxl_port_filter_mode {
 	CXL_PF_SINGLE,
diff --git a/cxl/cxl.c b/cxl/cxl.c
index ef4cda9..f0afcfe 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -72,6 +72,7 @@ static struct cmd_struct commands[] = {
 	{ "enable-port", .c_fn = cmd_enable_port },
 	{ "set-partition", .c_fn = cmd_set_partition },
 	{ "disable-bus", .c_fn = cmd_disable_bus },
+	{ "create-region", .c_fn = cmd_create_region },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/region.c b/cxl/region.c
new file mode 100644
index 0000000..8f455ab
--- /dev/null
+++ b/cxl/region.c
@@ -0,0 +1,594 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2020-2022 Intel Corporation. All rights reserved. */
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <limits.h>
+#include <util/log.h>
+#include <uuid/uuid.h>
+#include <util/json.h>
+#include <util/size.h>
+#include <cxl/libcxl.h>
+#include <json-c/json.h>
+#include <util/parse-options.h>
+#include <ccan/minmax/minmax.h>
+#include <ccan/short_types/short_types.h>
+
+#include "filter.h"
+#include "json.h"
+
+static struct region_params {
+	const char *bus;
+	const char *size;
+	const char *ways;
+	const char *granularity;
+	const char *type;
+	const char *root_decoder;
+	const char *region;
+	bool memdevs;
+	bool ep_decoders;
+	bool force;
+	bool human;
+	bool debug;
+} param;
+
+struct parsed_params {
+	u64 size;
+	u64 ep_min_size;
+	unsigned int ways;
+	unsigned int granularity;
+	const char **targets;
+	int num_targets;
+	struct cxl_decoder *root_decoder;
+	enum cxl_decoder_mode mode;
+};
+
+enum region_actions {
+	ACTION_CREATE,
+};
+
+static struct log_ctx rl;
+
+#define BASE_OPTIONS() \
+OPT_STRING('b', "bus", &param.bus, "bus name", \
+	   "Limit operation to the specified bus"), \
+OPT_STRING('d', "decoder", &param.root_decoder, "root decoder name", \
+	   "Limit to / use the specified root decoder"), \
+OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
+
+#define CREATE_OPTIONS() \
+OPT_STRING('s', "size", &param.size, \
+	   "size in bytes or with a K/M/G etc. suffix", \
+	   "total size desired for the resulting region."), \
+OPT_STRING('w', "ways", &param.ways, \
+	   "number of interleave ways", \
+	   "number of memdevs participating in the regions interleave set"), \
+OPT_STRING('g', "granularity", \
+	   &param.granularity, "interleave granularity", \
+	   "granularity of the interleave set"), \
+OPT_STRING('t', "type", &param.type, \
+	   "region type", "region type - 'pmem' or 'ram'"), \
+OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
+	    "non-option arguments are memdevs"), \
+OPT_BOOLEAN('e', "ep-decoders", &param.ep_decoders, \
+	    "non-option arguments are endpoint decoders"), \
+OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats")
+
+static const struct option create_options[] = {
+	BASE_OPTIONS(),
+	CREATE_OPTIONS(),
+	OPT_END(),
+};
+
+
+
+static int parse_create_options(int argc, const char **argv,
+				struct parsed_params *p)
+{
+	int i;
+
+	if (!param.root_decoder) {
+		log_err(&rl, "no root decoder specified\n");
+		return -EINVAL;
+	}
+
+	if (param.type) {
+		if (strcmp(param.type, "ram") == 0)
+			p->mode = CXL_DECODER_MODE_RAM;
+		else if (strcmp(param.type, "volatile") == 0)
+			p->mode = CXL_DECODER_MODE_RAM;
+		else if (strcmp(param.type, "pmem") == 0)
+			p->mode = CXL_DECODER_MODE_PMEM;
+		else {
+			log_err(&rl, "unsupported type: %s\n", param.type);
+			return -EINVAL;
+		}
+	} else
+		p->mode = CXL_DECODER_MODE_PMEM;
+
+	if (param.size) {
+		p->size = parse_size64(param.size);
+		if (p->size == ULLONG_MAX) {
+			log_err(&rl, "Invalid size: %s\n", param.size);
+			return -EINVAL;
+		}
+	}
+
+	if (param.ways) {
+		unsigned long ways = strtoul(param.ways, NULL, 0);
+
+		if (ways == ULONG_MAX || (int)ways <= 0) {
+			log_err(&rl, "Invalid interleave ways: %s\n",
+				param.ways);
+			return -EINVAL;
+		}
+		p->ways = ways;
+	} else if (argc) {
+		p->ways = argc;
+	} else {
+		log_err(&rl,
+			"couldn't determine interleave ways from options or arguments\n");
+		return -EINVAL;
+	}
+
+	if (param.granularity) {
+		unsigned long granularity = strtoul(param.granularity, NULL, 0);
+
+		if (granularity == ULONG_MAX || (int)granularity <= 0) {
+			log_err(&rl, "Invalid interleave granularity: %s\n",
+				param.granularity);
+			return -EINVAL;
+		}
+		p->granularity = granularity;
+	}
+
+
+	if (argc > (int)p->ways) {
+		for (i = p->ways; i < argc; i++)
+			log_err(&rl, "extra argument: %s\n", p->targets[i]);
+		return -EINVAL;
+	}
+
+	if (argc < (int)p->ways) {
+		log_err(&rl,
+			"too few target arguments (%d) for interleave ways (%u)\n",
+			argc, p->ways);
+		return -EINVAL;
+	}
+
+	if (p->size && p->ways) {
+		if (p->size % p->ways) {
+			log_err(&rl,
+				"size (%lu) is not an integral multiple of interleave-ways (%u)\n",
+				p->size, p->ways);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int parse_region_options(int argc, const char **argv,
+				struct cxl_ctx *ctx, enum region_actions action,
+				const struct option *options,
+				struct parsed_params *p, const char *usage)
+{
+	const char * const u[] = {
+		usage,
+		NULL
+	};
+
+	argc = parse_options(argc, argv, options, u, 0);
+	p->targets = argv;
+	p->num_targets = argc;
+
+	if (param.debug) {
+		cxl_set_log_priority(ctx, LOG_DEBUG);
+		rl.log_priority = LOG_DEBUG;
+	} else
+		rl.log_priority = LOG_INFO;
+
+	switch(action) {
+	case ACTION_CREATE:
+		return parse_create_options(argc, argv, p);
+	default:
+		return 0;
+	}
+}
+
+/**
+ * validate_memdev() - match memdev with the target provided,
+ *                     and determine its size contribution
+ * @memdev: cxl_memdev being tested for a match against the named target
+ * @target: target memdev from user (either directly, or deduced via
+ *          endpoint decoder
+ * @p:      params structure
+ *
+ * This is called for each memdev in the system, and only returns 'true' if
+ * the memdev name matches the target argument being tested. Additionally,
+ * it sets an ep_min_size attribute that always contains the size of the
+ * smallest target in the provided list. This is used during the automatic
+ * size determination later, to ensure that all targets contribute equally
+ * to the region in case of unevenly sized memdevs.
+ */
+static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
+			    struct parsed_params *p)
+{
+	const char *devname = cxl_memdev_get_devname(memdev);
+	u64 size;
+
+	if (strcmp(devname, target) != 0)
+		return false;
+
+	size = cxl_memdev_get_pmem_size(memdev);
+	if (!p->ep_min_size)
+		p->ep_min_size = size;
+	else
+		p->ep_min_size = min(p->ep_min_size, size);
+
+	return true;
+}
+
+static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
+{
+	unsigned int i, matched = 0;
+
+	for (i = 0; i < p->ways; i++) {
+		struct cxl_memdev *memdev;
+
+		cxl_memdev_foreach(ctx, memdev)
+			if (validate_memdev(memdev, p->targets[i], p))
+				matched++;
+	}
+	if (matched != p->ways) {
+		log_err(&rl,
+			"one or more memdevs not found in CXL topology\n");
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static int validate_config_ep_decoders(struct cxl_ctx *ctx,
+				   struct parsed_params *p)
+{
+	unsigned int i;
+
+	for (i = 0; i < p->ways; i++) {
+		struct cxl_decoder *decoder;
+		struct cxl_memdev *memdev;
+
+		decoder = cxl_decoder_get_by_name(ctx, p->targets[i]);
+		if (!decoder) {
+			log_err(&rl, "%s not found in CXL topology\n",
+				p->targets[i]);
+			return -ENXIO;
+		}
+
+		memdev = cxl_ep_decoder_get_memdev(decoder);
+		if (!memdev) {
+			log_err(&rl, "could not get memdev from %s\n",
+				p->targets[i]);
+			return -ENXIO;
+		}
+
+		if (!validate_memdev(memdev, cxl_memdev_get_devname(memdev), p))
+			return -ENXIO;
+	}
+
+	return 0;
+}
+
+static int validate_decoder(struct cxl_decoder *decoder,
+			    struct parsed_params *p)
+{
+	const char *devname = cxl_decoder_get_devname(decoder);
+
+	switch(p->mode) {
+	case CXL_DECODER_MODE_RAM:
+		if (!cxl_decoder_is_volatile_capable(decoder)) {
+			log_err(&rl, "%s is not volatile capable\n", devname);
+			return -EINVAL;
+		}
+		break;
+	case CXL_DECODER_MODE_PMEM:
+		if (!cxl_decoder_is_pmem_capable(decoder)) {
+			log_err(&rl, "%s is not pmem capable\n", devname);
+			return -EINVAL;
+		}
+		break;
+	default:
+		log_err(&rl, "unknown type: %s\n", param.type);
+		return -EINVAL;
+	}
+
+	/* TODO check if the interleave config is possible under this decoder */
+
+	return 0;
+}
+
+static int create_region_validate_config(struct cxl_ctx *ctx,
+					 struct parsed_params *p)
+{
+	struct cxl_bus *bus;
+	int rc;
+
+	cxl_bus_foreach(ctx, bus) {
+		struct cxl_decoder *decoder;
+		struct cxl_port *port;
+
+		if (!util_cxl_bus_filter(bus, param.bus))
+			continue;
+
+		port = cxl_bus_get_port(bus);
+		if (!cxl_port_is_root(port))
+			continue;
+
+		cxl_decoder_foreach (port, decoder) {
+			if (util_cxl_decoder_filter(decoder,
+						    param.root_decoder)) {
+				p->root_decoder = decoder;
+				goto found;
+			}
+		}
+	}
+
+found:
+	if (p->root_decoder == NULL) {
+		log_err(&rl, "%s not found in CXL topology\n",
+			param.root_decoder);
+		return -ENXIO;
+	}
+
+	rc = validate_decoder(p->root_decoder, p);
+	if (rc)
+		return rc;
+
+	if (param.memdevs)
+		return validate_config_memdevs(ctx, p);
+
+	return validate_config_ep_decoders(ctx, p);
+}
+
+static struct cxl_decoder *
+cxl_memdev_target_find_decoder(struct cxl_ctx *ctx, const char *memdev_name)
+{
+	struct cxl_endpoint *ep = NULL;
+	struct cxl_decoder *decoder;
+	struct cxl_memdev *memdev;
+	struct cxl_port *port;
+
+	cxl_memdev_foreach(ctx, memdev) {
+		const char *devname = cxl_memdev_get_devname(memdev);
+
+		if (strcmp(devname, memdev_name) != 0)
+			continue;
+
+		ep = cxl_memdev_get_endpoint(memdev);
+	}
+
+	if (!ep) {
+		log_err(&rl, "could not get an endpoint for %s\n",
+			memdev_name);
+		return NULL;
+	}
+
+	port = cxl_endpoint_get_port(ep);
+	if (!port) {
+		log_err(&rl, "could not get a port for %s\n",
+			memdev_name);
+		return NULL;
+	}
+
+	cxl_decoder_foreach(port, decoder)
+		if (cxl_decoder_get_size(decoder) == 0)
+			return decoder;
+
+	log_err(&rl, "could not get a free decoder for %s\n", memdev_name);
+	return NULL;
+}
+
+#define try(prefix, op, dev, p) \
+do { \
+	int __rc = prefix##_##op(dev, p); \
+	if (__rc) { \
+		log_err(&rl, "%s: " #op " failed: %s\n", \
+				prefix##_get_devname(dev), \
+				strerror(abs(__rc))); \
+		rc = __rc; \
+		goto err_delete; \
+	} \
+} while (0)
+
+static int cxl_region_determine_granularity(struct cxl_region *region,
+					    struct parsed_params *p)
+{
+	const char *devname = cxl_region_get_devname(region);
+	unsigned int granularity, ways;
+
+	/* Default granularity will be the root decoder's granularity */
+	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
+	if (granularity == 0 || granularity == UINT_MAX) {
+		log_err(&rl, "%s: unable to determine root decoder granularity\n",
+			devname);
+		return -ENXIO;
+	}
+
+	/* If no user-supplied granularity, just use the default */
+	if (!p->granularity)
+		return granularity;
+
+	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
+	if (ways == 0 || ways == UINT_MAX) {
+		log_err(&rl, "%s: unable to determine root decoder ways\n",
+			devname);
+		return -ENXIO;
+	}
+
+	/* For ways == 1, any user-supplied granularity is fine */
+	if (ways == 1)
+		return p->granularity;
+
+	/*
+	 * For ways > 1, only allow the same granularity as the selected
+	 * root decoder
+	 */
+	if (p->granularity == granularity)
+		return granularity;
+
+	log_err(&rl,
+		"%s: For an x%d root, only root decoder granularity (%d) permitted\n",
+		devname, ways, granularity);
+	return -EINVAL;
+}
+
+static int create_region(struct cxl_ctx *ctx, int *count,
+			 struct parsed_params *p)
+{
+	unsigned long flags = UTIL_JSON_TARGETS;
+	struct json_object *jregion;
+	unsigned int i, granularity;
+	struct cxl_region *region;
+	const char *devname;
+	uuid_t uuid;
+	u64 size;
+	int rc;
+
+	rc = create_region_validate_config(ctx, p);
+	if (rc)
+		return rc;
+
+	if (p->size) {
+		size = p->size;
+	} else if (p->ep_min_size) {
+		size = p->ep_min_size * p->ways;
+	} else {
+		log_err(&rl, "%s: unable to determine region size\n", __func__);
+		return -ENXIO;
+	}
+
+	if (p->mode == CXL_DECODER_MODE_PMEM) {
+		region = cxl_decoder_create_pmem_region(p->root_decoder);
+		if (!region) {
+			log_err(&rl, "failed to create region under %s\n",
+				param.root_decoder);
+			return -ENXIO;
+		}
+	} else {
+		log_err(&rl, "region type '%s' not supported yet\n",
+			param.type);
+		return -EOPNOTSUPP;
+	}
+
+	devname = cxl_region_get_devname(region);
+
+	rc = cxl_region_determine_granularity(region, p);
+	if (rc < 0)
+		goto err_delete;
+	granularity = rc;
+
+	uuid_generate(uuid);
+	try(cxl_region, set_interleave_granularity, region, granularity);
+	try(cxl_region, set_interleave_ways, region, p->ways);
+	try(cxl_region, set_uuid, region, uuid);
+	try(cxl_region, set_size, region, size);
+
+	for (i = 0; i < p->ways; i++) {
+		struct cxl_decoder *ep_decoder = NULL;
+
+		if (param.ep_decoders) {
+			ep_decoder =
+				cxl_decoder_get_by_name(ctx, p->targets[i]);
+			if (cxl_decoder_get_size(ep_decoder) != 0) {
+				log_err(&rl, "%s: %s already in use\n", devname,
+					cxl_decoder_get_devname(ep_decoder));
+				rc = -EBUSY;
+				goto err_delete;
+			}
+		} else if (param.memdevs) {
+			ep_decoder = cxl_memdev_target_find_decoder(
+				ctx, p->targets[i]);
+		}
+		if (!ep_decoder) {
+			rc = -ENXIO;
+			goto err_delete;
+		}
+		if (cxl_decoder_get_mode(ep_decoder) != p->mode) {
+			/*
+			 * We know by this time that the decoder is 'free'.
+			 * For the memdevs path, we would've found a free
+			 * decoder to start with, and for the ep_decoders path
+			 * the size has been checked for 0 above.
+			 * Thus it is safe to change the mode here if needed.
+			 */
+			try(cxl_decoder, set_dpa_size, ep_decoder, 0);
+			try(cxl_decoder, set_mode, ep_decoder, p->mode);
+		}
+		try(cxl_decoder, set_dpa_size, ep_decoder, size/p->ways);
+		rc = cxl_region_set_target(region, i, ep_decoder);
+		if (rc) {
+			log_err(&rl, "%s: failed to set target%d to %s\n",
+				devname, i, p->targets[i]);
+			goto err_delete;
+		}
+	}
+
+	rc = cxl_region_decode_commit(region);
+	if (rc) {
+		log_err(&rl, "%s: failed to commit decode: %s\n", devname,
+			strerror(-rc));
+		goto err_delete;
+	}
+
+	rc = cxl_region_enable(region);
+	if (rc) {
+		log_err(&rl, "%s: failed to enable: %s\n", devname,
+			strerror(-rc));
+		goto err_delete;
+	}
+	*count = 1;
+
+	if (isatty(1))
+		flags |= UTIL_JSON_HUMAN;
+	jregion = util_cxl_region_to_json(region, flags);
+	if (jregion)
+		printf("%s\n", json_object_to_json_string_ext(jregion,
+					JSON_C_TO_STRING_PRETTY));
+
+	return 0;
+
+err_delete:
+	cxl_region_delete(region);
+	return rc;
+}
+
+static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
+			 enum region_actions action,
+			 const struct option *options, struct parsed_params *p,
+			 int *count, const char *u)
+{
+	int rc = -ENXIO;
+
+	log_init(&rl, "cxl region", "CXL_REGION_LOG");
+	rc = parse_region_options(argc, argv, ctx, action, options, p, u);
+	if (rc)
+		return rc;
+
+	if (action == ACTION_CREATE)
+		return create_region(ctx, count, p);
+
+	return rc;
+}
+
+int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	const char *u = "cxl create-region <target0> ... [<options>]";
+	struct parsed_params p = { 0 };
+	int rc, count = 0;
+
+	rc = region_action(argc, argv, ctx, ACTION_CREATE, create_options, &p,
+			   &count, u);
+	log_info(&rl, "created %d region%s\n", count, count == 1 ? "" : "s");
+	return rc == 0 ? 0 : EXIT_FAILURE;
+}
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index 423be90..340cdee 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -23,6 +23,7 @@ filedeps = [
   'memdev-option.txt',
   'labels-options.txt',
   'debug-option.txt',
+  'region-description.txt',
 ]
 
 cxl_manpages = [
@@ -39,6 +40,7 @@ cxl_manpages = [
   'cxl-set-partition.txt',
   'cxl-reserve-dpa.txt',
   'cxl-free-dpa.txt',
+  'cxl-create-region.txt',
 ]
 
 foreach man : cxl_manpages
diff --git a/cxl/meson.build b/cxl/meson.build
index d63dcb1..f2474aa 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -3,6 +3,7 @@ cxl_src = [
   'list.c',
   'port.c',
   'bus.c',
+  'region.c',
   'memdev.c',
   'json.c',
   'filter.c',
-- 
2.37.1

[ndctl PATCH v2 07/10] cxl: add commands to {enable,disable,destroy}-region

[Vishal Verma]

With a template from cxl-create-region in place, add its friends:

  cxl enable-region
  cxl disable-region
  cxl destroy-region

Cc: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-destroy-region.txt |  41 +++++
 Documentation/cxl/cxl-disable-region.txt |  36 +++++
 Documentation/cxl/cxl-enable-region.txt  |  36 +++++
 Documentation/cxl/decoder-option.txt     |   6 +
 cxl/builtin.h                            |   3 +
 cxl/cxl.c                                |   3 +
 cxl/region.c                             | 193 ++++++++++++++++++++++-
 Documentation/cxl/meson.build            |   4 +
 8 files changed, 321 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/cxl-destroy-region.txt
 create mode 100644 Documentation/cxl/cxl-disable-region.txt
 create mode 100644 Documentation/cxl/cxl-enable-region.txt
 create mode 100644 Documentation/cxl/decoder-option.txt

diff --git a/Documentation/cxl/cxl-destroy-region.txt b/Documentation/cxl/cxl-destroy-region.txt
new file mode 100644
index 0000000..74f4093
--- /dev/null
+++ b/Documentation/cxl/cxl-destroy-region.txt
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-destroy-region(1)
+=====================
+
+NAME
+----
+cxl-destroy-region - destroy specified region(s).
+
+SYNOPSIS
+--------
+[verse]
+'cxl destroy-region <region> [<options>]'
+
+include::region-description.txt[]
+
+EXAMPLE
+-------
+----
+# cxl destroy-region all
+destroyed 2 regions
+----
+
+OPTIONS
+-------
+include::bus-option.txt[]
+
+-f::
+--force::
+	Force a destroy operation even if the region is active.
+	This will attempt to disable the region first.
+
+include::decoder-option.txt[]
+
+include::debug-option.txt[]
+
+include::../copyright.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1], linkcxl:cxl-create-region[1]
diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
new file mode 100644
index 0000000..6a39aee
--- /dev/null
+++ b/Documentation/cxl/cxl-disable-region.txt
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-disable-region(1)
+=====================
+
+NAME
+----
+cxl-disable-region - disable specified region(s).
+
+SYNOPSIS
+--------
+[verse]
+'cxl disable-region <region> [<options>]'
+
+include::region-description.txt[]
+
+EXAMPLE
+-------
+----
+# cxl disable-region all
+disabled 2 regions
+----
+
+OPTIONS
+-------
+include::bus-option.txt[]
+
+include::decoder-option.txt[]
+
+include::debug-option.txt[]
+
+include::../copyright.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1], linkcxl:cxl-enable-region[1]
diff --git a/Documentation/cxl/cxl-enable-region.txt b/Documentation/cxl/cxl-enable-region.txt
new file mode 100644
index 0000000..f6ef00f
--- /dev/null
+++ b/Documentation/cxl/cxl-enable-region.txt
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-enable-region(1)
+=====================
+
+NAME
+----
+cxl-enable-region - enable specified region(s).
+
+SYNOPSIS
+--------
+[verse]
+'cxl enable-region <region> [<options>]'
+
+include::region-description.txt[]
+
+EXAMPLE
+-------
+----
+# cxl enable-region all
+enabled 2 regions
+----
+
+OPTIONS
+-------
+include::bus-option.txt[]
+
+include::decoder-option.txt[]
+
+include::debug-option.txt[]
+
+include::../copyright.txt[]
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1], linkcxl:cxl-disable-region[1]
diff --git a/Documentation/cxl/decoder-option.txt b/Documentation/cxl/decoder-option.txt
new file mode 100644
index 0000000..e638d6e
--- /dev/null
+++ b/Documentation/cxl/decoder-option.txt
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+-d::
+--decoder=::
+	The root decoder to limit the operation to. Only regions that are
+	children of the specified decoder will be acted upon.
diff --git a/cxl/builtin.h b/cxl/builtin.h
index 843bada..b28c221 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -19,4 +19,7 @@ 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);
 int cmd_disable_bus(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_enable_region(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_disable_region(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_destroy_region(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index f0afcfe..dd1be7a 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -73,6 +73,9 @@ static struct cmd_struct commands[] = {
 	{ "set-partition", .c_fn = cmd_set_partition },
 	{ "disable-bus", .c_fn = cmd_disable_bus },
 	{ "create-region", .c_fn = cmd_create_region },
+	{ "enable-region", .c_fn = cmd_enable_region },
+	{ "disable-region", .c_fn = cmd_disable_region },
+	{ "destroy-region", .c_fn = cmd_destroy_region },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/region.c b/cxl/region.c
index 8f455ab..3f83dd4 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -46,6 +46,9 @@ struct parsed_params {
 
 enum region_actions {
 	ACTION_CREATE,
+	ACTION_ENABLE,
+	ACTION_DISABLE,
+	ACTION_DESTROY,
 };
 
 static struct log_ctx rl;
@@ -81,7 +84,22 @@ static const struct option create_options[] = {
 	OPT_END(),
 };
 
+static const struct option enable_options[] = {
+	BASE_OPTIONS(),
+	OPT_END(),
+};
 
+static const struct option disable_options[] = {
+	BASE_OPTIONS(),
+	OPT_END(),
+};
+
+static const struct option destroy_options[] = {
+	BASE_OPTIONS(),
+	OPT_BOOLEAN('f', "force", &param.force,
+		    "destroy region even if currently active"),
+	OPT_END(),
+};
 
 static int parse_create_options(int argc, const char **argv,
 				struct parsed_params *p)
@@ -563,12 +581,122 @@ err_delete:
 	return rc;
 }
 
+static int destroy_region(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	unsigned int ways, i;
+	int rc;
+
+	/* First, unbind/disable the region if needed */
+	if (cxl_region_is_enabled(region)) {
+		if (param.force) {
+			rc = cxl_region_disable(region);
+			if (rc) {
+				log_err(&rl, "%s: error disabling region: %s\n",
+					devname, strerror(-rc));
+				return rc;
+			}
+		} else {
+			log_err(&rl, "%s active. Disable it or use --force\n",
+				devname);
+			return -EBUSY;
+		}
+	}
+
+	/* Reset the region decode in preparation for removal */
+	rc = cxl_region_decode_reset(region);
+	if (rc) {
+		log_err(&rl, "%s: failed to reset decode: %s\n", devname,
+			strerror(-rc));
+		return rc;
+	}
+
+	/* Reset all endpoint decoders and region targets */
+	ways = cxl_region_get_interleave_ways(region);
+	if (ways == 0 || ways == UINT_MAX) {
+		log_err(&rl, "%s: error getting interleave ways\n", devname);
+		return -ENXIO;
+	}
+
+	for (i = 0; i < ways; i++) {
+		struct cxl_decoder *ep_decoder;
+
+		ep_decoder = cxl_region_get_target_decoder(region, i);
+		if (!ep_decoder)
+			return -ENXIO;
+
+		rc = cxl_region_clear_target(region, i);
+		if (rc) {
+			log_err(&rl, "%s: clearing target%d failed: %s\n",
+				devname, i, strerror(abs(rc)));
+			return rc;
+		}
+
+		rc = cxl_decoder_set_dpa_size(ep_decoder, 0);
+		if (rc) {
+			log_err(&rl, "%s: set_dpa_size failed: %s\n",
+				cxl_decoder_get_devname(ep_decoder),
+				strerror(abs(rc)));
+			return rc;
+		}
+	}
+
+	/* Finally, delete the region */
+	return cxl_region_delete(region);
+}
+
+static int do_region_xable(struct cxl_region *region, enum region_actions action)
+{
+	switch (action) {
+	case ACTION_ENABLE:
+		return cxl_region_enable(region);
+	case ACTION_DISABLE:
+		return cxl_region_disable(region);
+	case ACTION_DESTROY:
+		return destroy_region(region);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int decoder_region_action(struct parsed_params *p,
+				 struct cxl_decoder *decoder,
+				 enum region_actions action, int *count)
+{
+	struct cxl_region *region, *_r;
+	int rc = 0, err_rc = 0;
+
+	cxl_region_foreach_safe (decoder, region, _r) {
+		int i, match = 0;
+
+		for (i = 0; i < p->num_targets; i++) {
+			if (util_cxl_region_filter(region, p->targets[i])) {
+				match = 1;
+				break;
+			}
+		}
+		if (!match)
+			continue;
+
+		rc = do_region_xable(region, action);
+		if (rc == 0) {
+			*count += 1;
+		} else {
+			log_err(&rl, "%s: failed: %s\n",
+				cxl_region_get_devname(region), strerror(-rc));
+			err_rc = rc;
+		}
+	}
+	return err_rc ? err_rc : rc;
+}
+
 static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 			 enum region_actions action,
 			 const struct option *options, struct parsed_params *p,
 			 int *count, const char *u)
 {
-	int rc = -ENXIO;
+	int rc = 0, err_rc = 0;
+	struct cxl_bus *bus;
 
 	log_init(&rl, "cxl region", "CXL_REGION_LOG");
 	rc = parse_region_options(argc, argv, ctx, action, options, p, u);
@@ -578,6 +706,33 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 	if (action == ACTION_CREATE)
 		return create_region(ctx, count, p);
 
+	cxl_bus_foreach(ctx, bus) {
+		struct cxl_decoder *decoder;
+		struct cxl_port *port;
+
+		if (!util_cxl_bus_filter(bus, param.bus))
+			continue;
+
+		port = cxl_bus_get_port(bus);
+		if (!cxl_port_is_root(port))
+			continue;
+
+		cxl_decoder_foreach (port, decoder) {
+			decoder = util_cxl_decoder_filter(decoder,
+							  param.root_decoder);
+			if (!decoder)
+				continue;
+			rc = decoder_region_action(p, decoder, action, count);
+			if (rc)
+				err_rc = rc;
+		}
+	}
+
+	if (err_rc) {
+		log_err(&rl, "one or more failures, last failure: %s\n",
+			strerror(-err_rc));
+		return err_rc;
+	}
 	return rc;
 }
 
@@ -592,3 +747,39 @@ int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx)
 	log_info(&rl, "created %d region%s\n", count, count == 1 ? "" : "s");
 	return rc == 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_enable_region(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	const char *u = "cxl enable-region <region0> ... [<options>]";
+	struct parsed_params p = { 0 };
+	int rc, count = 0;
+
+	rc = region_action(argc, argv, ctx, ACTION_ENABLE, enable_options, &p,
+			   &count, u);
+	log_info(&rl, "enabled %d region%s\n", count, count == 1 ? "" : "s");
+	return rc == 0 ? 0 : EXIT_FAILURE;
+}
+
+int cmd_disable_region(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	const char *u = "cxl disable-region <region0> ... [<options>]";
+	struct parsed_params p = { 0 };
+	int rc, count = 0;
+
+	rc = region_action(argc, argv, ctx, ACTION_DISABLE, disable_options, &p,
+			   &count, u);
+	log_info(&rl, "disabled %d region%s\n", count, count == 1 ? "" : "s");
+	return rc == 0 ? 0 : EXIT_FAILURE;
+}
+
+int cmd_destroy_region(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	const char *u = "cxl destroy-region <region0> ... [<options>]";
+	struct parsed_params p = { 0 };
+	int rc, count = 0;
+
+	rc = region_action(argc, argv, ctx, ACTION_DESTROY, destroy_options, &p,
+			   &count, u);
+	log_info(&rl, "destroyed %d region%s\n", count, count == 1 ? "" : "s");
+	return rc == 0 ? 0 : EXIT_FAILURE;
+}
diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build
index 340cdee..147ea71 100644
--- a/Documentation/cxl/meson.build
+++ b/Documentation/cxl/meson.build
@@ -24,6 +24,7 @@ filedeps = [
   'labels-options.txt',
   'debug-option.txt',
   'region-description.txt',
+  'decoder-option.txt',
 ]
 
 cxl_manpages = [
@@ -41,6 +42,9 @@ cxl_manpages = [
   'cxl-reserve-dpa.txt',
   'cxl-free-dpa.txt',
   'cxl-create-region.txt',
+  'cxl-disable-region.txt',
+  'cxl-enable-region.txt',
+  'cxl-destroy-region.txt',
 ]
 
 foreach man : cxl_manpages
-- 
2.37.1

[ndctl PATCH v2 08/10] cxl/list: make memdevs and regions the default listing

[Vishal Verma]

Instead of only listing regions by default (which can often be empty if
no regions have been configured), change the default listing mode to
both memdevs and regions. This will allow a plain 'cxl-list' to be a
quick health check of whether all the expected memdevs have enumerated
correctly, and see any regions that have been configured.

Cc: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/list.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cxl/list.c b/cxl/list.c
index 88ca9d9..5f604ec 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -100,9 +100,10 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 			param.regions = true;
 	}
 
-	/* List regions by default */
+	/* List regions and memdevs by default */
 	if (num_list_flags() == 0) {
 		param.regions = true;
+		param.memdevs = true;
 	}
 
 	log_init(&param.ctx, "cxl list", "CXL_LIST_LOG");
-- 
2.37.1

[ndctl PATCH v2 09/10] test: add a cxl-create-region test

[Vishal Verma]

Add a unit test to exercise the cxl-create-region command with different
combinations of memdevs and decoders, using cxl_test based mocked
devices.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 test/cxl-create-region.sh | 126 ++++++++++++++++++++++++++++++++++++++
 test/meson.build          |   2 +
 2 files changed, 128 insertions(+)
 create mode 100644 test/cxl-create-region.sh

diff --git a/test/cxl-create-region.sh b/test/cxl-create-region.sh
new file mode 100644
index 0000000..8e297ca
--- /dev/null
+++ b/test/cxl-create-region.sh
@@ -0,0 +1,126 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 Intel Corporation. All rights reserved.
+
+. $(dirname $0)/common
+
+rc=1
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+udevadm settle
+
+destroy_regions()
+{
+	if [[ "$*" ]]; then
+		$CXL destroy-region -f -b cxl_test "$@"
+	else
+		$CXL destroy-region -f -b cxl_test all
+	fi
+}
+
+create_x1_region()
+{
+	mem="$1"
+
+	# find a pmem capable root decoder for this mem
+	decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
+		  jq -r ".[] |
+		  select(.pmem_capable == true) |
+		  select(.nr_targets == 1) |
+		  .decoder")
+
+	if [[ ! $decoder ]]; then
+		echo "no suitable decoder found for $mem, skipping"
+		return
+	fi
+	op_attempted=1
+
+	# create region
+	region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+
+	if [[ ! $region ]]; then
+		echo "create-region failed for $decoder / $mem"
+		err "$LINENO"
+	fi
+
+	# cycle disable/enable
+	$CXL disable-region --bus=cxl_test "$region"
+	$CXL enable-region --bus=cxl_test "$region"
+
+	# cycle destroying and creating the same region
+	destroy_regions "$region"
+	region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+
+	if [[ ! $region ]]; then
+		echo "create-region failed for $decoder / $mem"
+		err "$LINENO"
+	fi
+}
+
+create_subregions()
+{
+	slice=$((256 << 20))
+	mem="$1"
+
+	# find a pmem capable root decoder for this mem
+	decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
+		  jq -r ".[] |
+		  select(.pmem_capable == true) |
+		  select(.nr_targets == 1) |
+		  .decoder")
+
+	if [[ ! $decoder ]]; then
+		echo "no suitable decoder found for $mem, skipping"
+		return
+	fi
+	op_attempted=1
+
+	size="$($CXL list -m "$mem" | jq -r '.[].pmem_size')"
+	if [[ ! $size ]]; then
+		echo "$mem: unable to determine size"
+		err "$LINENO"
+	fi
+
+	num_regions=$((size / slice))
+
+	declare -a regions
+	for (( i = 0; i < num_regions; i++ )); do
+		regions[$i]=$($CXL create-region -d "$decoder" -m "$mem" -s "$slice" | jq -r ".region")
+		if [[ ! $region ]]; then
+			echo "create sub-region failed for $decoder / $mem"
+			err "$LINENO"
+		fi
+		udevadm settle
+	done
+
+	for (( i = (num_regions - 1); i >= 0; i-- )); do
+		destroy_regions "${regions[$i]}"
+	done
+}
+
+# test reading labels directly through cxl-cli
+readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
+
+for mem in ${mems[@]}; do
+	op_attempted=0
+	create_x1_region "$mem"
+	if (( op_attempted == 1 )); then
+		destroy_regions
+	fi
+done
+
+# test multiple subregions under the same decoder, using slices of the same memdev
+# to test out back-to-back pmem DPA allocations on memdevs
+for mem in ${mems[@]}; do
+	op_attempted=0
+	create_subregions "$mem"
+done
+
+modprobe -r cxl_test
diff --git a/test/meson.build b/test/meson.build
index b382f46..5953c28 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -153,6 +153,7 @@ track_uuid = find_program('track-uuid.sh')
 cxl_topo = find_program('cxl-topology.sh')
 cxl_sysfs = find_program('cxl-region-sysfs.sh')
 cxl_labels = find_program('cxl-labels.sh')
+cxl_create_region = find_program('cxl-create-region.sh')
 
 tests = [
   [ 'libndctl',               libndctl,		  'ndctl' ],
@@ -180,6 +181,7 @@ tests = [
   [ 'cxl-topology.sh',	      cxl_topo,		  'cxl'   ],
   [ 'cxl-region-sysfs.sh',    cxl_sysfs,	  'cxl'   ],
   [ 'cxl-labels.sh',          cxl_labels,	  'cxl'   ],
+  [ 'cxl-create-region.sh',   cxl_create_region,  'cxl'   ],
 ]
 
 if get_option('destructive').enabled()
-- 
2.37.1

[ndctl PATCH v2 10/10] cxl/decoder: add a max_available_extent attribute

[Vishal Verma]

Add a max_available_extent attribute to cxl_decoder. In order to aid in
its calculation, change the order of regions in the root decoder's list
to be sorted by start HPA of the region.

Additionally, emit this attribute in decoder listings, and consult it
for available space before creating a new region.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/lib/private.h  |  1 +
 cxl/lib/libcxl.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++-
 cxl/libcxl.h       |  3 ++
 cxl/json.c         |  8 +++++
 cxl/region.c       | 14 +++++++-
 cxl/lib/libcxl.sym |  1 +
 6 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 8619bb1..8705e46 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -104,6 +104,7 @@ struct cxl_decoder {
 	u64 size;
 	u64 dpa_resource;
 	u64 dpa_size;
+	u64 max_available_extent;
 	void *dev_buf;
 	size_t buf_len;
 	char *dev_path;
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index b4d7890..3c1a2c3 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -446,6 +446,11 @@ CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
 	return 0;
 }
 
+static int region_start_cmp(struct cxl_region *r1, struct cxl_region *r2)
+{
+	return ((r1->start < r2->start) ? -1 : 1);
+}
+
 static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 {
 	const char *devname = devpath_to_devname(cxlregion_base);
@@ -528,7 +533,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 			return region_dup;
 		}
 
-	list_add(&decoder->regions, &region->list);
+	list_add_sorted(&decoder->regions, region, list, region_start_cmp);
 
 	return region;
 err:
@@ -1606,6 +1611,74 @@ cxl_endpoint_get_memdev(struct cxl_endpoint *endpoint)
 	return NULL;
 }
 
+static int cxl_region_is_configured(struct cxl_region *region)
+{
+	if ((region->start == 0) && (region->size == 0) &&
+	    (region->decode_state == CXL_DECODE_RESET))
+		return 0;
+	return 1;
+}
+
+/**
+ * cxl_decoder_calc_max_available_extent() - calculate max available free space
+ * @decoder - the root decoder to calculate the free extents for
+ *
+ * The add_cxl_region() function  adds regions to the parent decoder's list
+ * sorted by the region's start HPAs. It can also be assumed that regions have
+ * no overlapped / aliased HPA space. Therefore, calculating each extent is as
+ * simple as walking the region list in order, and subtracting the previous
+ * region's end HPA from the next region's start HPA (and taking into account
+ * the decoder's start and end HPAs as well).
+ */
+static unsigned long long
+cxl_decoder_calc_max_available_extent(struct cxl_decoder *decoder)
+{
+	struct cxl_port *port = cxl_decoder_get_port(decoder);
+	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
+	u64 prev_end = 0, max_extent = 0;
+	struct cxl_region *region;
+
+	if (!cxl_port_is_root(port)) {
+		err(ctx, "%s: not a root decoder\n",
+		    cxl_decoder_get_devname(decoder));
+		return ULLONG_MAX;
+	}
+
+	/*
+	 * Preload prev_end with decoder's start, so that the extent
+	 * calculation for the first region Just Works
+	 */
+	prev_end = decoder->start;
+
+	cxl_region_foreach(decoder, region) {
+		if (!cxl_region_is_configured(region))
+			continue;
+
+		/*
+		 * Note: Normally, end = (start + size - 1), but since
+		 * this is calculating extents between regions, it can
+		 * skip the '- 1'. For example, if a region ends at 0xfff,
+		 * and the next region immediately starts at 0x1000,
+		 * the 'extent' between them is 0, not 1. With
+		 * end = (start + size), this new 'adjusted' end for the
+		 * first region will have been calculated as 0x1000.
+		 * Subtracting the next region's start (0x1000) from this
+		 * correctly gets the extent size as 0.
+		 */
+		max_extent = max(max_extent, region->start - prev_end);
+		prev_end = region->start + region->size;
+	}
+
+	/*
+	 * Finally, consider the extent after the last region, up to the end
+	 * of the decoder's address space, if any. If there were no regions,
+	 * this simply reduces to decoder->size.
+	 */
+	max_extent = max(max_extent, decoder->start + decoder->size - prev_end);
+
+	return max_extent;
+}
+
 static int decoder_id_cmp(struct cxl_decoder *d1, struct cxl_decoder *d2)
 {
 	return d1->id - d2->id;
@@ -1735,6 +1808,8 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
 			if (sysfs_read_attr(ctx, path, buf) == 0)
 				*(flag->flag) = !!strtoul(buf, NULL, 0);
 		}
+		decoder->max_available_extent =
+			cxl_decoder_calc_max_available_extent(decoder);
 		break;
 	}
 	}
@@ -1899,6 +1974,12 @@ cxl_decoder_get_dpa_size(struct cxl_decoder *decoder)
 	return decoder->dpa_size;
 }
 
+CXL_EXPORT unsigned long long
+cxl_decoder_get_max_available_extent(struct cxl_decoder *decoder)
+{
+	return decoder->max_available_extent;
+}
+
 CXL_EXPORT int cxl_decoder_set_dpa_size(struct cxl_decoder *decoder,
 					unsigned long long size)
 {
@@ -2053,6 +2134,9 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
 		return NULL;
 	}
 
+	/* Force a re-init of regions so that the new one can be discovered */
+	free_regions(decoder);
+
 	/* create_region was successful, walk to the new region */
 	cxl_region_foreach(decoder, region) {
 		const char *devname = cxl_region_get_devname(region);
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 0b84977..6a23238 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -133,6 +133,9 @@ unsigned long long cxl_decoder_get_resource(struct cxl_decoder *decoder);
 unsigned long long cxl_decoder_get_size(struct cxl_decoder *decoder);
 unsigned long long cxl_decoder_get_dpa_resource(struct cxl_decoder *decoder);
 unsigned long long cxl_decoder_get_dpa_size(struct cxl_decoder *decoder);
+unsigned long long
+cxl_decoder_get_max_available_extent(struct cxl_decoder *decoder);
+
 enum cxl_decoder_mode {
 	CXL_DECODER_MODE_NONE,
 	CXL_DECODER_MODE_MIXED,
diff --git a/cxl/json.c b/cxl/json.c
index ad93413..28d9936 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -499,6 +499,14 @@ struct json_object *util_cxl_decoder_to_json(struct cxl_decoder *decoder,
 	}
 
 	if (cxl_port_is_root(port) && cxl_decoder_is_mem_capable(decoder)) {
+		size = cxl_decoder_get_max_available_extent(decoder);
+		if (size < ULLONG_MAX) {
+			jobj = util_json_object_hex(size, flags);
+			if (jobj)
+				json_object_object_add(jdecoder,
+						       "max_available_extent",
+						       jobj);
+		}
 		if (cxl_decoder_is_pmem_capable(decoder)) {
 			jobj = json_object_new_boolean(true);
 			if (jobj)
diff --git a/cxl/region.c b/cxl/region.c
index 3f83dd4..cc0f8bc 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -468,9 +468,9 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 	struct json_object *jregion;
 	unsigned int i, granularity;
 	struct cxl_region *region;
+	u64 size, max_extent;
 	const char *devname;
 	uuid_t uuid;
-	u64 size;
 	int rc;
 
 	rc = create_region_validate_config(ctx, p);
@@ -485,6 +485,18 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 		log_err(&rl, "%s: unable to determine region size\n", __func__);
 		return -ENXIO;
 	}
+	max_extent = cxl_decoder_get_max_available_extent(p->root_decoder);
+	if (max_extent == ULLONG_MAX) {
+		log_err(&rl, "%s: unable to determine max extent\n",
+			cxl_decoder_get_devname(p->root_decoder));
+		return -EINVAL;
+	}
+	if (size > max_extent) {
+		log_err(&rl,
+			"%s: region size %#lx exceeds max available space\n",
+			cxl_decoder_get_devname(p->root_decoder), size);
+		return -ENOSPC;
+	}
 
 	if (p->mode == CXL_DECODER_MODE_PMEM) {
 		region = cxl_decoder_create_pmem_region(p->root_decoder);
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 6bf3e91..454bfeb 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -213,4 +213,5 @@ global:
 	cxl_ep_decoder_get_memdev;
 	cxl_decoder_get_interleave_granularity;
 	cxl_decoder_get_interleave_ways;
+	cxl_decoder_get_max_available_extent;
 } LIBCXL_2;
-- 
2.37.1

RE: [ndctl PATCH v2 04/10] cxl-cli: add region listing support

[Dan Williams]

Vishal Verma wrote:
> Add cxl_region -> json and cxl_mapping -> json emitter helpers, and
> teach cxl_filter_walk about cxl_regions. With these in place, 'cxl-list'
> can now emit json objects for CXL regions. They can be top-level objects
> if requested by themselves, or nested under root-decoders, if listed
> along with decoders. Allow a plain 'cxl list' command to imply
> '--regions'.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-list.txt |  13 ++-
>  cxl/filter.h                   |   4 +
>  cxl/json.h                     |   5 ++
>  cxl/filter.c                   | 158 +++++++++++++++++++++++++++++++--
>  cxl/json.c                     | 123 +++++++++++++++++++++++++
>  cxl/list.c                     |  25 +++---
>  6 files changed, 304 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index f6aba0c..2906c2f 100644
> --- a/Documentation/cxl/cxl-list.txt
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -309,8 +309,9 @@ OPTIONS
>  
>  -T::
>  --targets::
> -	Extend decoder listings with downstream port target information, and /
> -	or port and bus listings with the downstream port information.
> +	Extend decoder listings with downstream port target information, port
> +	and bus listings with the downstream port information, and / or regions
> +	with mapping information.
>  ----
>  # cxl list -BTu -b ACPI.CXL
>  {
> @@ -327,6 +328,14 @@ OPTIONS
>  }
>  ----
>  
> +-R::
> +--regions::
> +	Include region objects in the listing.
> +
> +-r::
> +--region::
> +	Specify the region name to filter the emitted regions.

Should this match the template of the other filter options, i.e.:

"Specify CXL region device name(s), or device id(s), to filter the
listing."

...because -r may be a list and it may filter more than just emitted regions.

Other than that, it looks good to me.

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

RE: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

[Dan Williams]

Vishal Verma wrote:
> Add libcxl APIs to create a region under a given root decoder, and to
> set different attributes for the new region. These allow setting the
> size, interleave_ways, interleave_granularity, uuid, and the target
> devices for the newly minted cxl_region object.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/lib/libcxl.txt |  69 ++++++
>  cxl/lib/private.h                |   2 +
>  cxl/lib/libcxl.c                 | 377 ++++++++++++++++++++++++++++++-
>  cxl/libcxl.h                     |  23 +-
>  cxl/lib/libcxl.sym               |  16 ++
>  5 files changed, 484 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index 7a38ce4..c3a8f36 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -508,6 +508,75 @@ device to represent the root of a PCI device hierarchy. The
>  cxl_target_get_physical_node() helper returns the device name of that
>  companion object in the PCI hierarchy.
>  
> +==== REGIONS
> +A CXL region is composed of one or more slices of CXL memdevs, with configurable
> +interleave settings - both the number of interleave ways, and the interleave
> +granularity. In terms of hierarchy, it is the child of a CXL root decoder. A root
> +decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), may have
> +multiple chile regions, but a region is strictly tied to one root decoder.

Mmm, that's a spicy region.

s/chile/child/

> +
> +A region also defines a set of mappings which are slices of capacity on a memdev,

Since the above already defined that a region is composed of one or more
slices of CXL memdevs, how about:

"The slices that compose a region are called mappings. A mapping is a
tuple of 'memdev', 'endpoint decoder', and the 'position'.

> +each represented by an endpoint decoder.
> +
> +===== REGION: Enumeration
> +----
> +struct cxl_region *cxl_region_get_first(struct cxl_decoder *decoder);
> +struct cxl_region *cxl_region_get_next(struct cxl_region *region);
> +
> +#define cxl_region_foreach(decoder, region)                                    \
> +	for (region = cxl_region_get_first(decoder); region != NULL;           \
> +	     region = cxl_region_get_next(region))
> +
> +#define cxl_region_foreach_safe(decoder, region, _region)                      \
> +	for (region = cxl_region_get_first(decoder),                           \
> +	     _region = region ? cxl_region_get_next(region) : NULL;            \
> +	     region != NULL;                                                   \
> +	     region = _region,                                                 \
> +	     _region = _region ? cxl_region_get_next(_region) : NULL)
> +----
> +
> +===== REGION: Attributes
> +----
> +int cxl_region_get_id(struct cxl_region *region);
> +const char *cxl_region_get_devname(struct cxl_region *region);
> +void cxl_region_get_uuid(struct cxl_region *region, uuid_t uu);
> +unsigned long long cxl_region_get_size(struct cxl_region *region);
> +unsigned long long cxl_region_get_resource(struct cxl_region *region);
> +unsigned int cxl_region_get_interleave_ways(struct cxl_region *region);
> +unsigned int cxl_region_get_interleave_granularity(struct cxl_region *region);
> +struct cxl_decoder *cxl_region_get_target_decoder(struct cxl_region *region,
> +						  int position);
> +int cxl_region_set_size(struct cxl_region *region, unsigned long long size);
> +int cxl_region_set_uuid(struct cxl_region *region, uuid_t uu);
> +int cxl_region_set_interleave_ways(struct cxl_region *region,
> +				   unsigned int ways);
> +int cxl_region_set_interleave_granularity(struct cxl_region *region,
> +					  unsigned int granularity);
> +int cxl_region_set_target(struct cxl_region *region, int position,
> +			  struct cxl_decoder *decoder);
> +int cxl_region_clear_target(struct cxl_region *region, int position);
> +int cxl_region_clear_all_targets(struct cxl_region *region);
> +int cxl_region_decode_commit(struct cxl_region *region);
> +int cxl_region_decode_reset(struct cxl_region *region);
> +----
> +
> +A region's resource attribute is the Host Physical Address at which the region's
> +address space starts. The region's address space is a subset of the parent root
> +decoder's address space.
> +
> +The interleave ways is the number of component memdevs participating in the
> +region.
> +
> +The interleave granularity depends on the root decoder's granularity, and must
> +follow the interleave math rules defined in the CXL spec.
> +
> +Regions have a list of targets 0..N, which are programmed with the name of an
> +endpoint decoder under each participating memdev.
> +
> +The 'decode_commit' and 'decode_reset' attributes reserve and free DPA space
> +on a given memdev by allocating an endpoint decoder, and programming it based
> +on the region's interleave geometry.
> +
>  include::../../copyright.txt[]
>  
>  SEE ALSO
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index da49a6c..8619bb1 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -110,6 +110,8 @@ struct cxl_decoder {
>  	int nr_targets;
>  	int id;
>  	enum cxl_decoder_mode mode;
> +	unsigned int interleave_ways;
> +	unsigned int interleave_granularity;
>  	bool pmem_capable;
>  	bool volatile_capable;
>  	bool mem_capable;
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 8dd804c..b4d7890 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -17,6 +17,7 @@
>  #include <ccan/minmax/minmax.h>
>  #include <ccan/array_size/array_size.h>
>  #include <ccan/short_types/short_types.h>
> +#include <ccan/container_of/container_of.h>
>  
>  #include <util/log.h>
>  #include <util/list.h>
> @@ -412,6 +413,39 @@ CXL_EXPORT int cxl_region_enable(struct cxl_region *region)
>  	return 0;
>  }
>  
> +static int cxl_region_delete_name(struct cxl_decoder *decoder,
> +				  const char *devname)
> +{
> +	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> +	char *path = decoder->dev_buf;
> +	int rc;
> +
> +	sprintf(path, "%s/delete_region", decoder->dev_path);
> +	rc = sysfs_write_attr(ctx, path, devname);
> +	if (rc != 0) {
> +		err(ctx, "error deleting region: %s\n", strerror(-rc));
> +		return rc;
> +	}
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
> +{
> +	struct cxl_decoder *decoder = cxl_region_get_decoder(region);
> +	const char *devname = cxl_region_get_devname(region);
> +	int rc;
> +
> +	if (cxl_region_is_enabled(region))
> +		return -EBUSY;
> +
> +	rc = cxl_region_delete_name(decoder, devname);
> +	if (rc != 0)
> +		return rc;
> +
> +	free_region(region);
> +	return 0;
> +}
> +
>  static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  {
>  	const char *devname = devpath_to_devname(cxlregion_base);
> @@ -589,6 +623,258 @@ cxl_region_get_interleave_granularity(struct cxl_region *region)
>  	return region->interleave_granularity;
>  }
>  
> +CXL_EXPORT struct cxl_decoder *
> +cxl_region_get_target_decoder(struct cxl_region *region, int position)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	struct cxl_decoder *decoder;
> +	char buf[SYSFS_ATTR_SIZE];
> +
> +	if (snprintf(path, len, "%s/target%d", region->dev_path, position) >=
> +	    len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return NULL;
> +	}
> +
> +	rc = sysfs_read_attr(ctx, path, buf);
> +	if (rc < 0) {
> +		err(ctx, "%s: error reading target%d: %s\n", devname,
> +		    position, strerror(-rc));
> +		return NULL;
> +	}
> +
> +	decoder = cxl_decoder_get_by_name(ctx, buf);
> +	if (!decoder) {
> +		err(ctx, "%s: error locating decoder for target%d\n", devname,
> +		    position);
> +		return NULL;
> +	}
> +	return decoder;
> +}
> +
> +CXL_EXPORT int cxl_region_set_size(struct cxl_region *region,
> +				   unsigned long long size)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	char buf[SYSFS_ATTR_SIZE];
> +
> +	if (size == 0) {
> +		dbg(ctx, "%s: cannot use %s to delete a region\n", __func__,
> +		    devname);
> +		return -EINVAL;
> +	}
> +
> +	if (snprintf(path, len, "%s/size", region->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	sprintf(buf, "%#llx\n", size);
> +	rc = sysfs_write_attr(ctx, path, buf);
> +	if (rc < 0)
> +		return rc;
> +
> +	region->size = size;
> +
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_set_uuid(struct cxl_region *region, uuid_t uu)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	char uuid[SYSFS_ATTR_SIZE];
> +
> +	if (snprintf(path, len, "%s/uuid", region->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	uuid_unparse(uu, uuid);
> +	rc = sysfs_write_attr(ctx, path, uuid);
> +	if (rc != 0)
> +		return rc;
> +	memcpy(region->uuid, uu, sizeof(uuid_t));
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_set_interleave_ways(struct cxl_region *region,
> +					      unsigned int ways)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	char buf[SYSFS_ATTR_SIZE];
> +
> +	if (snprintf(path, len, "%s/interleave_ways",
> +		     region->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	sprintf(buf, "%u\n", ways);
> +	rc = sysfs_write_attr(ctx, path, buf);
> +	if (rc < 0)
> +		return rc;
> +
> +	region->interleave_ways = ways;
> +
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_set_interleave_granularity(struct cxl_region *region,
> +						     unsigned int granularity)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	char buf[SYSFS_ATTR_SIZE];
> +
> +	if (snprintf(path, len, "%s/interleave_granularity",
> +		     region->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	sprintf(buf, "%u\n", granularity);
> +	rc = sysfs_write_attr(ctx, path, buf);
> +	if (rc < 0)
> +		return rc;
> +
> +	region->interleave_granularity = granularity;
> +
> +	return 0;
> +}
> +
> +static int region_write_target(struct cxl_region *region, int position,
> +			       struct cxl_decoder *decoder)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	const char *dec_name = "";
> +
> +	if (decoder)
> +		dec_name = cxl_decoder_get_devname(decoder);
> +
> +	if (snprintf(path, len, "%s/target%d", region->dev_path, position) >=
> +	    len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	rc = sysfs_write_attr(ctx, path, dec_name);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_set_target(struct cxl_region *region, int position,
> +				     struct cxl_decoder *decoder)
> +{
> +	if (!decoder)
> +		return -ENXIO;
> +
> +	return region_write_target(region, position, decoder);
> +}
> +
> +CXL_EXPORT int cxl_region_clear_target(struct cxl_region *region, int position)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int rc;
> +
> +	if (cxl_region_is_enabled(region)) {
> +		err(ctx, "%s: can't clear targets on an active region\n",
> +		    devname);
> +		return -EBUSY;
> +	}
> +
> +	rc = region_write_target(region, position, NULL);
> +	if (rc) {
> +		err(ctx, "%s: error clearing target%d: %s\n",
> +		    devname, position, strerror(-rc));
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_clear_all_targets(struct cxl_region *region)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	unsigned int ways, i;
> +	int rc;
> +
> +	if (cxl_region_is_enabled(region)) {
> +		err(ctx, "%s: can't clear targets on an active region\n",
> +		    devname);
> +		return -EBUSY;
> +	}
> +
> +	ways = cxl_region_get_interleave_ways(region);
> +	if (ways == 0 || ways == UINT_MAX)
> +		return -ENXIO;
> +
> +	for (i = 0; i < ways; i++) {
> +		rc = region_write_target(region, i, NULL);
> +		if (rc) {
> +			err(ctx, "%s: error clearing target%d: %s\n",
> +			    devname, i, strerror(-rc));
> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int set_region_decode(struct cxl_region *region,
> +			     enum cxl_decode_state decode_state)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> +	int len = region->buf_len, rc;
> +	char *path = region->dev_buf;
> +	char buf[SYSFS_ATTR_SIZE];
> +
> +	if (snprintf(path, len, "%s/commit", region->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return -ENXIO;
> +	}
> +
> +	sprintf(buf, "%d\n", decode_state);
> +	rc = sysfs_write_attr(ctx, path, buf);
> +	if (rc < 0)
> +		return rc;
> +
> +	region->decode_state = decode_state;
> +
> +	return 0;
> +}
> +
> +CXL_EXPORT int cxl_region_decode_commit(struct cxl_region *region)
> +{
> +	return set_region_decode(region, CXL_DECODE_COMMIT);
> +}
> +
> +CXL_EXPORT int cxl_region_decode_reset(struct cxl_region *region)
> +{
> +	return set_region_decode(region, CXL_DECODE_RESET);
> +}
> +
>  static struct cxl_decoder *__cxl_port_match_decoder(struct cxl_port *port,
>  						    const char *ident)
>  {
> @@ -623,8 +909,8 @@ static struct cxl_decoder *cxl_port_find_decoder(struct cxl_port *port,
>  	return NULL;
>  }
>  
> -static struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
> -						   const char *ident)
> +CXL_EXPORT struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
> +						       const char *ident)
>  {
>  	struct cxl_bus *bus;
>  
> @@ -1387,6 +1673,18 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
>  	} else
>  		decoder->mode = CXL_DECODER_MODE_NONE;
>  
> +	sprintf(path, "%s/interleave_granularity", cxldecoder_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		decoder->interleave_granularity = UINT_MAX;
> +	else
> +		decoder->interleave_granularity = strtoul(buf, NULL, 0);
> +
> +	sprintf(path, "%s/interleave_ways", cxldecoder_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		decoder->interleave_ways = UINT_MAX;
> +	else
> +		decoder->interleave_ways = strtoul(buf, NULL, 0);
> +
>  	switch (port->type) {
>  	case CXL_PORT_ENDPOINT:
>  		sprintf(path, "%s/dpa_resource", cxldecoder_base);
> @@ -1719,6 +2017,63 @@ CXL_EXPORT bool cxl_decoder_is_locked(struct cxl_decoder *decoder)
>  	return decoder->locked;
>  }
>  
> +CXL_EXPORT unsigned int
> +cxl_decoder_get_interleave_granularity(struct cxl_decoder *decoder)
> +{
> +	return decoder->interleave_granularity;
> +}
> +
> +CXL_EXPORT unsigned int
> +cxl_decoder_get_interleave_ways(struct cxl_decoder *decoder)
> +{
> +	return decoder->interleave_ways;
> +}
> +
> +CXL_EXPORT struct cxl_region *
> +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> +{
> +	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> +	char *path = decoder->dev_buf;
> +	char buf[SYSFS_ATTR_SIZE];
> +	struct cxl_region *region;
> +	int rc;
> +
> +	sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> +	rc = sysfs_read_attr(ctx, path, buf);
> +	if (rc < 0) {
> +		err(ctx, "failed to read new region name: %s\n",
> +		    strerror(-rc));
> +		return NULL;
> +	}
> +
> +	rc = sysfs_write_attr(ctx, path, buf);
> +	if (rc < 0) {
> +		err(ctx, "failed to write new region name: %s\n",
> +		    strerror(-rc));
> +		return NULL;
> +	}

I think there either needs to be a "decoder->regions_init = 0" here, or
a direct call to "add_cxl_region(decoder...)" just in case this context
had already listed regions before creating a new one.

I like the precision of "add_cxl_region()", but that needs to open code
some of the internals of sysfs_device_parse(), so maybe
"decoder->regions_init = 0" is ok for now.

> +
> +	/* create_region was successful, walk to the new region */
> +	cxl_region_foreach(decoder, region) {
> +		const char *devname = cxl_region_get_devname(region);
> +
> +		if (strcmp(devname, buf) == 0)
> +			goto found;
> +	}
> +
> +	/*
> +	 * If walking to the region we just created failed, something has gone
> +	 * very wrong. Attempt to delete it to avoid leaving a dangling region
> +	 * id behind.
> +	 */
> +	err(ctx, "failed to add new region to libcxl\n");
> +	cxl_region_delete_name(decoder, buf);
> +	return NULL;
> +
> + found:
> +	return region;
> +}
> +
>  CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
>  {
>  	return decoder->nr_targets;
> @@ -1729,6 +2084,24 @@ CXL_EXPORT const char *cxl_decoder_get_devname(struct cxl_decoder *decoder)
>  	return devpath_to_devname(decoder->dev_path);
>  }
>  
> +CXL_EXPORT struct cxl_memdev *
> +cxl_ep_decoder_get_memdev(struct cxl_decoder *decoder)

Hmm, this is the only place where the API assumes the type of the
decoder. The other root-only or endpoint-only decoder attribute getters
are just cxl_decoder_get_*(), so I think drop the "_ep".

Other than the items listed above, this looks good.

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

Re: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

[Verma, Vishal L]

On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > Add libcxl APIs to create a region under a given root decoder, and to
> > set different attributes for the new region. These allow setting the
> > size, interleave_ways, interleave_granularity, uuid, and the target
> > devices for the newly minted cxl_region object.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  Documentation/cxl/lib/libcxl.txt |  69 ++++++
> >  cxl/lib/private.h                |   2 +
> >  cxl/lib/libcxl.c                 | 377 ++++++++++++++++++++++++++++++-
> >  cxl/libcxl.h                     |  23 +-
> >  cxl/lib/libcxl.sym               |  16 ++
> >  5 files changed, 484 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > index 7a38ce4..c3a8f36 100644
> > --- a/Documentation/cxl/lib/libcxl.txt
> > +++ b/Documentation/cxl/lib/libcxl.txt
> > @@ -508,6 +508,75 @@ device to represent the root of a PCI device hierarchy. The
> >  cxl_target_get_physical_node() helper returns the device name of that
> >  companion object in the PCI hierarchy.
> >  
> > +==== REGIONS
> > +A CXL region is composed of one or more slices of CXL memdevs, with configurable
> > +interleave settings - both the number of interleave ways, and the interleave
> > +granularity. In terms of hierarchy, it is the child of a CXL root decoder. A root
> > +decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), may have
> > +multiple chile regions, but a region is strictly tied to one root decoder.
> 
> Mmm, that's a spicy region.
> 
> s/chile/child/

Hah yep will fix.

> 
> > +
> > +A region also defines a set of mappings which are slices of capacity on a memdev,
> 
> Since the above already defined that a region is composed of one or more
> slices of CXL memdevs, how about:
> 
> "The slices that compose a region are called mappings. A mapping is a
> tuple of 'memdev', 'endpoint decoder', and the 'position'.

Yep sounds good.

[snip]

> 
> > +CXL_EXPORT struct cxl_region *
> > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> > +{
> > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > +       char *path = decoder->dev_buf;
> > +       char buf[SYSFS_ATTR_SIZE];
> > +       struct cxl_region *region;
> > +       int rc;
> > +
> > +       sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> > +       rc = sysfs_read_attr(ctx, path, buf);
> > +       if (rc < 0) {
> > +               err(ctx, "failed to read new region name: %s\n",
> > +                   strerror(-rc));
> > +               return NULL;
> > +       }
> > +
> > +       rc = sysfs_write_attr(ctx, path, buf);
> > +       if (rc < 0) {
> > +               err(ctx, "failed to write new region name: %s\n",
> > +                   strerror(-rc));
> > +               return NULL;
> > +       }
> 
> I think there either needs to be a "decoder->regions_init = 0" here, or
> a direct call to "add_cxl_region(decoder...)" just in case this context
> had already listed regions before creating a new one.
> 
> I like the precision of "add_cxl_region()", but that needs to open code
> some of the internals of sysfs_device_parse(), so maybe
> "decoder->regions_init = 0" is ok for now.

Yes, I found that out - and added this - in patch 10 (I can instead
move it here - it makes sense).

Until patch 10, during region creation, nothing had done regions_init
until this point, so this happens to work. Patch 10 where we do walk
the regions before this point to calculate the max available space,
necessitates the reset here.

That being said, potentially all of patch 10 is squash-able into
different bits of the series - I left it at the end so the
max_available_extent stuff can be reviewed on its own.

I'm happy to go either way on squashing it or keeping it standalone.

> 
> > +
> > +       /* create_region was successful, walk to the new region */
> > +       cxl_region_foreach(decoder, region) {
> > +               const char *devname = cxl_region_get_devname(region);
> > +
> > +               if (strcmp(devname, buf) == 0)
> > +                       goto found;
> > +       }
> > +
> > +       /*
> > +        * If walking to the region we just created failed, something has gone
> > +        * very wrong. Attempt to delete it to avoid leaving a dangling region
> > +        * id behind.
> > +        */
> > +       err(ctx, "failed to add new region to libcxl\n");
> > +       cxl_region_delete_name(decoder, buf);
> > +       return NULL;
> > +
> > + found:
> > +       return region;
> > +}
> > +
> >  CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
> >  {
> >         return decoder->nr_targets;
> > @@ -1729,6 +2084,24 @@ CXL_EXPORT const char *cxl_decoder_get_devname(struct cxl_decoder *decoder)
> >         return devpath_to_devname(decoder->dev_path);
> >  }
> >  
> > +CXL_EXPORT struct cxl_memdev *
> > +cxl_ep_decoder_get_memdev(struct cxl_decoder *decoder)
> 
> Hmm, this is the only place where the API assumes the type of the
> decoder. The other root-only or endpoint-only decoder attribute getters
> are just cxl_decoder_get_*(), so I think drop the "_ep".

Agreed, will drop the ep_.

> 
> Other than the items listed above, this looks good.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks Dan!

Re: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

[Dan Williams]

Verma, Vishal L wrote:
> On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote:
> > Vishal Verma wrote:
> > > Add libcxl APIs to create a region under a given root decoder, and to
> > > set different attributes for the new region. These allow setting the
> > > size, interleave_ways, interleave_granularity, uuid, and the target
> > > devices for the newly minted cxl_region object.
> > > 
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  Documentation/cxl/lib/libcxl.txt |  69 ++++++
> > >  cxl/lib/private.h                |   2 +
> > >  cxl/lib/libcxl.c                 | 377 ++++++++++++++++++++++++++++++-
> > >  cxl/libcxl.h                     |  23 +-
> > >  cxl/lib/libcxl.sym               |  16 ++
> > >  5 files changed, 484 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > > index 7a38ce4..c3a8f36 100644
> > > --- a/Documentation/cxl/lib/libcxl.txt
> > > +++ b/Documentation/cxl/lib/libcxl.txt
> > > @@ -508,6 +508,75 @@ device to represent the root of a PCI device hierarchy. The
> > >  cxl_target_get_physical_node() helper returns the device name of that
> > >  companion object in the PCI hierarchy.
> > >  
> > > +==== REGIONS
> > > +A CXL region is composed of one or more slices of CXL memdevs, with configurable
> > > +interleave settings - both the number of interleave ways, and the interleave
> > > +granularity. In terms of hierarchy, it is the child of a CXL root decoder. A root
> > > +decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), may have
> > > +multiple chile regions, but a region is strictly tied to one root decoder.
> > 
> > Mmm, that's a spicy region.
> > 
> > s/chile/child/
> 
> Hah yep will fix.
> 
> > 
> > > +
> > > +A region also defines a set of mappings which are slices of capacity on a memdev,
> > 
> > Since the above already defined that a region is composed of one or more
> > slices of CXL memdevs, how about:
> > 
> > "The slices that compose a region are called mappings. A mapping is a
> > tuple of 'memdev', 'endpoint decoder', and the 'position'.
> 
> Yep sounds good.
> 
> [snip]
> 
> > 
> > > +CXL_EXPORT struct cxl_region *
> > > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> > > +{
> > > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > > +       char *path = decoder->dev_buf;
> > > +       char buf[SYSFS_ATTR_SIZE];
> > > +       struct cxl_region *region;
> > > +       int rc;
> > > +
> > > +       sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> > > +       rc = sysfs_read_attr(ctx, path, buf);
> > > +       if (rc < 0) {
> > > +               err(ctx, "failed to read new region name: %s\n",
> > > +                   strerror(-rc));
> > > +               return NULL;
> > > +       }
> > > +
> > > +       rc = sysfs_write_attr(ctx, path, buf);
> > > +       if (rc < 0) {
> > > +               err(ctx, "failed to write new region name: %s\n",
> > > +                   strerror(-rc));
> > > +               return NULL;
> > > +       }
> > 
> > I think there either needs to be a "decoder->regions_init = 0" here, or
> > a direct call to "add_cxl_region(decoder...)" just in case this context
> > had already listed regions before creating a new one.
> > 
> > I like the precision of "add_cxl_region()", but that needs to open code
> > some of the internals of sysfs_device_parse(), so maybe
> > "decoder->regions_init = 0" is ok for now.
> 
> Yes, I found that out - and added this - in patch 10 (I can instead
> move it here - it makes sense).
> 
> Until patch 10, during region creation, nothing had done regions_init
> until this point, so this happens to work. Patch 10 where we do walk
> the regions before this point to calculate the max available space,
> necessitates the reset here.
> 
> That being said, potentially all of patch 10 is squash-able into
> different bits of the series - I left it at the end so the
> max_available_extent stuff can be reviewed on its own.
> 
> I'm happy to go either way on squashing it or keeping it standalone.

If this was the kernel I would say squash, since bisecting might be
impacted, but as long as the fix is there later in the series I think
that's ok.

Note, I was less worried about the cxl-cli tool tripping over this, and
more about 3rd party applications using libcxl, but those are even less
likely to use a mid-release commit.

RE: [ndctl PATCH v2 06/10] cxl: add a 'create-region' command

[Dan Williams]

Vishal Verma wrote:
> Add a 'create-region' command to cxl-cli that walks the platform's CXL
> hierarchy to find an appropriate root decoder based on any options
> provided, and uses libcxl APIs to create a 'region' that is comprehended
> by libnvdimm and ndctl.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/bus-option.txt         |   5 +
>  Documentation/cxl/cxl-create-region.txt  | 114 +++++
>  Documentation/cxl/region-description.txt |   7 +
>  cxl/builtin.h                            |   1 +
>  cxl/filter.h                             |   4 +-
>  cxl/cxl.c                                |   1 +
>  cxl/region.c                             | 594 +++++++++++++++++++++++
>  Documentation/cxl/meson.build            |   2 +
>  cxl/meson.build                          |   1 +
>  9 files changed, 728 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/bus-option.txt
>  create mode 100644 Documentation/cxl/cxl-create-region.txt
>  create mode 100644 Documentation/cxl/region-description.txt
>  create mode 100644 cxl/region.c
> 
> diff --git a/Documentation/cxl/bus-option.txt b/Documentation/cxl/bus-option.txt
> new file mode 100644
> index 0000000..02e2f08
> --- /dev/null
> +++ b/Documentation/cxl/bus-option.txt
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +-b::
> +--bus=::
> +	Restrict the operation to the specified bus.
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> new file mode 100644
> index 0000000..15dc742
> --- /dev/null
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +cxl-create-region(1)
> +====================
> +
> +NAME
> +----
> +cxl-create-region - Assemble a CXL region by setting up attributes of its
> +constituent CXL memdevs.
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'cxl create-region [<options>]'
> +
> +include::region-description.txt[]
> +
> +For create-region, a size can optionally be specified, but if not, the maximum
> +possible size for each memdev will be used up to the available decode capacity
> +in the system for the given memory type. For persistent regions a UUID can
> +optionally be specified, but if not, one will be generated.
> +
> +If the region-creation operation is successful, a region object will be
> +emitted on stdout in JSON format (see examples). If the specified arguments
> +cannot be satisfied with a legal configuration, then an appropriate error will
> +be emitted on stderr.
> +
> +EXAMPLE
> +-------
> +----
> +# cxl create-region -m -d decoder0.1 -w 2 -g 1024 mem0 mem1
> +{
> +  "region":"region0",
> +  "resource":"0xc90000000",
> +  "size":"512.00 MiB (536.87 MB)",
> +  "interleave_ways":2,
> +  "interleave_granularity":1024,
> +  "mappings":[
> +    {
> +      "position":1,
> +      "decoder":"decoder4.0"
> +    },
> +    {
> +      "position":0,
> +      "decoder":"decoder3.0"
> +    }
> +  ]
> +}
> +created 1 region
> +----
> +
> +OPTIONS
> +-------
> +<target(s)>::
> +The CXL targets that should be used to form the region. This is optional,
> +as they can be chosen automatically based on other options chosen. The number of
> +'target' arguments must match the '--ways' option (if provided). The
> +targets may be memdevs, or endpoints. The options below control what type of
> +targets are being used.
> +
> +include::bus-option.txt[]
> +
> +-m::
> +--memdevs::
> +	Indicate that the non-option arguments for 'target(s)' refer to memdev
> +	names.

Are they names or filter parameters ala 'cxl list -m'? I.e. do you
foresee being able to do something like:

"cxl create-region -p $port -m all"

...to just select all the memdevs that are descendants of $port in the
future? More of a curiosity about future possibilities then a request
for change.

> +
> +-e::
> +--ep-decoders::
> +	Indicate that the non-option arguments for 'target(s)' refer to endpoint
> +	decoder names.

I wonder if this should have a note about it being for test-only
purposes? Given the strict CXL decoder allocation rules I wonder if
anyone can use this in practice? There might be some synergy with 'cxl
reserve-dpa' where this option could be used to say "do not allocate new
decoders, and do not reserve more DPA, just try to use the DPA that was
already reserved in the following decoders".

We might even delete this option for now unless I am missing the marquee
use case for it? Because unless someone knows what they are doing they
are almost always going to be wrong.

> +
> +-s::
> +--size=::
> +	Specify the total size for the new region. This is optional, and by
> +	default, the maximum possible size will be used.

How about add:

"The maximum possible size is gated by both the contiguous free HPA
space remaining in the root decoder, and the available DPA space in the
component memdevs."

> +
> +-t::
> +--type=::
> +	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> +
> +-U::
> +--uuid=::
> +	Specify a UUID for the new region. This shouldn't usually need to be
> +	specified, as one will be generated by default.
> +
> +-w::
> +--ways=::
> +	The number of interleave ways for the new region's interleave. This
> +	should be equal to the number of memdevs specified in --memdevs, if
> +	--memdevs is being supplied. If --memdevs is not specified, an
> +	appropriate number of memdevs will be chosen based on the number of
> +	ways specified.

The reverse is also true, right? That if -w is not specified then the
number of ways is determined by the number of targets specified, or by
other default target searches. I guess notes about those enhanced
default modes can wait until more 'create-region' porcelain arrives.

> +
> +-g::
> +--granularity=::
> +	The interleave granularity for the new region. Must match the selected
> +	root decoder's (if provided) granularity.

This just has me thinking that the kernel needs to up-level its code
comments and changelogs on the "why" for this restriction to somewhere
this man page can reference.

However second sentence should be:

"If the root decoder is interleaved across more than one host-bridge
then this value must match that granularity. Otherwise, for
non-interleaved decode windows, any granularity can be
specified as long as all devices support that setting."

As I type that it raises 2 questions:

1/ If someone does "cxl create-region -g 1024" with no other arguments,
will it fallback to a decoder that can support that setting if its first
choice can not?

2/ Per Dave's recent series [1], cxl-cli is missing any consideration
that a given endpoint may not have the support for the interleave
address bits that are necessary for a given 'create-region' request.
Does not need to be solved now, but should be queued up next.

2a/ Same for interleave-ways, there are endpoint capabilities that need
to be accounted.

[1]: https://lore.kernel.org/linux-cxl/165999244272.493131.1975513183227389633.stgit@djiang5-desk4.jf.intel.com/

> +
> +-d::
> +--decoder=::
> +	The root decoder that the region should be created under. If not
> +	supplied, the first cross-host bridge (if available), decoder that
> +	supports the largest interleave will be chosen.
> +
> +include::human-option.txt[]
> +
> +include::debug-option.txt[]
> +
> +include::../copyright.txt[]
> +
> +SEE ALSO
> +--------
> +linkcxl:cxl-list[1],
> diff --git a/Documentation/cxl/region-description.txt b/Documentation/cxl/region-description.txt
> new file mode 100644
> index 0000000..d7e3077
> --- /dev/null
> +++ b/Documentation/cxl/region-description.txt
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +DESCRIPTION
> +-----------
> +A CXL region is composed of one or more slices of CXL memdevs, with configurable
> +interleave settings - both the number of interleave ways, and the interleave
> +granularity.
> diff --git a/cxl/builtin.h b/cxl/builtin.h
> index 9e6fc62..843bada 100644
> --- a/cxl/builtin.h
> +++ b/cxl/builtin.h
> @@ -18,4 +18,5 @@ 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);
>  int cmd_disable_bus(int argc, const char **argv, struct cxl_ctx *ctx);
> +int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
>  #endif /* _CXL_BUILTIN_H_ */
> diff --git a/cxl/filter.h b/cxl/filter.h
> index 609433c..d22d8b1 100644
> --- a/cxl/filter.h
> +++ b/cxl/filter.h
> @@ -35,8 +35,10 @@ struct cxl_memdev *util_cxl_memdev_filter(struct cxl_memdev *memdev,
>  struct cxl_port *util_cxl_port_filter_by_memdev(struct cxl_port *port,
>  						const char *ident,
>  						const char *serial);
> -struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
> +struct cxl_decoder *util_cxl_decoder_filter(struct cxl_decoder *decoder,
>  					    const char *__ident);
> +struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
> +					  const char *__ident);
>  
>  enum cxl_port_filter_mode {
>  	CXL_PF_SINGLE,
> diff --git a/cxl/cxl.c b/cxl/cxl.c
> index ef4cda9..f0afcfe 100644
> --- a/cxl/cxl.c
> +++ b/cxl/cxl.c
> @@ -72,6 +72,7 @@ static struct cmd_struct commands[] = {
>  	{ "enable-port", .c_fn = cmd_enable_port },
>  	{ "set-partition", .c_fn = cmd_set_partition },
>  	{ "disable-bus", .c_fn = cmd_disable_bus },
> +	{ "create-region", .c_fn = cmd_create_region },
>  };
>  
>  int main(int argc, const char **argv)
> diff --git a/cxl/region.c b/cxl/region.c
> new file mode 100644
> index 0000000..8f455ab
> --- /dev/null
> +++ b/cxl/region.c
> @@ -0,0 +1,594 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2020-2022 Intel Corporation. All rights reserved. */
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <limits.h>
> +#include <util/log.h>
> +#include <uuid/uuid.h>
> +#include <util/json.h>
> +#include <util/size.h>
> +#include <cxl/libcxl.h>
> +#include <json-c/json.h>
> +#include <util/parse-options.h>
> +#include <ccan/minmax/minmax.h>
> +#include <ccan/short_types/short_types.h>
> +
> +#include "filter.h"
> +#include "json.h"
> +
> +static struct region_params {
> +	const char *bus;
> +	const char *size;
> +	const char *ways;
> +	const char *granularity;
> +	const char *type;
> +	const char *root_decoder;
> +	const char *region;
> +	bool memdevs;
> +	bool ep_decoders;
> +	bool force;
> +	bool human;
> +	bool debug;
> +} param;
> +
> +struct parsed_params {
> +	u64 size;
> +	u64 ep_min_size;
> +	unsigned int ways;
> +	unsigned int granularity;
> +	const char **targets;
> +	int num_targets;
> +	struct cxl_decoder *root_decoder;
> +	enum cxl_decoder_mode mode;
> +};
> +
> +enum region_actions {
> +	ACTION_CREATE,
> +};
> +
> +static struct log_ctx rl;
> +
> +#define BASE_OPTIONS() \
> +OPT_STRING('b', "bus", &param.bus, "bus name", \
> +	   "Limit operation to the specified bus"), \
> +OPT_STRING('d', "decoder", &param.root_decoder, "root decoder name", \
> +	   "Limit to / use the specified root decoder"), \
> +OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
> +
> +#define CREATE_OPTIONS() \
> +OPT_STRING('s', "size", &param.size, \
> +	   "size in bytes or with a K/M/G etc. suffix", \
> +	   "total size desired for the resulting region."), \
> +OPT_STRING('w', "ways", &param.ways, \
> +	   "number of interleave ways", \
> +	   "number of memdevs participating in the regions interleave set"), \
> +OPT_STRING('g', "granularity", \
> +	   &param.granularity, "interleave granularity", \
> +	   "granularity of the interleave set"), \
> +OPT_STRING('t', "type", &param.type, \
> +	   "region type", "region type - 'pmem' or 'ram'"), \
> +OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
> +	    "non-option arguments are memdevs"), \
> +OPT_BOOLEAN('e', "ep-decoders", &param.ep_decoders, \
> +	    "non-option arguments are endpoint decoders"), \
> +OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats")
> +
> +static const struct option create_options[] = {
> +	BASE_OPTIONS(),
> +	CREATE_OPTIONS(),
> +	OPT_END(),
> +};
> +
> +
> +
> +static int parse_create_options(int argc, const char **argv,
> +				struct parsed_params *p)
> +{
> +	int i;
> +
> +	if (!param.root_decoder) {
> +		log_err(&rl, "no root decoder specified\n");
> +		return -EINVAL;
> +	}
> +
> +	if (param.type) {
> +		if (strcmp(param.type, "ram") == 0)
> +			p->mode = CXL_DECODER_MODE_RAM;
> +		else if (strcmp(param.type, "volatile") == 0)
> +			p->mode = CXL_DECODER_MODE_RAM;
> +		else if (strcmp(param.type, "pmem") == 0)
> +			p->mode = CXL_DECODER_MODE_PMEM;
> +		else {
> +			log_err(&rl, "unsupported type: %s\n", param.type);
> +			return -EINVAL;

This probably wants a common helper that can be shared with
__reserve_dpa() and add_cxl_decoder() that do the same conversion. I.e.
cxl_decoder_mode_name() needs a buddy. That can be a follow on change.
ISTR I might have already noted that, so forgive the duplicate comment.

> +		}
> +	} else
> +		p->mode = CXL_DECODER_MODE_PMEM;
> +
> +	if (param.size) {
> +		p->size = parse_size64(param.size);
> +		if (p->size == ULLONG_MAX) {
> +			log_err(&rl, "Invalid size: %s\n", param.size);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (param.ways) {
> +		unsigned long ways = strtoul(param.ways, NULL, 0);
> +
> +		if (ways == ULONG_MAX || (int)ways <= 0) {
> +			log_err(&rl, "Invalid interleave ways: %s\n",
> +				param.ways);
> +			return -EINVAL;
> +		}
> +		p->ways = ways;
> +	} else if (argc) {
> +		p->ways = argc;

This is where:

    cxl create-region -p $port -m all

...would not work, but maybe requiring explicit targets is ok. There's
so many potential moving pieces in a CXL topology maybe we do not want
to go there with this flexibility.

> +	} else {
> +		log_err(&rl,
> +			"couldn't determine interleave ways from options or arguments\n");
> +		return -EINVAL;
> +	}
> +
> +	if (param.granularity) {
> +		unsigned long granularity = strtoul(param.granularity, NULL, 0);
> +
> +		if (granularity == ULONG_MAX || (int)granularity <= 0) {
> +			log_err(&rl, "Invalid interleave granularity: %s\n",
> +				param.granularity);
> +			return -EINVAL;
> +		}
> +		p->granularity = granularity;
> +	}
> +
> +
> +	if (argc > (int)p->ways) {
> +		for (i = p->ways; i < argc; i++)
> +			log_err(&rl, "extra argument: %s\n", p->targets[i]);
> +		return -EINVAL;
> +	}
> +
> +	if (argc < (int)p->ways) {
> +		log_err(&rl,
> +			"too few target arguments (%d) for interleave ways (%u)\n",
> +			argc, p->ways);
> +		return -EINVAL;
> +	}
> +
> +	if (p->size && p->ways) {
> +		if (p->size % p->ways) {
> +			log_err(&rl,
> +				"size (%lu) is not an integral multiple of interleave-ways (%u)\n",
> +				p->size, p->ways);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int parse_region_options(int argc, const char **argv,
> +				struct cxl_ctx *ctx, enum region_actions action,
> +				const struct option *options,
> +				struct parsed_params *p, const char *usage)
> +{
> +	const char * const u[] = {
> +		usage,
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, options, u, 0);
> +	p->targets = argv;
> +	p->num_targets = argc;
> +
> +	if (param.debug) {
> +		cxl_set_log_priority(ctx, LOG_DEBUG);
> +		rl.log_priority = LOG_DEBUG;
> +	} else
> +		rl.log_priority = LOG_INFO;
> +
> +	switch(action) {
> +	case ACTION_CREATE:
> +		return parse_create_options(argc, argv, p);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +/**
> + * validate_memdev() - match memdev with the target provided,
> + *                     and determine its size contribution
> + * @memdev: cxl_memdev being tested for a match against the named target
> + * @target: target memdev from user (either directly, or deduced via
> + *          endpoint decoder
> + * @p:      params structure
> + *
> + * This is called for each memdev in the system, and only returns 'true' if
> + * the memdev name matches the target argument being tested. Additionally,
> + * it sets an ep_min_size attribute that always contains the size of the
> + * smallest target in the provided list. This is used during the automatic
> + * size determination later, to ensure that all targets contribute equally
> + * to the region in case of unevenly sized memdevs.
> + */
> +static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
> +			    struct parsed_params *p)
> +{
> +	const char *devname = cxl_memdev_get_devname(memdev);
> +	u64 size;
> +
> +	if (strcmp(devname, target) != 0)
> +		return false;
> +
> +	size = cxl_memdev_get_pmem_size(memdev);
> +	if (!p->ep_min_size)
> +		p->ep_min_size = size;
> +	else
> +		p->ep_min_size = min(p->ep_min_size, size);
> +
> +	return true;
> +}
> +
> +static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
> +{
> +	unsigned int i, matched = 0;
> +
> +	for (i = 0; i < p->ways; i++) {
> +		struct cxl_memdev *memdev;
> +
> +		cxl_memdev_foreach(ctx, memdev)
> +			if (validate_memdev(memdev, p->targets[i], p))
> +				matched++;
> +	}
> +	if (matched != p->ways) {
> +		log_err(&rl,
> +			"one or more memdevs not found in CXL topology\n");
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int validate_config_ep_decoders(struct cxl_ctx *ctx,
> +				   struct parsed_params *p)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < p->ways; i++) {
> +		struct cxl_decoder *decoder;
> +		struct cxl_memdev *memdev;
> +
> +		decoder = cxl_decoder_get_by_name(ctx, p->targets[i]);
> +		if (!decoder) {
> +			log_err(&rl, "%s not found in CXL topology\n",
> +				p->targets[i]);
> +			return -ENXIO;
> +		}
> +
> +		memdev = cxl_ep_decoder_get_memdev(decoder);
> +		if (!memdev) {
> +			log_err(&rl, "could not get memdev from %s\n",
> +				p->targets[i]);
> +			return -ENXIO;
> +		}
> +
> +		if (!validate_memdev(memdev, cxl_memdev_get_devname(memdev), p))
> +			return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int validate_decoder(struct cxl_decoder *decoder,
> +			    struct parsed_params *p)
> +{
> +	const char *devname = cxl_decoder_get_devname(decoder);
> +
> +	switch(p->mode) {
> +	case CXL_DECODER_MODE_RAM:
> +		if (!cxl_decoder_is_volatile_capable(decoder)) {
> +			log_err(&rl, "%s is not volatile capable\n", devname);
> +			return -EINVAL;
> +		}
> +		break;
> +	case CXL_DECODER_MODE_PMEM:
> +		if (!cxl_decoder_is_pmem_capable(decoder)) {
> +			log_err(&rl, "%s is not pmem capable\n", devname);
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		log_err(&rl, "unknown type: %s\n", param.type);
> +		return -EINVAL;
> +	}
> +
> +	/* TODO check if the interleave config is possible under this decoder */
> +
> +	return 0;
> +}
> +
> +static int create_region_validate_config(struct cxl_ctx *ctx,
> +					 struct parsed_params *p)
> +{
> +	struct cxl_bus *bus;
> +	int rc;
> +
> +	cxl_bus_foreach(ctx, bus) {
> +		struct cxl_decoder *decoder;
> +		struct cxl_port *port;
> +
> +		if (!util_cxl_bus_filter(bus, param.bus))
> +			continue;
> +
> +		port = cxl_bus_get_port(bus);
> +		if (!cxl_port_is_root(port))
> +			continue;
> +
> +		cxl_decoder_foreach (port, decoder) {
> +			if (util_cxl_decoder_filter(decoder,
> +						    param.root_decoder)) {
> +				p->root_decoder = decoder;
> +				goto found;
> +			}
> +		}
> +	}
> +
> +found:
> +	if (p->root_decoder == NULL) {
> +		log_err(&rl, "%s not found in CXL topology\n",
> +			param.root_decoder);
> +		return -ENXIO;
> +	}
> +
> +	rc = validate_decoder(p->root_decoder, p);
> +	if (rc)
> +		return rc;
> +
> +	if (param.memdevs)
> +		return validate_config_memdevs(ctx, p);
> +
> +	return validate_config_ep_decoders(ctx, p);
> +}
> +
> +static struct cxl_decoder *
> +cxl_memdev_target_find_decoder(struct cxl_ctx *ctx, const char *memdev_name)
> +{
> +	struct cxl_endpoint *ep = NULL;
> +	struct cxl_decoder *decoder;
> +	struct cxl_memdev *memdev;
> +	struct cxl_port *port;
> +
> +	cxl_memdev_foreach(ctx, memdev) {
> +		const char *devname = cxl_memdev_get_devname(memdev);
> +
> +		if (strcmp(devname, memdev_name) != 0)
> +			continue;
> +
> +		ep = cxl_memdev_get_endpoint(memdev);
> +	}
> +
> +	if (!ep) {
> +		log_err(&rl, "could not get an endpoint for %s\n",
> +			memdev_name);
> +		return NULL;
> +	}
> +
> +	port = cxl_endpoint_get_port(ep);
> +	if (!port) {
> +		log_err(&rl, "could not get a port for %s\n",
> +			memdev_name);
> +		return NULL;
> +	}
> +
> +	cxl_decoder_foreach(port, decoder)
> +		if (cxl_decoder_get_size(decoder) == 0)
> +			return decoder;
> +
> +	log_err(&rl, "could not get a free decoder for %s\n", memdev_name);
> +	return NULL;
> +}
> +
> +#define try(prefix, op, dev, p) \
> +do { \
> +	int __rc = prefix##_##op(dev, p); \
> +	if (__rc) { \
> +		log_err(&rl, "%s: " #op " failed: %s\n", \
> +				prefix##_get_devname(dev), \
> +				strerror(abs(__rc))); \
> +		rc = __rc; \
> +		goto err_delete; \
> +	} \
> +} while (0)
> +
> +static int cxl_region_determine_granularity(struct cxl_region *region,
> +					    struct parsed_params *p)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	unsigned int granularity, ways;
> +
> +	/* Default granularity will be the root decoder's granularity */
> +	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
> +	if (granularity == 0 || granularity == UINT_MAX) {
> +		log_err(&rl, "%s: unable to determine root decoder granularity\n",
> +			devname);
> +		return -ENXIO;
> +	}
> +
> +	/* If no user-supplied granularity, just use the default */
> +	if (!p->granularity)
> +		return granularity;
> +
> +	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> +	if (ways == 0 || ways == UINT_MAX) {
> +		log_err(&rl, "%s: unable to determine root decoder ways\n",
> +			devname);
> +		return -ENXIO;
> +	}
> +
> +	/* For ways == 1, any user-supplied granularity is fine */

...modulo a future patch that checks the device's interleave capability.

Rest of the patch looks good. After fixing up the option descriptions
per above you can add:

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

RE: [ndctl PATCH v2 09/10] test: add a cxl-create-region test

[Dan Williams]

Vishal Verma wrote:
> Add a unit test to exercise the cxl-create-region command with different
> combinations of memdevs and decoders, using cxl_test based mocked
> devices.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  test/cxl-create-region.sh | 126 ++++++++++++++++++++++++++++++++++++++
>  test/meson.build          |   2 +
>  2 files changed, 128 insertions(+)
>  create mode 100644 test/cxl-create-region.sh

Sweet, looks good.

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

RE: [ndctl PATCH v2 10/10] cxl/decoder: add a max_available_extent attribute

[Dan Williams]

Vishal Verma wrote:
> Add a max_available_extent attribute to cxl_decoder. In order to aid in
> its calculation, change the order of regions in the root decoder's list
> to be sorted by start HPA of the region.
> 
> Additionally, emit this attribute in decoder listings, and consult it
> for available space before creating a new region.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/lib/private.h  |  1 +
>  cxl/lib/libcxl.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++-
>  cxl/libcxl.h       |  3 ++
>  cxl/json.c         |  8 +++++
>  cxl/region.c       | 14 +++++++-
>  cxl/lib/libcxl.sym |  1 +
>  6 files changed, 111 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index 8619bb1..8705e46 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -104,6 +104,7 @@ struct cxl_decoder {
>  	u64 size;
>  	u64 dpa_resource;
>  	u64 dpa_size;
> +	u64 max_available_extent;
>  	void *dev_buf;
>  	size_t buf_len;
>  	char *dev_path;
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index b4d7890..3c1a2c3 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -446,6 +446,11 @@ CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
>  	return 0;
>  }
>  
> +static int region_start_cmp(struct cxl_region *r1, struct cxl_region *r2)
> +{
> +	return ((r1->start < r2->start) ? -1 : 1);

I think you want 'equal' case too, right?

val = r1->start - r2->start;
if (val > r1->start)
	return -1;
if (val < r1->start)
	return 1;
return 0;

> +}
> +
>  static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  {
>  	const char *devname = devpath_to_devname(cxlregion_base);
> @@ -528,7 +533,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  			return region_dup;
>  		}
>  
> -	list_add(&decoder->regions, &region->list);
> +	list_add_sorted(&decoder->regions, region, list, region_start_cmp);
>  
>  	return region;
>  err:
> @@ -1606,6 +1611,74 @@ cxl_endpoint_get_memdev(struct cxl_endpoint *endpoint)
>  	return NULL;
>  }
>  
> +static int cxl_region_is_configured(struct cxl_region *region)

s/int/bool/

> +{
> +	if ((region->start == 0) && (region->size == 0) &&
> +	    (region->decode_state == CXL_DECODE_RESET))
> +		return 0;
> +	return 1;

That can be squished to just:

	return region->size && region->decode_state != CXL_DECODE_RESET;

...becase region->start == 0 is a valid state for a configured region.


> +}
> +
> +/**
> + * cxl_decoder_calc_max_available_extent() - calculate max available free space
> + * @decoder - the root decoder to calculate the free extents for
> + *
> + * The add_cxl_region() function  adds regions to the parent decoder's list
> + * sorted by the region's start HPAs. It can also be assumed that regions have
> + * no overlapped / aliased HPA space. Therefore, calculating each extent is as
> + * simple as walking the region list in order, and subtracting the previous
> + * region's end HPA from the next region's start HPA (and taking into account
> + * the decoder's start and end HPAs as well).
> + */
> +static unsigned long long
> +cxl_decoder_calc_max_available_extent(struct cxl_decoder *decoder)
> +{
> +	struct cxl_port *port = cxl_decoder_get_port(decoder);
> +	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> +	u64 prev_end = 0, max_extent = 0;
> +	struct cxl_region *region;
> +
> +	if (!cxl_port_is_root(port)) {
> +		err(ctx, "%s: not a root decoder\n",
> +		    cxl_decoder_get_devname(decoder));
> +		return ULLONG_MAX;
> +	}
> +
> +	/*
> +	 * Preload prev_end with decoder's start, so that the extent
> +	 * calculation for the first region Just Works
> +	 */
> +	prev_end = decoder->start;
> +
> +	cxl_region_foreach(decoder, region) {
> +		if (!cxl_region_is_configured(region))
> +			continue;
> +
> +		/*
> +		 * Note: Normally, end = (start + size - 1), but since
> +		 * this is calculating extents between regions, it can
> +		 * skip the '- 1'. For example, if a region ends at 0xfff,
> +		 * and the next region immediately starts at 0x1000,
> +		 * the 'extent' between them is 0, not 1. With
> +		 * end = (start + size), this new 'adjusted' end for the
> +		 * first region will have been calculated as 0x1000.
> +		 * Subtracting the next region's start (0x1000) from this
> +		 * correctly gets the extent size as 0.
> +		 */

Not sure if I prefer this block comment, or just seeing prev_end being
done with -1 math and max_extent doing the + 1 because it's a size...

The latter seems more idiomatic to my eyes, but I'll leave it up to you.

> +		max_extent = max(max_extent, region->start - prev_end);
> +		prev_end = region->start + region->size;
> +	}
> +
> +	/*
> +	 * Finally, consider the extent after the last region, up to the end
> +	 * of the decoder's address space, if any. If there were no regions,
> +	 * this simply reduces to decoder->size.
> +	 */
> +	max_extent = max(max_extent, decoder->start + decoder->size - prev_end);
> +
> +	return max_extent;
> +}
> +
>  static int decoder_id_cmp(struct cxl_decoder *d1, struct cxl_decoder *d2)
>  {
>  	return d1->id - d2->id;
> @@ -1735,6 +1808,8 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
>  			if (sysfs_read_attr(ctx, path, buf) == 0)
>  				*(flag->flag) = !!strtoul(buf, NULL, 0);
>  		}
> +		decoder->max_available_extent =
> +			cxl_decoder_calc_max_available_extent(decoder);
>  		break;
>  	}
>  	}
> @@ -1899,6 +1974,12 @@ cxl_decoder_get_dpa_size(struct cxl_decoder *decoder)
>  	return decoder->dpa_size;
>  }
>  
> +CXL_EXPORT unsigned long long
> +cxl_decoder_get_max_available_extent(struct cxl_decoder *decoder)
> +{
> +	return decoder->max_available_extent;
> +}
> +
>  CXL_EXPORT int cxl_decoder_set_dpa_size(struct cxl_decoder *decoder,
>  					unsigned long long size)
>  {
> @@ -2053,6 +2134,9 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  		return NULL;
>  	}
>  
> +	/* Force a re-init of regions so that the new one can be discovered */
> +	free_regions(decoder);

You do not need to free them, the re-init will do dup detection and
*should* just result in the new one being added. In fact, you probably
do not *want* to free them as that could cause problems for library
users that were holding a 'struct cxl_region' object before the call to
cxl_decoder_create_pmem_region().

With the above fixes folded in you can add:

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

Re: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

[Verma, Vishal L]

On Thu, 2022-08-11 at 11:42 -0700, Dan Williams wrote:
> Verma, Vishal L wrote:
> > On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote:
> > 
> > 
> > [snip]
> > 
> > > 
> > > > +CXL_EXPORT struct cxl_region *
> > > > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> > > > +{
> > > > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > > > +       char *path = decoder->dev_buf;
> > > > +       char buf[SYSFS_ATTR_SIZE];
> > > > +       struct cxl_region *region;
> > > > +       int rc;
> > > > +
> > > > +       sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> > > > +       rc = sysfs_read_attr(ctx, path, buf);
> > > > +       if (rc < 0) {
> > > > +               err(ctx, "failed to read new region name: %s\n",
> > > > +                   strerror(-rc));
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       rc = sysfs_write_attr(ctx, path, buf);
> > > > +       if (rc < 0) {
> > > > +               err(ctx, "failed to write new region name: %s\n",
> > > > +                   strerror(-rc));
> > > > +               return NULL;
> > > > +       }
> > > 
> > > I think there either needs to be a "decoder->regions_init = 0" here, or
> > > a direct call to "add_cxl_region(decoder...)" just in case this context
> > > had already listed regions before creating a new one.
> > > 
> > > I like the precision of "add_cxl_region()", but that needs to open code
> > > some of the internals of sysfs_device_parse(), so maybe
> > > "decoder->regions_init = 0" is ok for now.
> > 
> > Yes, I found that out - and added this - in patch 10 (I can instead
> > move it here - it makes sense).
> > 
> > Until patch 10, during region creation, nothing had done regions_init
> > until this point, so this happens to work. Patch 10 where we do walk
> > the regions before this point to calculate the max available space,
> > necessitates the reset here.
> > 
> > That being said, potentially all of patch 10 is squash-able into
> > different bits of the series - I left it at the end so the
> > max_available_extent stuff can be reviewed on its own.
> > 
> > I'm happy to go either way on squashing it or keeping it standalone.
> 
> If this was the kernel I would say squash, since bisecting might be
> impacted, but as long as the fix is there later in the series I think
> that's ok.
> 
> Note, I was less worried about the cxl-cli tool tripping over this, and
> more about 3rd party applications using libcxl, but those are even less
> likely to use a mid-release commit.


I think moving just the invalidation here to preserve bisectability for
non cxl-cli users makes sense. 

Re: [ndctl PATCH v2 06/10] cxl: add a 'create-region' command

[Verma, Vishal L]

On Thu, 2022-08-11 at 12:34 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > Add a 'create-region' command to cxl-cli that walks the platform's CXL
> > hierarchy to find an appropriate root decoder based on any options
> > provided, and uses libcxl APIs to create a 'region' that is comprehended
> > by libnvdimm and ndctl.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  Documentation/cxl/bus-option.txt         |   5 +
> >  Documentation/cxl/cxl-create-region.txt  | 114 +++++
> >  Documentation/cxl/region-description.txt |   7 +
> >  cxl/builtin.h                            |   1 +
> >  cxl/filter.h                             |   4 +-
> >  cxl/cxl.c                                |   1 +
> >  cxl/region.c                             | 594 +++++++++++++++++++++++
> >  Documentation/cxl/meson.build            |   2 +
> >  cxl/meson.build                          |   1 +
> >  9 files changed, 728 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/cxl/bus-option.txt
> >  create mode 100644 Documentation/cxl/cxl-create-region.txt
> >  create mode 100644 Documentation/cxl/region-description.txt
> >  create mode 100644 cxl/region.c
> > 
> > diff --git a/Documentation/cxl/bus-option.txt b/Documentation/cxl/bus-option.txt
> > new file mode 100644
> > index 0000000..02e2f08
> > --- /dev/null
> > +++ b/Documentation/cxl/bus-option.txt
> > @@ -0,0 +1,5 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +-b::
> > +--bus=::
> > +       Restrict the operation to the specified bus.
> > diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> > new file mode 100644
> > index 0000000..15dc742
> > --- /dev/null
> > +++ b/Documentation/cxl/cxl-create-region.txt
> > @@ -0,0 +1,114 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +cxl-create-region(1)
> > +====================
> > +
> > +NAME
> > +----
> > +cxl-create-region - Assemble a CXL region by setting up attributes of its
> > +constituent CXL memdevs.
> > +
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'cxl create-region [<options>]'
> > +
> > +include::region-description.txt[]
> > +
> > +For create-region, a size can optionally be specified, but if not, the maximum
> > +possible size for each memdev will be used up to the available decode capacity
> > +in the system for the given memory type. For persistent regions a UUID can
> > +optionally be specified, but if not, one will be generated.
> > +
> > +If the region-creation operation is successful, a region object will be
> > +emitted on stdout in JSON format (see examples). If the specified arguments
> > +cannot be satisfied with a legal configuration, then an appropriate error will
> > +be emitted on stderr.
> > +
> > +EXAMPLE
> > +-------
> > +----
> > +# cxl create-region -m -d decoder0.1 -w 2 -g 1024 mem0 mem1
> > +{
> > +  "region":"region0",
> > +  "resource":"0xc90000000",
> > +  "size":"512.00 MiB (536.87 MB)",
> > +  "interleave_ways":2,
> > +  "interleave_granularity":1024,
> > +  "mappings":[
> > +    {
> > +      "position":1,
> > +      "decoder":"decoder4.0"
> > +    },
> > +    {
> > +      "position":0,
> > +      "decoder":"decoder3.0"
> > +    }
> > +  ]
> > +}
> > +created 1 region
> > +----
> > +
> > +OPTIONS
> > +-------
> > +<target(s)>::
> > +The CXL targets that should be used to form the region. This is optional,
> > +as they can be chosen automatically based on other options chosen. The number of
> > +'target' arguments must match the '--ways' option (if provided). The
> > +targets may be memdevs, or endpoints. The options below control what type of
> > +targets are being used.
> > +
> > +include::bus-option.txt[]
> > +
> > +-m::
> > +--memdevs::
> > +       Indicate that the non-option arguments for 'target(s)' refer to memdev
> > +       names.
> 
> Are they names or filter parameters ala 'cxl list -m'? I.e. do you
> foresee being able to do something like:

Hm, in the current implementation they really are just names - I just
use the remaining argv[] as the targets array, but..

> 
> "cxl create-region -p $port -m all"
> 
> ...to just select all the memdevs that are descendants of $port in the
> future? More of a curiosity about future possibilities then a request
> for change.

This sounds like a useful thing to do - perhaps with the next bit of
porcelain additions.

> 
> > +
> > +-e::
> > +--ep-decoders::
> > +       Indicate that the non-option arguments for 'target(s)' refer to endpoint
> > +       decoder names.
> 
> I wonder if this should have a note about it being for test-only
> purposes? Given the strict CXL decoder allocation rules I wonder if
> anyone can use this in practice? There might be some synergy with 'cxl
> reserve-dpa' where this option could be used to say "do not allocate new
> decoders, and do not reserve more DPA, just try to use the DPA that was
> already reserved in the following decoders".
> 
> We might even delete this option for now unless I am missing the marquee
> use case for it? Because unless someone knows what they are doing they
> are almost always going to be wrong.

Yeah I agree - it is definitely not straightforward to use, and I don't
see a practical use case. I think I only had it because the ABI wanted
decoders, and very early implementations had me explicitly asking for
/everything/.

I'm okay adding a note that this shouldn't normally be used, or
removing it entirely.

> 
> > +
> > +-s::
> > +--size=::
> > +       Specify the total size for the new region. This is optional, and by
> > +       default, the maximum possible size will be used.
> 
> How about add:
> 
> "The maximum possible size is gated by both the contiguous free HPA
> space remaining in the root decoder, and the available DPA space in the
> component memdevs."

Yep.

> 
> > +
> > +-t::
> > +--type=::
> > +       Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> > +
> > +-U::
> > +--uuid=::
> > +       Specify a UUID for the new region. This shouldn't usually need to be
> > +       specified, as one will be generated by default.
> > +
> > +-w::
> > +--ways=::
> > +       The number of interleave ways for the new region's interleave. This
> > +       should be equal to the number of memdevs specified in --memdevs, if
> > +       --memdevs is being supplied. If --memdevs is not specified, an
> > +       appropriate number of memdevs will be chosen based on the number of
> > +       ways specified.
> 
> The reverse is also true, right? That if -w is not specified then the
> number of ways is determined by the number of targets specified, or by
> other default target searches. I guess notes about those enhanced
> default modes can wait until more 'create-region' porcelain arrives.

Hm actually in the current state, /only/ the reverse is true, so this
description was certainly a bit forward looking. I'll fix to say what
it actually does today.

> 
> > +
> > +-g::
> > +--granularity=::
> > +       The interleave granularity for the new region. Must match the selected
> > +       root decoder's (if provided) granularity.
> 
> This just has me thinking that the kernel needs to up-level its code
> comments and changelogs on the "why" for this restriction to somewhere
> this man page can reference.
> 
> However second sentence should be:
> 
> "If the root decoder is interleaved across more than one host-bridge
> then this value must match that granularity. Otherwise, for
> non-interleaved decode windows, any granularity can be
> specified as long as all devices support that setting."
> 
> As I type that it raises 2 questions:
> 
> 1/ If someone does "cxl create-region -g 1024" with no other arguments,
> will it fallback to a decoder that can support that setting if its first
> choice can not?

Well there's no automatic root decoder selection at all in this series,
but for future porcelain, I'd imagine it should try to match exactly
any args that were specified, and fail if /all/ of those can't be
matched.

e.g.:
decoder1 - 2-way - IG:256, and
decoder2 - 1-way - IG:1024

and we get 'cxl create-region -g 1024', we should pick decoder 2,
create a x1 interleave under it. Does that make sense?

> 
> 2/ Per Dave's recent series [1], cxl-cli is missing any consideration
> that a given endpoint may not have the support for the interleave
> address bits that are necessary for a given 'create-region' request.
> Does not need to be solved now, but should be queued up next.
> 
> 2a/ Same for interleave-ways, there are endpoint capabilities that need
> to be accounted.

Yep agreed.

> 
> [1]: https://lore.kernel.org/linux-cxl/165999244272.493131.1975513183227389633.stgit@djiang5-desk4.jf.intel.com/
> 
[..]
> > 
> > +
> > +
> > +static int parse_create_options(int argc, const char **argv,
> > +                               struct parsed_params *p)
> > +{
> > +       int i;
> > +
> > +       if (!param.root_decoder) {
> > +               log_err(&rl, "no root decoder specified\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (param.type) {
> > +               if (strcmp(param.type, "ram") == 0)
> > +                       p->mode = CXL_DECODER_MODE_RAM;
> > +               else if (strcmp(param.type, "volatile") == 0)
> > +                       p->mode = CXL_DECODER_MODE_RAM;
> > +               else if (strcmp(param.type, "pmem") == 0)
> > +                       p->mode = CXL_DECODER_MODE_PMEM;
> > +               else {
> > +                       log_err(&rl, "unsupported type: %s\n", param.type);
> > +                       return -EINVAL;
> 
> This probably wants a common helper that can be shared with
> __reserve_dpa() and add_cxl_decoder() that do the same conversion. I.e.
> cxl_decoder_mode_name() needs a buddy. That can be a follow on change.
> ISTR I might have already noted that, so forgive the duplicate comment.

Ah I do remember this, I probably just missed this cleanup - I'll add
it for v3.

> 
> > +               }
> > +       } else
> > +               p->mode = CXL_DECODER_MODE_PMEM;
> > +
> > +       if (param.size) {
> > +               p->size = parse_size64(param.size);
> > +               if (p->size == ULLONG_MAX) {
> > +                       log_err(&rl, "Invalid size: %s\n", param.size);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       if (param.ways) {
> > +               unsigned long ways = strtoul(param.ways, NULL, 0);
> > +
> > +               if (ways == ULONG_MAX || (int)ways <= 0) {
> > +                       log_err(&rl, "Invalid interleave ways: %s\n",
> > +                               param.ways);
> > +                       return -EINVAL;
> > +               }
> > +               p->ways = ways;
> > +       } else if (argc) {
> > +               p->ways = argc;
> 
> This is where:
> 
>     cxl create-region -p $port -m all
> 
> ...would not work, but maybe requiring explicit targets is ok. There's
> so many potential moving pieces in a CXL topology maybe we do not want
> to go there with this flexibility.

Hm yeah - true. It was certainly convenient (ab)using argc and argv for
this, but if we did add the flexibility of specifying a filter, or even
multiple filters in the future, the targets array would need proper
reconstruction anyway, and counting the objects in it would come
alongwith it.

> 
> > 
[..]
> > +
> > +static int cxl_region_determine_granularity(struct cxl_region *region,
> > +                                           struct parsed_params *p)
> > +{
> > +       const char *devname = cxl_region_get_devname(region);
> > +       unsigned int granularity, ways;
> > +
> > +       /* Default granularity will be the root decoder's granularity */
> > +       granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
> > +       if (granularity == 0 || granularity == UINT_MAX) {
> > +               log_err(&rl, "%s: unable to determine root decoder granularity\n",
> > +                       devname);
> > +               return -ENXIO;
> > +       }
> > +
> > +       /* If no user-supplied granularity, just use the default */
> > +       if (!p->granularity)
> > +               return granularity;
> > +
> > +       ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> > +       if (ways == 0 || ways == UINT_MAX) {
> > +               log_err(&rl, "%s: unable to determine root decoder ways\n",
> > +                       devname);
> > +               return -ENXIO;
> > +       }
> > +
> > +       /* For ways == 1, any user-supplied granularity is fine */
> 
> ...modulo a future patch that checks the device's interleave capability.
> 
> Rest of the patch looks good. After fixing up the option descriptions
> per above you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks!

Re: [ndctl PATCH v2 10/10] cxl/decoder: add a max_available_extent attribute

[Verma, Vishal L]

On Thu, 2022-08-11 at 13:22 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > Add a max_available_extent attribute to cxl_decoder. In order to aid in
> > its calculation, change the order of regions in the root decoder's list
> > to be sorted by start HPA of the region.
> > 
> > Additionally, emit this attribute in decoder listings, and consult it
> > for available space before creating a new region.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  cxl/lib/private.h  |  1 +
> >  cxl/lib/libcxl.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++-
> >  cxl/libcxl.h       |  3 ++
> >  cxl/json.c         |  8 +++++
> >  cxl/region.c       | 14 +++++++-
> >  cxl/lib/libcxl.sym |  1 +
> >  6 files changed, 111 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index 8619bb1..8705e46 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -104,6 +104,7 @@ struct cxl_decoder {
> >         u64 size;
> >         u64 dpa_resource;
> >         u64 dpa_size;
> > +       u64 max_available_extent;
> >         void *dev_buf;
> >         size_t buf_len;
> >         char *dev_path;
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index b4d7890..3c1a2c3 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -446,6 +446,11 @@ CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
> >         return 0;
> >  }
> >  
> > +static int region_start_cmp(struct cxl_region *r1, struct cxl_region *r2)
> > +{
> > +       return ((r1->start < r2->start) ? -1 : 1);
> 
> I think you want 'equal' case too, right?
> 
> val = r1->start - r2->start;
> if (val > r1->start)
>         return -1;
> if (val < r1->start)
>         return 1;
> return 0;

I didn't add it because list_add_sorted didn't seem to do anything with
the equal case, but yeah I think for a compare helper, it makes sense
to be complete.

> 
> > +}
> > +
> >  static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
> >  {
> >         const char *devname = devpath_to_devname(cxlregion_base);
> > @@ -528,7 +533,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
> >                         return region_dup;
> >                 }
> >  
> > -       list_add(&decoder->regions, &region->list);
> > +       list_add_sorted(&decoder->regions, region, list, region_start_cmp);
> >  
> >         return region;
> >  err:
> > @@ -1606,6 +1611,74 @@ cxl_endpoint_get_memdev(struct cxl_endpoint *endpoint)
> >         return NULL;
> >  }
> >  
> > +static int cxl_region_is_configured(struct cxl_region *region)
> 
> s/int/bool/
> 
> > +{
> > +       if ((region->start == 0) && (region->size == 0) &&
> > +           (region->decode_state == CXL_DECODE_RESET))
> > +               return 0;
> > +       return 1;
> 
> That can be squished to just:
> 
>         return region->size && region->decode_state != CXL_DECODE_RESET;
> 
> ...becase region->start == 0 is a valid state for a configured region.
> 
> 
> > +}
> > +
> > +/**
> > + * cxl_decoder_calc_max_available_extent() - calculate max available free space
> > + * @decoder - the root decoder to calculate the free extents for
> > + *
> > + * The add_cxl_region() function  adds regions to the parent decoder's list
> > + * sorted by the region's start HPAs. It can also be assumed that regions have
> > + * no overlapped / aliased HPA space. Therefore, calculating each extent is as
> > + * simple as walking the region list in order, and subtracting the previous
> > + * region's end HPA from the next region's start HPA (and taking into account
> > + * the decoder's start and end HPAs as well).
> > + */
> > +static unsigned long long
> > +cxl_decoder_calc_max_available_extent(struct cxl_decoder *decoder)
> > +{
> > +       struct cxl_port *port = cxl_decoder_get_port(decoder);
> > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > +       u64 prev_end = 0, max_extent = 0;
> > +       struct cxl_region *region;
> > +
> > +       if (!cxl_port_is_root(port)) {
> > +               err(ctx, "%s: not a root decoder\n",
> > +                   cxl_decoder_get_devname(decoder));
> > +               return ULLONG_MAX;
> > +       }
> > +
> > +       /*
> > +        * Preload prev_end with decoder's start, so that the extent
> > +        * calculation for the first region Just Works
> > +        */
> > +       prev_end = decoder->start;
> > +
> > +       cxl_region_foreach(decoder, region) {
> > +               if (!cxl_region_is_configured(region))
> > +                       continue;
> > +
> > +               /*
> > +                * Note: Normally, end = (start + size - 1), but since
> > +                * this is calculating extents between regions, it can
> > +                * skip the '- 1'. For example, if a region ends at 0xfff,
> > +                * and the next region immediately starts at 0x1000,
> > +                * the 'extent' between them is 0, not 1. With
> > +                * end = (start + size), this new 'adjusted' end for the
> > +                * first region will have been calculated as 0x1000.
> > +                * Subtracting the next region's start (0x1000) from this
> > +                * correctly gets the extent size as 0.
> > +                */
> 
> Not sure if I prefer this block comment, or just seeing prev_end being
> done with -1 math and max_extent doing the + 1 because it's a size...
> 
> The latter seems more idiomatic to my eyes, but I'll leave it up to you.

Yep I got greedy with the trickery, but keeping it idiomatic makes more
sense.

> 
> > +               max_extent = max(max_extent, region->start - prev_end);
> > +               prev_end = region->start + region->size;
> > +       }
> > +
> > +       /*
> > +        * Finally, consider the extent after the last region, up to the end
> > +        * of the decoder's address space, if any. If there were no regions,
> > +        * this simply reduces to decoder->size.
> > +        */
> > +       max_extent = max(max_extent, decoder->start + decoder->size - prev_end);
> > +
> > +       return max_extent;
> > +}
> > +
> >  static int decoder_id_cmp(struct cxl_decoder *d1, struct cxl_decoder *d2)
> >  {
> >         return d1->id - d2->id;
> > @@ -1735,6 +1808,8 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
> >                         if (sysfs_read_attr(ctx, path, buf) == 0)
> >                                 *(flag->flag) = !!strtoul(buf, NULL, 0);
> >                 }
> > +               decoder->max_available_extent =
> > +                       cxl_decoder_calc_max_available_extent(decoder);
> >                 break;
> >         }
> >         }
> > @@ -1899,6 +1974,12 @@ cxl_decoder_get_dpa_size(struct cxl_decoder *decoder)
> >         return decoder->dpa_size;
> >  }
> >  
> > +CXL_EXPORT unsigned long long
> > +cxl_decoder_get_max_available_extent(struct cxl_decoder *decoder)
> > +{
> > +       return decoder->max_available_extent;
> > +}
> > +
> >  CXL_EXPORT int cxl_decoder_set_dpa_size(struct cxl_decoder *decoder,
> >                                         unsigned long long size)
> >  {
> > @@ -2053,6 +2134,9 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> >                 return NULL;
> >         }
> >  
> > +       /* Force a re-init of regions so that the new one can be discovered */
> > +       free_regions(decoder);
> 
> You do not need to free them, the re-init will do dup detection and
> *should* just result in the new one being added. In fact, you probably
> do not *want* to free them as that could cause problems for library
> users that were holding a 'struct cxl_region' object before the call to
> cxl_decoder_create_pmem_region().
> 
> With the above fixes folded in you can add:

Yep all other comments sound good.

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

Thanks!

Re: [ndctl PATCH v2 06/10] cxl: add a 'create-region' command

[Dan Williams]

Verma, Vishal L wrote:
[..]
> > > +-m::
> > > +--memdevs::
> > > +       Indicate that the non-option arguments for 'target(s)' refer to memdev
> > > +       names.
> > 
> > Are they names or filter parameters ala 'cxl list -m'? I.e. do you
> > foresee being able to do something like:
> 
> Hm, in the current implementation they really are just names - I just
> use the remaining argv[] as the targets array, but..
> 
> > 
> > "cxl create-region -p $port -m all"
> > 
> > ...to just select all the memdevs that are descendants of $port in the
> > future? More of a curiosity about future possibilities then a request
> > for change.
> 
> This sounds like a useful thing to do - perhaps with the next bit of
> porcelain additions.

Sure.

> 
> > 
> > > +
> > > +-e::
> > > +--ep-decoders::
> > > +       Indicate that the non-option arguments for 'target(s)' refer to endpoint
> > > +       decoder names.
> > 
> > I wonder if this should have a note about it being for test-only
> > purposes? Given the strict CXL decoder allocation rules I wonder if
> > anyone can use this in practice? There might be some synergy with 'cxl
> > reserve-dpa' where this option could be used to say "do not allocate new
> > decoders, and do not reserve more DPA, just try to use the DPA that was
> > already reserved in the following decoders".
> > 
> > We might even delete this option for now unless I am missing the marquee
> > use case for it? Because unless someone knows what they are doing they
> > are almost always going to be wrong.
> 
> Yeah I agree - it is definitely not straightforward to use, and I don't
> see a practical use case. I think I only had it because the ABI wanted
> decoders, and very early implementations had me explicitly asking for
> /everything/.
> 
> I'm okay adding a note that this shouldn't normally be used, or
> removing it entirely.

Hey, if there's a choice, you can never go wrong with red-diff.

> 
> > 
> > > +
> > > +-s::
> > > +--size=::
> > > +       Specify the total size for the new region. This is optional, and by
> > > +       default, the maximum possible size will be used.
> > 
> > How about add:
> > 
> > "The maximum possible size is gated by both the contiguous free HPA
> > space remaining in the root decoder, and the available DPA space in the
> > component memdevs."
> 
> Yep.
> 
> > 
> > > +
> > > +-t::
> > > +--type=::
> > > +       Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> > > +
> > > +-U::
> > > +--uuid=::
> > > +       Specify a UUID for the new region. This shouldn't usually need to be
> > > +       specified, as one will be generated by default.
> > > +
> > > +-w::
> > > +--ways=::
> > > +       The number of interleave ways for the new region's interleave. This
> > > +       should be equal to the number of memdevs specified in --memdevs, if
> > > +       --memdevs is being supplied. If --memdevs is not specified, an
> > > +       appropriate number of memdevs will be chosen based on the number of
> > > +       ways specified.
> > 
> > The reverse is also true, right? That if -w is not specified then the
> > number of ways is determined by the number of targets specified, or by
> > other default target searches. I guess notes about those enhanced
> > default modes can wait until more 'create-region' porcelain arrives.
> 
> Hm actually in the current state, /only/ the reverse is true, so this
> description was certainly a bit forward looking. I'll fix to say what
> it actually does today.

Cool.

> 
> > 
> > > +
> > > +-g::
> > > +--granularity=::
> > > +       The interleave granularity for the new region. Must match the selected
> > > +       root decoder's (if provided) granularity.
> > 
> > This just has me thinking that the kernel needs to up-level its code
> > comments and changelogs on the "why" for this restriction to somewhere
> > this man page can reference.
> > 
> > However second sentence should be:
> > 
> > "If the root decoder is interleaved across more than one host-bridge
> > then this value must match that granularity. Otherwise, for
> > non-interleaved decode windows, any granularity can be
> > specified as long as all devices support that setting."
> > 
> > As I type that it raises 2 questions:
> > 
> > 1/ If someone does "cxl create-region -g 1024" with no other arguments,
> > will it fallback to a decoder that can support that setting if its first
> > choice can not?
> 
> Well there's no automatic root decoder selection at all in this series,
> but for future porcelain, I'd imagine it should try to match exactly
> any args that were specified, and fail if /all/ of those can't be
> matched.
> 
> e.g.:
> decoder1 - 2-way - IG:256, and
> decoder2 - 1-way - IG:1024
> 
> and we get 'cxl create-region -g 1024', we should pick decoder 2,
> create a x1 interleave under it. Does that make sense?

Yup.

[..]
> > > +       } else if (argc) {
> > > +               p->ways = argc;
> > 
> > This is where:
> > 
> >     cxl create-region -p $port -m all
> > 
> > ...would not work, but maybe requiring explicit targets is ok. There's
> > so many potential moving pieces in a CXL topology maybe we do not want
> > to go there with this flexibility.
> 
> Hm yeah - true. It was certainly convenient (ab)using argc and argv for
> this, but if we did add the flexibility of specifying a filter, or even
> multiple filters in the future, the targets array would need proper
> reconstruction anyway, and counting the objects in it would come
> alongwith it.

Agree.