[PATCH ndctl 0/7] cxl: add support for listing and creating volatile regions

[PATCH ndctl 0/7] cxl: add support for listing and creating volatile regions

[Vishal Verma]

While enumeration of ram type regions already works in libcxl and
cxl-cli, it lacked an attribute to indicate pmem vs. ram. Add a new
'type' attribute to region listings to address this. Additionally, add
support for creating ram regions to the cxl-create-region command. The
region listings are also updated with dax-region information for
volatile regions.

This also includes fixed for a few bugs / usability issues identified
along the way - patches 1, 4, and 6. Patch 5 is a usability improvement
where based on decoder capabilities, the type of a region can be
inferred for the create-region command.

These have been tested against the ram-region additions to cxl_test
which are part of the kernel support patch set[1].
Additionally, tested against qemu using a WIP branch for volatile
support found here[2]. The 'run_qemu' script has a branch that creates
volatile memdevs in addition to pmem ones. This is also in a branch[3]
since it depends on [2].

These cxl-cli / libcxl patches themselves are also available in a
branch at [4].

[1]: https://lore.kernel.org/linux-cxl/167564534874.847146.5222419648551436750.stgit@dwillia2-xfh.jf.intel.com/
[2]: https://gitlab.com/jic23/qemu/-/commits/cxl-2023-01-26
[3]: https://github.com/pmem/run_qemu/commits/vv/ram-memdevs
[4]: https://github.com/pmem/ndctl/tree/vv/volatile-regions

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
Dan Williams (2):
      cxl/list: Include regions in the verbose listing
      cxl/list: Enumerate device-dax properties for regions

Vishal Verma (5):
      cxl/region: skip region_actions for region creation
      cxl: add a type attribute to region listings
      cxl: add core plumbing for creation of ram regions
      cxl/region: accept user-supplied UUIDs for pmem regions
      cxl/region: determine region type based on root decoder capability

 Documentation/cxl/cxl-create-region.txt |  6 ++-
 Documentation/cxl/cxl-list.txt          | 31 +++++++++++++
 Documentation/cxl/lib/libcxl.txt        |  8 ++++
 cxl/lib/private.h                       |  2 +
 cxl/lib/libcxl.c                        | 72 ++++++++++++++++++++++++++++--
 cxl/filter.h                            |  3 ++
 cxl/libcxl.h                            |  3 ++
 cxl/filter.c                            |  1 +
 cxl/json.c                              | 21 +++++++++
 cxl/list.c                              |  3 ++
 cxl/region.c                            | 79 ++++++++++++++++++++++++++++++---
 cxl/lib/libcxl.sym                      |  7 +++
 cxl/lib/meson.build                     |  1 +
 cxl/meson.build                         |  3 ++
 14 files changed, 229 insertions(+), 11 deletions(-)
---
base-commit: 08720628d2ba469e203a18c0b1ffbd90f4bfab1d
change-id: 20230120-vv-volatile-regions-063950cef590

Best regards,
-- 
Vishal Verma <vishal.l.verma@intel.com>

[PATCH ndctl 1/7] cxl/region: skip region_actions for region creation

[Vishal Verma]

Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
removed the early return for create-region, and this caused a
create-region operation to unnecessarily loop through buses and root
decoders only to EINVAL out because ACTION_CREATE is handled outside of
the other actions. This results in confising messages such as:

  # cxl create-region -t ram -d 0.0 -m 0,4
  {
    "region":"region7",
    "resource":"0xf030000000",
    "size":"512.00 MiB (536.87 MB)",
    ...
  }
  cxl region: decoder_region_action: region0: failed: Invalid argument
  cxl region: region_action: one or more failures, last failure: Invalid argument
  cxl region: cmd_create_region: created 1 region

Since there's no need to walk through the topology after creating a
region, and especially not to perform an invalid 'action', switch
back to retuening early for create-region.

Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/region.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cxl/region.c b/cxl/region.c
index efe05aa..38aa142 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 		return rc;
 
 	if (action == ACTION_CREATE)
-		rc = create_region(ctx, count, p);
+		return create_region(ctx, count, p);
 
 	cxl_bus_foreach(ctx, bus) {
 		struct cxl_decoder *decoder;

-- 
2.39.1

[PATCH ndctl 2/7] cxl: add a type attribute to region listings

[Vishal Verma]

In preparation for enumerating and creating 'volatile' or 'ram' type
regions, add a 'type' attribute to region listings, so these can be
distinguished from 'pmem' type regions easily. This depends on a new
'mode' attribute for region objects in sysfs. For older kernels that
lack this, region listings will simply omit emitting this attribute,
but otherwise not treat it as a failure.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/lib/libcxl.txt |  1 +
 cxl/lib/private.h                |  1 +
 cxl/lib/libcxl.c                 | 11 +++++++++++
 cxl/libcxl.h                     |  1 +
 cxl/json.c                       |  5 +++++
 cxl/lib/libcxl.sym               |  5 +++++
 6 files changed, 24 insertions(+)

diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index f9af376..dbc4b56 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -550,6 +550,7 @@ 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);
+enum cxl_decoder_mode cxl_region_get_mode(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);
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index f8871bd..306dc3a 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -149,6 +149,7 @@ struct cxl_region {
 	unsigned int interleave_ways;
 	unsigned int interleave_granularity;
 	enum cxl_decode_state decode_state;
+	enum cxl_decoder_mode mode;
 	struct kmod_module *module;
 	struct list_head mappings;
 };
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 4205a58..83f628b 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -561,6 +561,12 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
 	else
 		region->decode_state = strtoul(buf, NULL, 0);
 
+	sprintf(path, "%s/mode", cxlregion_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		region->mode = CXL_DECODER_MODE_NONE;
+	else
+		region->mode = cxl_decoder_mode_from_ident(buf);
+
 	sprintf(path, "%s/modalias", cxlregion_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		region->module = util_modalias_to_module(ctx, buf);
@@ -686,6 +692,11 @@ CXL_EXPORT unsigned long long cxl_region_get_resource(struct cxl_region *region)
 	return region->start;
 }
 
+CXL_EXPORT enum cxl_decoder_mode cxl_region_get_mode(struct cxl_region *region)
+{
+	return region->mode;
+}
+
 CXL_EXPORT unsigned int
 cxl_region_get_interleave_ways(struct cxl_region *region)
 {
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index d699af8..e6cca11 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -273,6 +273,7 @@ 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);
+enum cxl_decoder_mode cxl_region_get_mode(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,
diff --git a/cxl/json.c b/cxl/json.c
index 0fc44e4..f625380 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -827,6 +827,7 @@ void util_cxl_mappings_append_json(struct json_object *jregion,
 struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 					     unsigned long flags)
 {
+	enum cxl_decoder_mode mode = cxl_region_get_mode(region);
 	const char *devname = cxl_region_get_devname(region);
 	struct json_object *jregion, *jobj;
 	u64 val;
@@ -853,6 +854,10 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 			json_object_object_add(jregion, "size", jobj);
 	}
 
+	jobj = json_object_new_string(cxl_decoder_mode_name(mode));
+	if (jobj)
+		json_object_object_add(jregion, "type", jobj);
+
 	val = cxl_region_get_interleave_ways(region);
 	if (val < INT_MAX) {
 		jobj = json_object_new_int(val);
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 6bc0810..9832d09 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -242,3 +242,8 @@ global:
 	cxl_target_get_firmware_node;
 	cxl_dport_get_firmware_node;
 } LIBCXL_3;
+
+LIBCXL_5 {
+global:
+	cxl_region_get_mode;
+} LIBCXL_4;

-- 
2.39.1

[PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Vishal Verma]

Add support in libcxl to create ram regions through a new
cxl_decoder_create_ram_region() API, which works similarly to its pmem
sibling.

Enable ram region creation in cxl-cli, with the only differences from
the pmem flow being:
  1/ Use the above create_ram_region API, and
  2/ Elide setting the UUID, since ram regions don't have one

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-create-region.txt |  3 ++-
 cxl/lib/libcxl.c                        | 22 +++++++++++++++++++---
 cxl/libcxl.h                            |  1 +
 cxl/region.c                            | 32 ++++++++++++++++++++++++++++----
 cxl/lib/libcxl.sym                      |  1 +
 5 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
index 286779e..ada0e52 100644
--- a/Documentation/cxl/cxl-create-region.txt
+++ b/Documentation/cxl/cxl-create-region.txt
@@ -80,7 +80,8 @@ include::bus-option.txt[]
 -U::
 --uuid=::
 	Specify a UUID for the new region. This shouldn't usually need to be
-	specified, as one will be generated by default.
+	specified, as one will be generated by default. Only applicable to
+	pmem regions.
 
 -w::
 --ways=::
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 83f628b..c5b9b18 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -2234,8 +2234,8 @@ cxl_decoder_get_region(struct cxl_decoder *decoder)
 	return NULL;
 }
 
-CXL_EXPORT struct cxl_region *
-cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
+static struct cxl_region *cxl_decoder_create_region(struct cxl_decoder *decoder,
+						    enum cxl_decoder_mode mode)
 {
 	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
 	char *path = decoder->dev_buf;
@@ -2243,7 +2243,11 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
 	struct cxl_region *region;
 	int rc;
 
-	sprintf(path, "%s/create_pmem_region", decoder->dev_path);
+	if (mode == CXL_DECODER_MODE_PMEM)
+		sprintf(path, "%s/create_pmem_region", decoder->dev_path);
+	else if (mode == CXL_DECODER_MODE_RAM)
+		sprintf(path, "%s/create_ram_region", decoder->dev_path);
+
 	rc = sysfs_read_attr(ctx, path, buf);
 	if (rc < 0) {
 		err(ctx, "failed to read new region name: %s\n",
@@ -2282,6 +2286,18 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
 	return region;
 }
 
+CXL_EXPORT struct cxl_region *
+cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
+{
+	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_PMEM);
+}
+
+CXL_EXPORT struct cxl_region *
+cxl_decoder_create_ram_region(struct cxl_decoder *decoder)
+{
+	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_RAM);
+}
+
 CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
 {
 	return decoder->nr_targets;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index e6cca11..904156c 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -213,6 +213,7 @@ 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_get_region(struct cxl_decoder *decoder);
 struct cxl_region *cxl_decoder_create_pmem_region(struct cxl_decoder *decoder);
+struct cxl_region *cxl_decoder_create_ram_region(struct cxl_decoder *decoder);
 struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
 					    const char *ident);
 struct cxl_memdev *cxl_decoder_get_memdev(struct cxl_decoder *decoder);
diff --git a/cxl/region.c b/cxl/region.c
index 38aa142..0945a14 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
 		struct json_object *jobj =
 			json_object_array_get_idx(p->memdevs, i);
 		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
-		u64 size = cxl_memdev_get_pmem_size(memdev);
+		u64 size;
+
+		switch(p->mode) {
+		case CXL_DECODER_MODE_RAM:
+			size = cxl_memdev_get_ram_size(memdev);
+			break;
+		case CXL_DECODER_MODE_PMEM:
+			size = cxl_memdev_get_pmem_size(memdev);
+			break;
+		default:
+			/*
+			 * This will 'poison' ep_min_size with a 0, and
+			 * subsequently cause the region creation to fail.
+			 */
+			size = 0;
+		}
 
 		if (!p->ep_min_size)
 			p->ep_min_size = size;
@@ -589,8 +604,15 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 				param.root_decoder);
 			return -ENXIO;
 		}
+	} else if (p->mode == CXL_DECODER_MODE_RAM) {
+		region = cxl_decoder_create_ram_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",
+		log_err(&rl, "region type '%s' is not supported\n",
 			param.type);
 		return -EOPNOTSUPP;
 	}
@@ -602,10 +624,12 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 		goto out;
 	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);
+	if (p->mode == CXL_DECODER_MODE_PMEM) {
+		uuid_generate(uuid);
+		try(cxl_region, set_uuid, region, uuid);
+	}
 	try(cxl_region, set_size, region, size);
 
 	for (i = 0; i < p->ways; i++) {
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 9832d09..84f60ad 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -246,4 +246,5 @@ global:
 LIBCXL_5 {
 global:
 	cxl_region_get_mode;
+	cxl_decoder_create_ram_region;
 } LIBCXL_4;

-- 
2.39.1

[PATCH ndctl 4/7] cxl/region: accept user-supplied UUIDs for pmem regions

[Vishal Verma]

Attempting to add additional checking around user-supplied UUIDs against
'ram' type regions revealed that commit 21b089025178 ("cxl: add a 'create-region' command")
completely neglected to add the requisite support for accepting
user-supplied UUIDs, even though the man page for cxl-create-region
advertised the option.

Fix this by actually adding this option now, and add checks to validate
the user-supplied UUID, and refuse it for ram regions.

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

diff --git a/cxl/region.c b/cxl/region.c
index 0945a14..9079b2d 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -22,6 +22,7 @@ static struct region_params {
 	const char *bus;
 	const char *size;
 	const char *type;
+	const char *uuid;
 	const char *root_decoder;
 	const char *region;
 	int ways;
@@ -40,6 +41,7 @@ struct parsed_params {
 	u64 ep_min_size;
 	int ways;
 	int granularity;
+	uuid_t uuid;
 	struct json_object *memdevs;
 	int num_memdevs;
 	int argc;
@@ -74,6 +76,8 @@ OPT_INTEGER('g', "granularity", &param.granularity,  \
 	    "granularity of the interleave set"), \
 OPT_STRING('t', "type", &param.type, \
 	   "region type", "region type - 'pmem' or 'ram'"), \
+OPT_STRING('U', "uuid", &param.uuid, \
+	   "region uuid", "uuid for the new region (default: autogenerate)"), \
 OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
 	    "non-option arguments are memdevs"), \
 OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats")
@@ -293,6 +297,11 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
 
 	if (param.type) {
 		p->mode = cxl_decoder_mode_from_ident(param.type);
+		if (p->mode == CXL_DECODER_MODE_RAM && param.uuid) {
+			log_err(&rl,
+				"can't set UUID for ram / volatile regions");
+			return -EINVAL;
+		}
 		if (p->mode == CXL_DECODER_MODE_NONE) {
 			log_err(&rl, "unsupported type: %s\n", param.type);
 			return -EINVAL;
@@ -341,6 +350,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
 		}
 	}
 
+	if (param.uuid) {
+		if (uuid_parse(param.uuid, p->uuid)) {
+			error("failed to parse uuid: '%s'\n", param.uuid);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
@@ -566,7 +582,6 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 	int i, rc, granularity;
 	u64 size, max_extent;
 	const char *devname;
-	uuid_t uuid;
 
 	rc = create_region_validate_config(ctx, p);
 	if (rc)
@@ -627,8 +642,9 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 	try(cxl_region, set_interleave_granularity, region, granularity);
 	try(cxl_region, set_interleave_ways, region, p->ways);
 	if (p->mode == CXL_DECODER_MODE_PMEM) {
-		uuid_generate(uuid);
-		try(cxl_region, set_uuid, region, uuid);
+		if (!param.uuid)
+			uuid_generate(p->uuid);
+		try(cxl_region, set_uuid, region, p->uuid);
 	}
 	try(cxl_region, set_size, region, size);
 

-- 
2.39.1

[PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Vishal Verma]

In the common case, root decoders are expected to be either pmem
capable, or volatile capable, but not necessarily both simultaneously.
If a decoder only has one of pmem or volatile capabilities,
cxl-create-region should just infer the type of the region (pmem
or ram) based on this capability.

Maintain the default behavior of cxl-create-region to choose type=pmem,
but only as a fallback if the selected root decoder has multiple
capabilities. If it is only capable of either pmem, or ram, then infer
region type from this without requiring it to be specified explicitly.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-create-region.txt |  3 ++-
 cxl/region.c                            | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
index ada0e52..f11a412 100644
--- a/Documentation/cxl/cxl-create-region.txt
+++ b/Documentation/cxl/cxl-create-region.txt
@@ -75,7 +75,8 @@ include::bus-option.txt[]
 
 -t::
 --type=::
-	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
+	Specify the region type - 'pmem' or 'ram'. Default to root decoder
+	capability, and if that is ambiguous, default to 'pmem'.
 
 -U::
 --uuid=::
diff --git a/cxl/region.c b/cxl/region.c
index 9079b2d..1c8ccc7 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
 	return 0;
 }
 
+static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
+{
+	int num_cap = 0;
+
+	/* if param.type was explicitly specified, nothing to do here */
+	if (param.type)
+		return;
+
+	/*
+	 * if the root decoder only has one type of capability, default
+	 * to that mode for the region.
+	 */
+	if (cxl_decoder_is_pmem_capable(p->root_decoder))
+		num_cap++;
+	if (cxl_decoder_is_volatile_capable(p->root_decoder))
+		num_cap++;
+
+	if (num_cap == 1) {
+		if (cxl_decoder_is_volatile_capable(p->root_decoder))
+			p->mode = CXL_DECODER_MODE_RAM;
+		else if (cxl_decoder_is_pmem_capable(p->root_decoder))
+			p->mode = CXL_DECODER_MODE_PMEM;
+	}
+}
+
 static int create_region_validate_config(struct cxl_ctx *ctx,
 					 struct parsed_params *p)
 {
@@ -481,6 +506,8 @@ found:
 		return -ENXIO;
 	}
 
+	set_type_from_decoder(ctx, p);
+
 	rc = validate_decoder(p->root_decoder, p);
 	if (rc)
 		return rc;

-- 
2.39.1

[PATCH ndctl 6/7] cxl/list: Include regions in the verbose listing

[Vishal Verma]

From: Dan Williams <dan.j.williams@intel.com>

When verbose listing was added, region listing support was not available, so
it got missed. Add it now.

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

diff --git a/cxl/list.c b/cxl/list.c
index e3ef1fb..4e77aeb 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -126,6 +126,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.endpoints = true;
 		param.decoders = true;
 		param.targets = true;
+		param.regions = true;
 		/*fallthrough*/
 	case 0:
 		break;

-- 
2.39.1

[PATCH ndctl 7/7] cxl/list: Enumerate device-dax properties for regions

[Vishal Verma]

From: Dan Williams <dan.j.williams@intel.com>

Recently the kernel added support for routing newly mapped CXL regions to
device-dax. Include the json representation of a DAX region beneath its
host CXL region.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[vishal: fix missing dsxctl/json.h include in cxl/json.c]
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-list.txt   | 31 +++++++++++++++++++++++++++++++
 Documentation/cxl/lib/libcxl.txt |  7 +++++++
 cxl/lib/private.h                |  1 +
 cxl/lib/libcxl.c                 | 39 +++++++++++++++++++++++++++++++++++++++
 cxl/filter.h                     |  3 +++
 cxl/libcxl.h                     |  1 +
 cxl/filter.c                     |  1 +
 cxl/json.c                       | 16 ++++++++++++++++
 cxl/list.c                       |  2 ++
 cxl/lib/libcxl.sym               |  1 +
 cxl/lib/meson.build              |  1 +
 cxl/meson.build                  |  3 +++
 12 files changed, 106 insertions(+)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index 3410d49..c64d65d 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -380,6 +380,37 @@ OPTIONS
 --regions::
 	Include region objects in the listing.
 
+-X::
+--dax::
+	Append DAX information to region listings
+----
+# cxl list -RXu
+{
+  "region":"region4",
+  "resource":"0xf010000000",
+  "size":"512.00 MiB (536.87 MB)",
+  "interleave_ways":2,
+  "interleave_granularity":4096,
+  "decode_state":"commit",
+  "daxregion":{
+    "id":4,
+    "size":"512.00 MiB (536.87 MB)",
+    "align":2097152,
+    "devices":[
+      {
+        "chardev":"dax4.0",
+        "size":"512.00 MiB (536.87 MB)",
+        "target_node":0,
+        "align":2097152,
+        "mode":"system-ram",
+        "online_memblocks":0,
+        "total_memblocks":4
+      }
+    ]
+  }
+}
+----
+
 -r::
 --region::
 	Specify CXL region device name(s), or device id(s), to filter the listing.
diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
index dbc4b56..31bc855 100644
--- a/Documentation/cxl/lib/libcxl.txt
+++ b/Documentation/cxl/lib/libcxl.txt
@@ -568,6 +568,7 @@ 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);
+struct daxctl_region *cxl_region_get_daxctl_region(struct cxl_region *region);
 ----
 
 A region's resource attribute is the Host Physical Address at which the region's
@@ -587,6 +588,12 @@ 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.
 
+Once a region is active it is attached to either the NVDIMM subsystem
+where its properties can be interrogated by ndctl, or the DAX subsystem
+where its properties can be interrogated by daxctl. The helper
+cxl_region_get_daxctl_region() returns an 'struct daxctl_region *' that
+can be used with other libdaxctl APIs.
+
 include::../../copyright.txt[]
 
 SEE ALSO
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 306dc3a..d648992 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -150,6 +150,7 @@ struct cxl_region {
 	unsigned int interleave_granularity;
 	enum cxl_decode_state decode_state;
 	enum cxl_decoder_mode mode;
+	struct daxctl_region *dax_region;
 	struct kmod_module *module;
 	struct list_head mappings;
 };
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index c5b9b18..81855f4 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -26,6 +26,7 @@
 #include <util/bitmap.h>
 #include <cxl/cxl_mem.h>
 #include <cxl/libcxl.h>
+#include <daxctl/libdaxctl.h>
 #include "private.h"
 
 /**
@@ -49,6 +50,7 @@ struct cxl_ctx {
 	struct list_head memdevs;
 	struct list_head buses;
 	struct kmod_ctx *kmod_ctx;
+	struct daxctl_ctx *daxctl_ctx;
 	void *private_data;
 };
 
@@ -231,6 +233,7 @@ CXL_EXPORT void *cxl_get_private_data(struct cxl_ctx *ctx)
  */
 CXL_EXPORT int cxl_new(struct cxl_ctx **ctx)
 {
+	struct daxctl_ctx *daxctl_ctx;
 	struct udev_queue *udev_queue;
 	struct kmod_ctx *kmod_ctx;
 	struct udev *udev;
@@ -241,6 +244,10 @@ CXL_EXPORT int cxl_new(struct cxl_ctx **ctx)
 	if (!c)
 		return -ENOMEM;
 
+	rc = daxctl_new(&daxctl_ctx);
+	if (rc)
+		goto err_daxctl;
+
 	kmod_ctx = kmod_new(NULL, NULL);
 	if (check_kmod(kmod_ctx) != 0) {
 		rc = -ENXIO;
@@ -267,6 +274,7 @@ CXL_EXPORT int cxl_new(struct cxl_ctx **ctx)
 	list_head_init(&c->memdevs);
 	list_head_init(&c->buses);
 	c->kmod_ctx = kmod_ctx;
+	c->daxctl_ctx = daxctl_ctx;
 	c->udev = udev;
 	c->udev_queue = udev_queue;
 	c->timeout = 5000;
@@ -278,6 +286,8 @@ err_udev_queue:
 err_udev:
 	kmod_unref(kmod_ctx);
 err_kmod:
+	daxctl_unref(daxctl_ctx);
+err_daxctl:
 	free(c);
 	return rc;
 }
@@ -321,6 +331,7 @@ CXL_EXPORT void cxl_unref(struct cxl_ctx *ctx)
 	udev_queue_unref(ctx->udev_queue);
 	udev_unref(ctx->udev);
 	kmod_unref(ctx->kmod_ctx);
+	daxctl_unref(ctx->daxctl_ctx);
 	info(ctx, "context %p released\n", ctx);
 	free(ctx);
 }
@@ -746,6 +757,34 @@ cxl_region_get_target_decoder(struct cxl_region *region, int position)
 	return decoder;
 }
 
+CXL_EXPORT struct daxctl_region *
+cxl_region_get_daxctl_region(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;
+	uuid_t uuid = { 0 };
+	struct stat st;
+
+	if (region->dax_region)
+		return region->dax_region;
+
+	if (snprintf(region->dev_buf, len, "%s/dax_region%d", region->dev_path,
+		     region->id) >= len) {
+		err(ctx, "%s: buffer too small!\n", devname);
+		return NULL;
+	}
+
+	if (stat(path, &st) < 0)
+		return NULL;
+
+	region->dax_region =
+		daxctl_new_region(ctx->daxctl_ctx, region->id, uuid, path);
+
+	return region->dax_region;
+}
+
 CXL_EXPORT int cxl_region_set_size(struct cxl_region *region,
 				   unsigned long long size)
 {
diff --git a/cxl/filter.h b/cxl/filter.h
index b9f1350..c486514 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -28,6 +28,7 @@ struct cxl_filter_params {
 	bool health;
 	bool partition;
 	bool alert_config;
+	bool dax;
 	int verbose;
 	struct log_ctx ctx;
 };
@@ -80,6 +81,8 @@ static inline unsigned long cxl_filter_to_flags(struct cxl_filter_params *param)
 		flags |= UTIL_JSON_PARTITION;
 	if (param->alert_config)
 		flags |= UTIL_JSON_ALERT_CONFIG;
+	if (param->dax)
+		flags |= UTIL_JSON_DAX | UTIL_JSON_DAX_DEVS;
 	return flags;
 }
 
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 904156c..54d9f10 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -279,6 +279,7 @@ 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);
+struct daxctl_region *cxl_region_get_daxctl_region(struct cxl_region *region);
 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,
diff --git a/cxl/filter.c b/cxl/filter.c
index 90b13be..f90cbc8 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -11,6 +11,7 @@
 
 #include "filter.h"
 #include "json.h"
+#include "../daxctl/json.h"
 
 static const char *which_sep(const char *filter)
 {
diff --git a/cxl/json.c b/cxl/json.c
index f625380..6eb5b8f 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -10,6 +10,7 @@
 
 #include "filter.h"
 #include "json.h"
+#include "../daxctl/json.h"
 
 static struct json_object *util_cxl_memdev_health_to_json(
 		struct cxl_memdev *memdev, unsigned long flags)
@@ -889,7 +890,22 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 
 	util_cxl_mappings_append_json(jregion, region, flags);
 
+	if (flags & UTIL_JSON_DAX) {
+		struct daxctl_region *dax_region;
+
+		dax_region = cxl_region_get_daxctl_region(region);
+		if (dax_region) {
+			jobj = util_daxctl_region_to_json(dax_region, NULL,
+							  flags);
+			if (jobj)
+				json_object_object_add(jregion, "daxregion",
+						       jobj);
+		}
+	}
+
 	json_object_set_userdata(jregion, region, NULL);
+
+
 	return jregion;
 }
 
diff --git a/cxl/list.c b/cxl/list.c
index 4e77aeb..c01154e 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -45,6 +45,7 @@ static const struct option options[] = {
 	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('X', "dax", &param.dax, "include CXL DAX region enumeration"),
 	OPT_BOOLEAN('i', "idle", &param.idle, "include disabled devices"),
 	OPT_BOOLEAN('u', "human", &param.human,
 		    "use human friendly number formats"),
@@ -116,6 +117,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.health = true;
 		param.partition = true;
 		param.alert_config = true;
+		param.dax = true;
 		/* fallthrough */
 	case 2:
 		param.idle = true;
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 84f60ad..1c6177c 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -247,4 +247,5 @@ LIBCXL_5 {
 global:
 	cxl_region_get_mode;
 	cxl_decoder_create_ram_region;
+	cxl_region_get_daxctl_region;
 } LIBCXL_4;
diff --git a/cxl/lib/meson.build b/cxl/lib/meson.build
index 60b9de7..422a351 100644
--- a/cxl/lib/meson.build
+++ b/cxl/lib/meson.build
@@ -16,6 +16,7 @@ cxl = library('cxl',
     uuid,
     kmod,
     libudev,
+    daxctl_dep,
   ],
   version : libcxl_version,
   install : true,
diff --git a/cxl/meson.build b/cxl/meson.build
index f2474aa..4ead163 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -7,6 +7,8 @@ cxl_src = [
   'memdev.c',
   'json.c',
   'filter.c',
+  '../daxctl/json.c',
+  '../daxctl/filter.c',
 ]
 
 cxl_tool = executable('cxl',
@@ -14,6 +16,7 @@ cxl_tool = executable('cxl',
   include_directories : root_inc,
   dependencies : [
     cxl_dep,
+    daxctl_dep,
     util_dep,
     uuid,
     kmod,

-- 
2.39.1

Re: [PATCH ndctl 1/7] cxl/region: skip region_actions for region creation

[Fan Ni]

On Tue, Feb 07, 2023 at 12:16:27PM -0700, Vishal Verma wrote:
> Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> removed the early return for create-region, and this caused a
> create-region operation to unnecessarily loop through buses and root
> decoders only to EINVAL out because ACTION_CREATE is handled outside of
> the other actions. This results in confising messages such as:
s/confising/confusing/
> 
>   # cxl create-region -t ram -d 0.0 -m 0,4
>   {
>     "region":"region7",
>     "resource":"0xf030000000",
>     "size":"512.00 MiB (536.87 MB)",
>     ...
>   }
>   cxl region: decoder_region_action: region0: failed: Invalid argument
>   cxl region: region_action: one or more failures, last failure: Invalid argument
>   cxl region: cmd_create_region: created 1 region
> 
> Since there's no need to walk through the topology after creating a
> region, and especially not to perform an invalid 'action', switch
> back to retuening early for create-region.
s/retuening/returning/
> 
> Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index efe05aa..38aa142 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  		return rc;
>  
>  	if (action == ACTION_CREATE)
> -		rc = create_region(ctx, count, p);
> +		return create_region(ctx, count, p);
>  
>  	cxl_bus_foreach(ctx, bus) {
>  		struct cxl_decoder *decoder;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 1/7] cxl/region: skip region_actions for region creation

[Verma, Vishal L]

On Tue, 2023-02-07 at 22:07 +0000, Fan Ni wrote:
> On Tue, Feb 07, 2023 at 12:16:27PM -0700, Vishal Verma wrote:
> > Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> > removed the early return for create-region, and this caused a
> > create-region operation to unnecessarily loop through buses and root
> > decoders only to EINVAL out because ACTION_CREATE is handled outside of
> > the other actions. This results in confising messages such as:
> s/confising/confusing/
> > 
> >   # cxl create-region -t ram -d 0.0 -m 0,4
> >   {
> >     "region":"region7",
> >     "resource":"0xf030000000",
> >     "size":"512.00 MiB (536.87 MB)",
> >     ...
> >   }
> >   cxl region: decoder_region_action: region0: failed: Invalid argument
> >   cxl region: region_action: one or more failures, last failure: Invalid argument
> >   cxl region: cmd_create_region: created 1 region
> > 
> > Since there's no need to walk through the topology after creating a
> > region, and especially not to perform an invalid 'action', switch
> > back to retuening early for create-region.
> s/retuening/returning/

Thanks Fan! Fixed for v2.

> > 
> > Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  cxl/region.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index efe05aa..38aa142 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
> >                 return rc;
> >  
> >         if (action == ACTION_CREATE)
> > -               rc = create_region(ctx, count, p);
> > +               return create_region(ctx, count, p);
> >  
> >         cxl_bus_foreach(ctx, bus) {
> >                 struct cxl_decoder *decoder;
> > 
> > -- 
> > 2.39.1
> > 

Re: [PATCH ndctl 1/7] cxl/region: skip region_actions for region creation

[Ira Weiny]

Vishal Verma wrote:
> Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> removed the early return for create-region, and this caused a
> create-region operation to unnecessarily loop through buses and root
> decoders only to EINVAL out because ACTION_CREATE is handled outside of
> the other actions. This results in confising messages such as:
> 
>   # cxl create-region -t ram -d 0.0 -m 0,4
>   {
>     "region":"region7",
>     "resource":"0xf030000000",
>     "size":"512.00 MiB (536.87 MB)",
>     ...
>   }
>   cxl region: decoder_region_action: region0: failed: Invalid argument
>   cxl region: region_action: one or more failures, last failure: Invalid argument
>   cxl region: cmd_create_region: created 1 region
> 
> Since there's no need to walk through the topology after creating a
> region, and especially not to perform an invalid 'action', switch
> back to retuening early for create-region.
> 
> Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> Cc: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index efe05aa..38aa142 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  		return rc;
>  
>  	if (action == ACTION_CREATE)
> -		rc = create_region(ctx, count, p);
> +		return create_region(ctx, count, p);
>  
>  	cxl_bus_foreach(ctx, bus) {
>  		struct cxl_decoder *decoder;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 2/7] cxl: add a type attribute to region listings

[Ira Weiny]

Vishal Verma wrote:
> In preparation for enumerating and creating 'volatile' or 'ram' type
> regions, add a 'type' attribute to region listings, so these can be
> distinguished from 'pmem' type regions easily. This depends on a new
> 'mode' attribute for region objects in sysfs. For older kernels that
> lack this, region listings will simply omit emitting this attribute,
> but otherwise not treat it as a failure.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/lib/libcxl.txt |  1 +
>  cxl/lib/private.h                |  1 +
>  cxl/lib/libcxl.c                 | 11 +++++++++++
>  cxl/libcxl.h                     |  1 +
>  cxl/json.c                       |  5 +++++
>  cxl/lib/libcxl.sym               |  5 +++++
>  6 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index f9af376..dbc4b56 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -550,6 +550,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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);
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index f8871bd..306dc3a 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -149,6 +149,7 @@ struct cxl_region {
>  	unsigned int interleave_ways;
>  	unsigned int interleave_granularity;
>  	enum cxl_decode_state decode_state;
> +	enum cxl_decoder_mode mode;
>  	struct kmod_module *module;
>  	struct list_head mappings;
>  };
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 4205a58..83f628b 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -561,6 +561,12 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  	else
>  		region->decode_state = strtoul(buf, NULL, 0);
>  
> +	sprintf(path, "%s/mode", cxlregion_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		region->mode = CXL_DECODER_MODE_NONE;
> +	else
> +		region->mode = cxl_decoder_mode_from_ident(buf);
> +
>  	sprintf(path, "%s/modalias", cxlregion_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		region->module = util_modalias_to_module(ctx, buf);
> @@ -686,6 +692,11 @@ CXL_EXPORT unsigned long long cxl_region_get_resource(struct cxl_region *region)
>  	return region->start;
>  }
>  
> +CXL_EXPORT enum cxl_decoder_mode cxl_region_get_mode(struct cxl_region *region)
> +{
> +	return region->mode;
> +}
> +
>  CXL_EXPORT unsigned int
>  cxl_region_get_interleave_ways(struct cxl_region *region)
>  {
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index d699af8..e6cca11 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -273,6 +273,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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,
> diff --git a/cxl/json.c b/cxl/json.c
> index 0fc44e4..f625380 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -827,6 +827,7 @@ void util_cxl_mappings_append_json(struct json_object *jregion,
>  struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  					     unsigned long flags)
>  {
> +	enum cxl_decoder_mode mode = cxl_region_get_mode(region);
>  	const char *devname = cxl_region_get_devname(region);
>  	struct json_object *jregion, *jobj;
>  	u64 val;
> @@ -853,6 +854,10 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  			json_object_object_add(jregion, "size", jobj);
>  	}
>  
> +	jobj = json_object_new_string(cxl_decoder_mode_name(mode));
> +	if (jobj)
> +		json_object_object_add(jregion, "type", jobj);
> +
>  	val = cxl_region_get_interleave_ways(region);
>  	if (val < INT_MAX) {
>  		jobj = json_object_new_int(val);
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 6bc0810..9832d09 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -242,3 +242,8 @@ global:
>  	cxl_target_get_firmware_node;
>  	cxl_dport_get_firmware_node;
>  } LIBCXL_3;
> +
> +LIBCXL_5 {
> +global:
> +	cxl_region_get_mode;
> +} LIBCXL_4;
> 
> -- 
> 2.39.1
> 

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Ira Weiny]

Vishal Verma wrote:
> Add support in libcxl to create ram regions through a new
> cxl_decoder_create_ram_region() API, which works similarly to its pmem
> sibling.
> 
> Enable ram region creation in cxl-cli, with the only differences from
> the pmem flow being:
>   1/ Use the above create_ram_region API, and
>   2/ Elide setting the UUID, since ram regions don't have one
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-create-region.txt |  3 ++-
>  cxl/lib/libcxl.c                        | 22 +++++++++++++++++++---
>  cxl/libcxl.h                            |  1 +
>  cxl/region.c                            | 32 ++++++++++++++++++++++++++++----
>  cxl/lib/libcxl.sym                      |  1 +
>  5 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index 286779e..ada0e52 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -80,7 +80,8 @@ include::bus-option.txt[]
>  -U::
>  --uuid=::
>  	Specify a UUID for the new region. This shouldn't usually need to be
> -	specified, as one will be generated by default.
> +	specified, as one will be generated by default. Only applicable to
> +	pmem regions.
>  
>  -w::
>  --ways=::
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 83f628b..c5b9b18 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -2234,8 +2234,8 @@ cxl_decoder_get_region(struct cxl_decoder *decoder)
>  	return NULL;
>  }
>  
> -CXL_EXPORT struct cxl_region *
> -cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> +static struct cxl_region *cxl_decoder_create_region(struct cxl_decoder *decoder,
> +						    enum cxl_decoder_mode mode)
>  {
>  	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
>  	char *path = decoder->dev_buf;
> @@ -2243,7 +2243,11 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  	struct cxl_region *region;
>  	int rc;
>  
> -	sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> +	if (mode == CXL_DECODER_MODE_PMEM)
> +		sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> +	else if (mode == CXL_DECODER_MODE_RAM)
> +		sprintf(path, "%s/create_ram_region", decoder->dev_path);
> +
>  	rc = sysfs_read_attr(ctx, path, buf);
>  	if (rc < 0) {
>  		err(ctx, "failed to read new region name: %s\n",
> @@ -2282,6 +2286,18 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  	return region;
>  }
>  
> +CXL_EXPORT struct cxl_region *
> +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> +{
> +	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_PMEM);
> +}
> +
> +CXL_EXPORT struct cxl_region *
> +cxl_decoder_create_ram_region(struct cxl_decoder *decoder)
> +{
> +	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_RAM);
> +}
> +
>  CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
>  {
>  	return decoder->nr_targets;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index e6cca11..904156c 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -213,6 +213,7 @@ 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_get_region(struct cxl_decoder *decoder);
>  struct cxl_region *cxl_decoder_create_pmem_region(struct cxl_decoder *decoder);
> +struct cxl_region *cxl_decoder_create_ram_region(struct cxl_decoder *decoder);
>  struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
>  					    const char *ident);
>  struct cxl_memdev *cxl_decoder_get_memdev(struct cxl_decoder *decoder);
> diff --git a/cxl/region.c b/cxl/region.c
> index 38aa142..0945a14 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
>  		struct json_object *jobj =
>  			json_object_array_get_idx(p->memdevs, i);
>  		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> -		u64 size = cxl_memdev_get_pmem_size(memdev);
> +		u64 size;
> +
> +		switch(p->mode) {
> +		case CXL_DECODER_MODE_RAM:
> +			size = cxl_memdev_get_ram_size(memdev);
> +			break;
> +		case CXL_DECODER_MODE_PMEM:
> +			size = cxl_memdev_get_pmem_size(memdev);
> +			break;
> +		default:
> +			/*
> +			 * This will 'poison' ep_min_size with a 0, and
> +			 * subsequently cause the region creation to fail.
> +			 */
> +			size = 0;

Why not change collect_minsize() to return int and propagate the error up
through create_region_validate_config()?

It seems more confusing to hide a special value in size like this.

Ira

> +		}
>  
>  		if (!p->ep_min_size)
>  			p->ep_min_size = size;
> @@ -589,8 +604,15 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  				param.root_decoder);
>  			return -ENXIO;
>  		}
> +	} else if (p->mode == CXL_DECODER_MODE_RAM) {
> +		region = cxl_decoder_create_ram_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",
> +		log_err(&rl, "region type '%s' is not supported\n",
>  			param.type);
>  		return -EOPNOTSUPP;
>  	}
> @@ -602,10 +624,12 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  		goto out;
>  	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);
> +	if (p->mode == CXL_DECODER_MODE_PMEM) {
> +		uuid_generate(uuid);
> +		try(cxl_region, set_uuid, region, uuid);
> +	}
>  	try(cxl_region, set_size, region, size);
>  
>  	for (i = 0; i < p->ways; i++) {
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 9832d09..84f60ad 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -246,4 +246,5 @@ global:
>  LIBCXL_5 {
>  global:
>  	cxl_region_get_mode;
> +	cxl_decoder_create_ram_region;
>  } LIBCXL_4;
> 
> -- 
> 2.39.1
> 

Re: [PATCH ndctl 4/7] cxl/region: accept user-supplied UUIDs for pmem regions

[Ira Weiny]

Vishal Verma wrote:
> Attempting to add additional checking around user-supplied UUIDs against
> 'ram' type regions revealed that commit 21b089025178 ("cxl: add a 'create-region' command")
> completely neglected to add the requisite support for accepting
> user-supplied UUIDs, even though the man page for cxl-create-region
> advertised the option.
> 
> Fix this by actually adding this option now, and add checks to validate
> the user-supplied UUID, and refuse it for ram regions.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/region.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 0945a14..9079b2d 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -22,6 +22,7 @@ static struct region_params {
>  	const char *bus;
>  	const char *size;
>  	const char *type;
> +	const char *uuid;
>  	const char *root_decoder;
>  	const char *region;
>  	int ways;
> @@ -40,6 +41,7 @@ struct parsed_params {
>  	u64 ep_min_size;
>  	int ways;
>  	int granularity;
> +	uuid_t uuid;
>  	struct json_object *memdevs;
>  	int num_memdevs;
>  	int argc;
> @@ -74,6 +76,8 @@ OPT_INTEGER('g', "granularity", &param.granularity,  \
>  	    "granularity of the interleave set"), \
>  OPT_STRING('t', "type", &param.type, \
>  	   "region type", "region type - 'pmem' or 'ram'"), \
> +OPT_STRING('U', "uuid", &param.uuid, \
> +	   "region uuid", "uuid for the new region (default: autogenerate)"), \
>  OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
>  	    "non-option arguments are memdevs"), \
>  OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats")
> @@ -293,6 +297,11 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
>  
>  	if (param.type) {
>  		p->mode = cxl_decoder_mode_from_ident(param.type);
> +		if (p->mode == CXL_DECODER_MODE_RAM && param.uuid) {
> +			log_err(&rl,
> +				"can't set UUID for ram / volatile regions");
> +			return -EINVAL;
> +		}
>  		if (p->mode == CXL_DECODER_MODE_NONE) {
>  			log_err(&rl, "unsupported type: %s\n", param.type);
>  			return -EINVAL;
> @@ -341,6 +350,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
>  		}
>  	}
>  
> +	if (param.uuid) {
> +		if (uuid_parse(param.uuid, p->uuid)) {
> +			error("failed to parse uuid: '%s'\n", param.uuid);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -566,7 +582,6 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  	int i, rc, granularity;
>  	u64 size, max_extent;
>  	const char *devname;
> -	uuid_t uuid;
>  
>  	rc = create_region_validate_config(ctx, p);
>  	if (rc)
> @@ -627,8 +642,9 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  	try(cxl_region, set_interleave_granularity, region, granularity);
>  	try(cxl_region, set_interleave_ways, region, p->ways);
>  	if (p->mode == CXL_DECODER_MODE_PMEM) {
> -		uuid_generate(uuid);
> -		try(cxl_region, set_uuid, region, uuid);
> +		if (!param.uuid)
> +			uuid_generate(p->uuid);
> +		try(cxl_region, set_uuid, region, p->uuid);
>  	}
>  	try(cxl_region, set_size, region, size);
>  
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Ira Weiny]

Vishal Verma wrote:
> In the common case, root decoders are expected to be either pmem
> capable, or volatile capable, but not necessarily both simultaneously.
> If a decoder only has one of pmem or volatile capabilities,
> cxl-create-region should just infer the type of the region (pmem
> or ram) based on this capability.
> 
> Maintain the default behavior of cxl-create-region to choose type=pmem,
> but only as a fallback if the selected root decoder has multiple
> capabilities. If it is only capable of either pmem, or ram, then infer
> region type from this without requiring it to be specified explicitly.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-create-region.txt |  3 ++-
>  cxl/region.c                            | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index ada0e52..f11a412 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -75,7 +75,8 @@ include::bus-option.txt[]
>  
>  -t::
>  --type=::
> -	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> +	Specify the region type - 'pmem' or 'ram'. Default to root decoder
> +	capability, and if that is ambiguous, default to 'pmem'.
>  
>  -U::
>  --uuid=::
> diff --git a/cxl/region.c b/cxl/region.c
> index 9079b2d..1c8ccc7 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
>  	return 0;
>  }
>  
> +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> +{
> +	int num_cap = 0;
> +
> +	/* if param.type was explicitly specified, nothing to do here */
> +	if (param.type)
> +		return;
> +
> +	/*
> +	 * if the root decoder only has one type of capability, default
> +	 * to that mode for the region.
> +	 */
> +	if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +		num_cap++;
> +	if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +		num_cap++;
> +
> +	if (num_cap == 1) {
> +		if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_RAM;
> +		else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_PMEM;
> +	}

I feel like the default for p->mode should be moved here from
parse_create_options().  But I'm not sure what the flows might be like in
that case.  That means p->mode would default to NONE until here.

That would make the man page behavior and this function match up nicely
for future maintenance.

But I don't think this is wrong.  So:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> +}
> +
>  static int create_region_validate_config(struct cxl_ctx *ctx,
>  					 struct parsed_params *p)
>  {
> @@ -481,6 +506,8 @@ found:
>  		return -ENXIO;
>  	}
>  
> +	set_type_from_decoder(ctx, p);
> +
>  	rc = validate_decoder(p->root_decoder, p);
>  	if (rc)
>  		return rc;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 6/7] cxl/list: Include regions in the verbose listing

[Ira Weiny]

Vishal Verma wrote:
> From: Dan Williams <dan.j.williams@intel.com>
> 
> When verbose listing was added, region listing support was not available, so
> it got missed. Add it now.
> 

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/list.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/cxl/list.c b/cxl/list.c
> index e3ef1fb..4e77aeb 100644
> --- a/cxl/list.c
> +++ b/cxl/list.c
> @@ -126,6 +126,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>  		param.endpoints = true;
>  		param.decoders = true;
>  		param.targets = true;
> +		param.regions = true;
>  		/*fallthrough*/
>  	case 0:
>  		break;
> 
> -- 
> 2.39.1
> 

Re: [PATCH ndctl 7/7] cxl/list: Enumerate device-dax properties for regions

[Ira Weiny]

Vishal Verma wrote:
> From: Dan Williams <dan.j.williams@intel.com>
> 
> Recently the kernel added support for routing newly mapped CXL regions to
> device-dax. Include the json representation of a DAX region beneath its
> host CXL region.
> 

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> [vishal: fix missing dsxctl/json.h include in cxl/json.c]
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-list.txt   | 31 +++++++++++++++++++++++++++++++
>  Documentation/cxl/lib/libcxl.txt |  7 +++++++
>  cxl/lib/private.h                |  1 +
>  cxl/lib/libcxl.c                 | 39 +++++++++++++++++++++++++++++++++++++++
>  cxl/filter.h                     |  3 +++
>  cxl/libcxl.h                     |  1 +
>  cxl/filter.c                     |  1 +
>  cxl/json.c                       | 16 ++++++++++++++++
>  cxl/list.c                       |  2 ++
>  cxl/lib/libcxl.sym               |  1 +
>  cxl/lib/meson.build              |  1 +
>  cxl/meson.build                  |  3 +++
>  12 files changed, 106 insertions(+)
> 

RE: [PATCH ndctl 1/7] cxl/region: skip region_actions for region creation

[Dan Williams]

Vishal Verma wrote:
> Commit 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> removed the early return for create-region, and this caused a
> create-region operation to unnecessarily loop through buses and root
> decoders only to EINVAL out because ACTION_CREATE is handled outside of
> the other actions. This results in confising messages such as:
> 
>   # cxl create-region -t ram -d 0.0 -m 0,4
>   {
>     "region":"region7",
>     "resource":"0xf030000000",
>     "size":"512.00 MiB (536.87 MB)",
>     ...
>   }
>   cxl region: decoder_region_action: region0: failed: Invalid argument
>   cxl region: region_action: one or more failures, last failure: Invalid argument
>   cxl region: cmd_create_region: created 1 region
> 
> Since there's no need to walk through the topology after creating a
> region, and especially not to perform an invalid 'action', switch
> back to retuening early for create-region.
> 
> Fixes: 3d6cd829ec08 ("cxl/region: Use cxl_filter_walk() to gather create-region targets")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/region.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index efe05aa..38aa142 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -789,7 +789,7 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  		return rc;
>  
>  	if (action == ACTION_CREATE)
> -		rc = create_region(ctx, count, p);
> +		return create_region(ctx, count, p);

Looks good, can't remember the motivation for changing that.

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

...modulo the spelling fixes that Fan noted.

RE: [PATCH ndctl 2/7] cxl: add a type attribute to region listings

[Dan Williams]

Vishal Verma wrote:
> In preparation for enumerating and creating 'volatile' or 'ram' type
> regions, add a 'type' attribute to region listings, so these can be
> distinguished from 'pmem' type regions easily. This depends on a new
> 'mode' attribute for region objects in sysfs. For older kernels that
> lack this, region listings will simply omit emitting this attribute,
> but otherwise not treat it as a failure.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/lib/libcxl.txt |  1 +
>  cxl/lib/private.h                |  1 +
>  cxl/lib/libcxl.c                 | 11 +++++++++++
>  cxl/libcxl.h                     |  1 +
>  cxl/json.c                       |  5 +++++
>  cxl/lib/libcxl.sym               |  5 +++++
>  6 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index f9af376..dbc4b56 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -550,6 +550,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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);
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index f8871bd..306dc3a 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -149,6 +149,7 @@ struct cxl_region {
>  	unsigned int interleave_ways;
>  	unsigned int interleave_granularity;
>  	enum cxl_decode_state decode_state;
> +	enum cxl_decoder_mode mode;
>  	struct kmod_module *module;
>  	struct list_head mappings;
>  };
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 4205a58..83f628b 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -561,6 +561,12 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  	else
>  		region->decode_state = strtoul(buf, NULL, 0);
>  
> +	sprintf(path, "%s/mode", cxlregion_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		region->mode = CXL_DECODER_MODE_NONE;
> +	else
> +		region->mode = cxl_decoder_mode_from_ident(buf);
> +
>  	sprintf(path, "%s/modalias", cxlregion_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		region->module = util_modalias_to_module(ctx, buf);
> @@ -686,6 +692,11 @@ CXL_EXPORT unsigned long long cxl_region_get_resource(struct cxl_region *region)
>  	return region->start;
>  }
>  
> +CXL_EXPORT enum cxl_decoder_mode cxl_region_get_mode(struct cxl_region *region)
> +{
> +	return region->mode;
> +}
> +
>  CXL_EXPORT unsigned int
>  cxl_region_get_interleave_ways(struct cxl_region *region)
>  {
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index d699af8..e6cca11 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -273,6 +273,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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,
> diff --git a/cxl/json.c b/cxl/json.c
> index 0fc44e4..f625380 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -827,6 +827,7 @@ void util_cxl_mappings_append_json(struct json_object *jregion,
>  struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  					     unsigned long flags)
>  {
> +	enum cxl_decoder_mode mode = cxl_region_get_mode(region);
>  	const char *devname = cxl_region_get_devname(region);
>  	struct json_object *jregion, *jobj;
>  	u64 val;
> @@ -853,6 +854,10 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  			json_object_object_add(jregion, "size", jobj);
>  	}
>  
> +	jobj = json_object_new_string(cxl_decoder_mode_name(mode));
> +	if (jobj)
> +		json_object_object_add(jregion, "type", jobj);
> +

I am thinking this should be gated by an:

    if (mode != CXL_DECODER_MODE_NONE)

...just to avoid saying "type : none" on older kernels where there is an
implied non-NONE type.

Otherwise looks good to me:

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

RE: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Dan Williams]

Vishal Verma wrote:
> Add support in libcxl to create ram regions through a new
> cxl_decoder_create_ram_region() API, which works similarly to its pmem
> sibling.
> 
> Enable ram region creation in cxl-cli, with the only differences from
> the pmem flow being:
>   1/ Use the above create_ram_region API, and
>   2/ Elide setting the UUID, since ram regions don't have one
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Looks good,

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

RE: [PATCH ndctl 4/7] cxl/region: accept user-supplied UUIDs for pmem regions

[Dan Williams]

Vishal Verma wrote:
> Attempting to add additional checking around user-supplied UUIDs against
> 'ram' type regions revealed that commit 21b089025178 ("cxl: add a 'create-region' command")
> completely neglected to add the requisite support for accepting
> user-supplied UUIDs, even though the man page for cxl-create-region
> advertised the option.
> 
> Fix this by actually adding this option now, and add checks to validate
> the user-supplied UUID, and refuse it for ram regions.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---

Looks good,

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

RE: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Dan Williams]

Vishal Verma wrote:
> In the common case, root decoders are expected to be either pmem
> capable, or volatile capable, but not necessarily both simultaneously.
> If a decoder only has one of pmem or volatile capabilities,
> cxl-create-region should just infer the type of the region (pmem
> or ram) based on this capability.
> 
> Maintain the default behavior of cxl-create-region to choose type=pmem,
> but only as a fallback if the selected root decoder has multiple
> capabilities. If it is only capable of either pmem, or ram, then infer
> region type from this without requiring it to be specified explicitly.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  Documentation/cxl/cxl-create-region.txt |  3 ++-
>  cxl/region.c                            | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index ada0e52..f11a412 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -75,7 +75,8 @@ include::bus-option.txt[]
>  
>  -t::
>  --type=::
> -	Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'.
> +	Specify the region type - 'pmem' or 'ram'. Default to root decoder
> +	capability, and if that is ambiguous, default to 'pmem'.
>  
>  -U::
>  --uuid=::
> diff --git a/cxl/region.c b/cxl/region.c
> index 9079b2d..1c8ccc7 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
>  	return 0;
>  }
>  
> +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> +{
> +	int num_cap = 0;
> +
> +	/* if param.type was explicitly specified, nothing to do here */
> +	if (param.type)
> +		return;
> +
> +	/*
> +	 * if the root decoder only has one type of capability, default
> +	 * to that mode for the region.
> +	 */
> +	if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +		num_cap++;
> +	if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +		num_cap++;
> +
> +	if (num_cap == 1) {
> +		if (cxl_decoder_is_volatile_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_RAM;
> +		else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> +			p->mode = CXL_DECODER_MODE_PMEM;
> +	}

Is @num_cap needed? I.e. if this just does:

    if (cxl_decoder_is_volatile_capable(p->root_decoder))
    	p->mode = CXL_DECODER_MODE_RAM;
    if (cxl_decoder_is_pmem_capable(p->root_decoder))
    	p->mode = CXL_DECODER_MODE_PMEM;

...then it matches the changelog of defaulting to pmem if both types are
set, and otherwise the single capability dominates.

RE: [PATCH ndctl 7/7] cxl/list: Enumerate device-dax properties for regions

[Dan Williams]

Vishal Verma wrote:
> From: Dan Williams <dan.j.williams@intel.com>
> 
> Recently the kernel added support for routing newly mapped CXL regions to
> device-dax. Include the json representation of a DAX region beneath its
> host CXL region.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> [vishal: fix missing dsxctl/json.h include in cxl/json.c]

s/dsxctl/daxctl/

...definitely needed, wonder why my build didn't fail?

Does cxl/filter.c need it? Looks like I added it there instead of where
it was needed.

Re: [PATCH ndctl 2/7] cxl: add a type attribute to region listings

[Verma, Vishal L]

On Tue, 2023-02-07 at 21:47 -0800, Dan Williams wrote:
> Vishal Verma wrote:
<..>
> > 
> > @@ -853,6 +854,10 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
> >                         json_object_object_add(jregion, "size", jobj);
> >         }
> >  
> > +       jobj = json_object_new_string(cxl_decoder_mode_name(mode));
> > +       if (jobj)
> > +               json_object_object_add(jregion, "type", jobj);
> > +
> 
> I am thinking this should be gated by an:
> 
>     if (mode != CXL_DECODER_MODE_NONE)
> 
> ...just to avoid saying "type : none" on older kernels where there is an
> implied non-NONE type.

Yep makes sense, I'll add this.

> 
> Otherwise looks good to me:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Verma, Vishal L]

On Tue, 2023-02-07 at 19:55 -0800, Ira Weiny wrote:
> Vishal Verma wrote:
<..>
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index 38aa142..0945a14 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
> >                 struct json_object *jobj =
> >                         json_object_array_get_idx(p->memdevs, i);
> >                 struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > -               u64 size = cxl_memdev_get_pmem_size(memdev);
> > +               u64 size;
> > +
> > +               switch(p->mode) {
> > +               case CXL_DECODER_MODE_RAM:
> > +                       size = cxl_memdev_get_ram_size(memdev);
> > +                       break;
> > +               case CXL_DECODER_MODE_PMEM:
> > +                       size = cxl_memdev_get_pmem_size(memdev);
> > +                       break;
> > +               default:
> > +                       /*
> > +                        * This will 'poison' ep_min_size with a 0, and
> > +                        * subsequently cause the region creation to fail.
> > +                        */
> > +                       size = 0;
> 
> Why not change collect_minsize() to return int and propagate the error up
> through create_region_validate_config()?
> 
> It seems more confusing to hide a special value in size like this.
> 
Hm, true, though the default case should never get hit. In fact I was
planning to leave it out entirely until gcc warned that I can't skip
cases if switching for an enum. I think the comment is maybe misleading
in that it makes one think that this is some special handling. It would
probably be clearer to make size = 0 in the initial declaration, and
make the default case a no-op (maybe with a comment that we would never
get here). Does that sound better?
> 

Re: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Verma, Vishal L]

On Tue, 2023-02-07 at 20:07 -0800, Ira Weiny wrote:
> Vishal Verma wrote:
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index 9079b2d..1c8ccc7 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
> >         return 0;
> >  }
> >  
> > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> > +{
> > +       int num_cap = 0;
> > +
> > +       /* if param.type was explicitly specified, nothing to do here */
> > +       if (param.type)
> > +               return;
> > +
> > +       /*
> > +        * if the root decoder only has one type of capability, default
> > +        * to that mode for the region.
> > +        */
> > +       if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > +               num_cap++;
> > +       if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > +               num_cap++;
> > +
> > +       if (num_cap == 1) {
> > +               if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > +                       p->mode = CXL_DECODER_MODE_RAM;
> > +               else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > +                       p->mode = CXL_DECODER_MODE_PMEM;
> > +       }
> 
> I feel like the default for p->mode should be moved here from
> parse_create_options().  But I'm not sure what the flows might be like in
> that case.  That means p->mode would default to NONE until here.
> 
> That would make the man page behavior and this function match up nicely
> for future maintenance.

Hm, do they not match now? I can see the appeal in collecting the
default mode setup in one place, but in my mind the early checks /
defaults in parse_create_options() are a simple, initial pass for
canned defaults, and conflicting option checks. Things like
set_type_from_decoder() are more of a 'second pass' thing where we may
do more involved porcelain type decision making based on the full
topology.

> 
> But I don't think this is wrong.  So:
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Verma, Vishal L]

On Tue, 2023-02-07 at 21:55 -0800, Dan Williams wrote:
> Vishal Verma wrote:
> > 
<..> 
> > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> > +{
> > +       int num_cap = 0;
> > +
> > +       /* if param.type was explicitly specified, nothing to do here */
> > +       if (param.type)
> > +               return;
> > +
> > +       /*
> > +        * if the root decoder only has one type of capability, default
> > +        * to that mode for the region.
> > +        */
> > +       if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > +               num_cap++;
> > +       if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > +               num_cap++;
> > +
> > +       if (num_cap == 1) {
> > +               if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > +                       p->mode = CXL_DECODER_MODE_RAM;
> > +               else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > +                       p->mode = CXL_DECODER_MODE_PMEM;
> > +       }
> 
> Is @num_cap needed? I.e. if this just does:
> 
>     if (cxl_decoder_is_volatile_capable(p->root_decoder))
>         p->mode = CXL_DECODER_MODE_RAM;
>     if (cxl_decoder_is_pmem_capable(p->root_decoder))
>         p->mode = CXL_DECODER_MODE_PMEM;
> 
> ...then it matches the changelog of defaulting to pmem if both types are
> set, and otherwise the single capability dominates.

Oh true, that's clever! I hope it isn't too clever for future-me when
coming across this and wondering what is happening, but for now I like
it, so I'll run with it :)

Re: [PATCH ndctl 7/7] cxl/list: Enumerate device-dax properties for regions

[Verma, Vishal L]

On Tue, 2023-02-07 at 22:00 -0800, Dan Williams wrote:
> Vishal Verma wrote:
> > From: Dan Williams <dan.j.williams@intel.com>
> > 
> > Recently the kernel added support for routing newly mapped CXL regions to
> > device-dax. Include the json representation of a DAX region beneath its
> > host CXL region.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > [vishal: fix missing dsxctl/json.h include in cxl/json.c]
> 
> s/dsxctl/daxctl/
> 
> ...definitely needed, wonder why my build didn't fail?

IIRC it was a warning, not an error - it assumed a default int return
type, and warned about that.

> 
> Does cxl/filter.c need it? Looks like I added it there instead of where
> it was needed.

Ah true, cxl/filter.c doesn't need it. I'll remove it from there.

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Ira Weiny]

Verma, Vishal L wrote:
> On Tue, 2023-02-07 at 19:55 -0800, Ira Weiny wrote:
> > Vishal Verma wrote:
> <..>
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index 38aa142..0945a14 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
> > >                 struct json_object *jobj =
> > >                         json_object_array_get_idx(p->memdevs, i);
> > >                 struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > > -               u64 size = cxl_memdev_get_pmem_size(memdev);
> > > +               u64 size;
> > > +
> > > +               switch(p->mode) {
> > > +               case CXL_DECODER_MODE_RAM:
> > > +                       size = cxl_memdev_get_ram_size(memdev);
> > > +                       break;
> > > +               case CXL_DECODER_MODE_PMEM:
> > > +                       size = cxl_memdev_get_pmem_size(memdev);
> > > +                       break;
> > > +               default:
> > > +                       /*
> > > +                        * This will 'poison' ep_min_size with a 0, and
> > > +                        * subsequently cause the region creation to fail.
> > > +                        */
> > > +                       size = 0;
> > 
> > Why not change collect_minsize() to return int and propagate the error up
> > through create_region_validate_config()?
> > 
> > It seems more confusing to hide a special value in size like this.
> > 
> Hm, true, though the default case should never get hit. In fact I was
> planning to leave it out entirely until gcc warned that I can't skip
> cases if switching for an enum. I think the comment is maybe misleading
> in that it makes one think that this is some special handling. It would
> probably be clearer to make size = 0 in the initial declaration, and
> make the default case a no-op (maybe with a comment that we would never
> get here). Does that sound better?
> > 

To me the question is whether size == 0 is an error or just a valid size
which something at the higher level determines is an error.  To me the
default case here is that mode is incorrectly set to get a valid size.
Your comment implied that as well.

Having size == 0 is ok generally as this is a 'min size'.  So I will not
argue the point too much.

Ira

Re: [PATCH ndctl 5/7] cxl/region: determine region type based on root decoder capability

[Ira Weiny]

Verma, Vishal L wrote:
> On Tue, 2023-02-07 at 20:07 -0800, Ira Weiny wrote:
> > Vishal Verma wrote:
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index 9079b2d..1c8ccc7 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder,
> > >         return 0;
> > >  }
> > >  
> > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
> > > +{
> > > +       int num_cap = 0;
> > > +
> > > +       /* if param.type was explicitly specified, nothing to do here */
> > > +       if (param.type)
> > > +               return;
> > > +
> > > +       /*
> > > +        * if the root decoder only has one type of capability, default
> > > +        * to that mode for the region.
> > > +        */
> > > +       if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > > +               num_cap++;
> > > +       if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > > +               num_cap++;
> > > +
> > > +       if (num_cap == 1) {
> > > +               if (cxl_decoder_is_volatile_capable(p->root_decoder))
> > > +                       p->mode = CXL_DECODER_MODE_RAM;
> > > +               else if (cxl_decoder_is_pmem_capable(p->root_decoder))
> > > +                       p->mode = CXL_DECODER_MODE_PMEM;
> > > +       }
> > 
> > I feel like the default for p->mode should be moved here from
> > parse_create_options().  But I'm not sure what the flows might be like in
> > that case.  That means p->mode would default to NONE until here.
> > 
> > That would make the man page behavior and this function match up nicely
> > for future maintenance.
> 
> Hm, do they not match now?

As I said it is correct.

> I can see the appeal in collecting the
> default mode setup in one place, but in my mind the early checks /
> defaults in parse_create_options() are a simple, initial pass for
> canned defaults, and conflicting option checks. Things like
> set_type_from_decoder() are more of a 'second pass' thing where we may
> do more involved porcelain type decision making based on the full
> topology.

Yea I can see it that way too.

> 
> > 
> > But I don't think this is wrong.  So:
> > 
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
 
My tag stands.
Ira

Re: [PATCH ndctl 2/7] cxl: add a type attribute to region listings

[Fan Ni]

On Tue, Feb 07, 2023 at 12:16:28PM -0700, Vishal Verma wrote:
> In preparation for enumerating and creating 'volatile' or 'ram' type
> regions, add a 'type' attribute to region listings, so these can be
> distinguished from 'pmem' type regions easily. This depends on a new
> 'mode' attribute for region objects in sysfs. For older kernels that
> lack this, region listings will simply omit emitting this attribute,
> but otherwise not treat it as a failure.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  Documentation/cxl/lib/libcxl.txt |  1 +
>  cxl/lib/private.h                |  1 +
>  cxl/lib/libcxl.c                 | 11 +++++++++++
>  cxl/libcxl.h                     |  1 +
>  cxl/json.c                       |  5 +++++
>  cxl/lib/libcxl.sym               |  5 +++++
>  6 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index f9af376..dbc4b56 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -550,6 +550,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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);
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index f8871bd..306dc3a 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -149,6 +149,7 @@ struct cxl_region {
>  	unsigned int interleave_ways;
>  	unsigned int interleave_granularity;
>  	enum cxl_decode_state decode_state;
> +	enum cxl_decoder_mode mode;
>  	struct kmod_module *module;
>  	struct list_head mappings;
>  };
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 4205a58..83f628b 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -561,6 +561,12 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  	else
>  		region->decode_state = strtoul(buf, NULL, 0);
>  
> +	sprintf(path, "%s/mode", cxlregion_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		region->mode = CXL_DECODER_MODE_NONE;
> +	else
> +		region->mode = cxl_decoder_mode_from_ident(buf);
> +
>  	sprintf(path, "%s/modalias", cxlregion_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		region->module = util_modalias_to_module(ctx, buf);
> @@ -686,6 +692,11 @@ CXL_EXPORT unsigned long long cxl_region_get_resource(struct cxl_region *region)
>  	return region->start;
>  }
>  
> +CXL_EXPORT enum cxl_decoder_mode cxl_region_get_mode(struct cxl_region *region)
> +{
> +	return region->mode;
> +}
> +
>  CXL_EXPORT unsigned int
>  cxl_region_get_interleave_ways(struct cxl_region *region)
>  {
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index d699af8..e6cca11 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -273,6 +273,7 @@ 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);
> +enum cxl_decoder_mode cxl_region_get_mode(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,
> diff --git a/cxl/json.c b/cxl/json.c
> index 0fc44e4..f625380 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -827,6 +827,7 @@ void util_cxl_mappings_append_json(struct json_object *jregion,
>  struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  					     unsigned long flags)
>  {
> +	enum cxl_decoder_mode mode = cxl_region_get_mode(region);
>  	const char *devname = cxl_region_get_devname(region);
>  	struct json_object *jregion, *jobj;
>  	u64 val;
> @@ -853,6 +854,10 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>  			json_object_object_add(jregion, "size", jobj);
>  	}
>  
> +	jobj = json_object_new_string(cxl_decoder_mode_name(mode));
> +	if (jobj)
> +		json_object_object_add(jregion, "type", jobj);
> +
>  	val = cxl_region_get_interleave_ways(region);
>  	if (val < INT_MAX) {
>  		jobj = json_object_new_int(val);
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 6bc0810..9832d09 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -242,3 +242,8 @@ global:
>  	cxl_target_get_firmware_node;
>  	cxl_dport_get_firmware_node;
>  } LIBCXL_3;
> +
> +LIBCXL_5 {
> +global:
> +	cxl_region_get_mode;
> +} LIBCXL_4;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Fan Ni]

On Tue, Feb 07, 2023 at 12:16:29PM -0700, Vishal Verma wrote:
> Add support in libcxl to create ram regions through a new
> cxl_decoder_create_ram_region() API, which works similarly to its pmem
> sibling.
> 
> Enable ram region creation in cxl-cli, with the only differences from
> the pmem flow being:
>   1/ Use the above create_ram_region API, and
>   2/ Elide setting the UUID, since ram regions don't have one
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

One minor thing, there exists some code format inconsistency in
cxl/region.c file (not introduced by the patch). For exmaple,
the "switch" sometimes is followed with a space but sometime not.

> ---
>  Documentation/cxl/cxl-create-region.txt |  3 ++-
>  cxl/lib/libcxl.c                        | 22 +++++++++++++++++++---
>  cxl/libcxl.h                            |  1 +
>  cxl/region.c                            | 32 ++++++++++++++++++++++++++++----
>  cxl/lib/libcxl.sym                      |  1 +
>  5 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index 286779e..ada0e52 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -80,7 +80,8 @@ include::bus-option.txt[]
>  -U::
>  --uuid=::
>  	Specify a UUID for the new region. This shouldn't usually need to be
> -	specified, as one will be generated by default.
> +	specified, as one will be generated by default. Only applicable to
> +	pmem regions.
>  
>  -w::
>  --ways=::
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 83f628b..c5b9b18 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -2234,8 +2234,8 @@ cxl_decoder_get_region(struct cxl_decoder *decoder)
>  	return NULL;
>  }
>  
> -CXL_EXPORT struct cxl_region *
> -cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> +static struct cxl_region *cxl_decoder_create_region(struct cxl_decoder *decoder,
> +						    enum cxl_decoder_mode mode)
>  {
>  	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
>  	char *path = decoder->dev_buf;
> @@ -2243,7 +2243,11 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  	struct cxl_region *region;
>  	int rc;
>  
> -	sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> +	if (mode == CXL_DECODER_MODE_PMEM)
> +		sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> +	else if (mode == CXL_DECODER_MODE_RAM)
> +		sprintf(path, "%s/create_ram_region", decoder->dev_path);
> +
>  	rc = sysfs_read_attr(ctx, path, buf);
>  	if (rc < 0) {
>  		err(ctx, "failed to read new region name: %s\n",
> @@ -2282,6 +2286,18 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  	return region;
>  }
>  
> +CXL_EXPORT struct cxl_region *
> +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> +{
> +	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_PMEM);
> +}
> +
> +CXL_EXPORT struct cxl_region *
> +cxl_decoder_create_ram_region(struct cxl_decoder *decoder)
> +{
> +	return cxl_decoder_create_region(decoder, CXL_DECODER_MODE_RAM);
> +}
> +
>  CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
>  {
>  	return decoder->nr_targets;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index e6cca11..904156c 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -213,6 +213,7 @@ 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_get_region(struct cxl_decoder *decoder);
>  struct cxl_region *cxl_decoder_create_pmem_region(struct cxl_decoder *decoder);
> +struct cxl_region *cxl_decoder_create_ram_region(struct cxl_decoder *decoder);
>  struct cxl_decoder *cxl_decoder_get_by_name(struct cxl_ctx *ctx,
>  					    const char *ident);
>  struct cxl_memdev *cxl_decoder_get_memdev(struct cxl_decoder *decoder);
> diff --git a/cxl/region.c b/cxl/region.c
> index 38aa142..0945a14 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
>  		struct json_object *jobj =
>  			json_object_array_get_idx(p->memdevs, i);
>  		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> -		u64 size = cxl_memdev_get_pmem_size(memdev);
> +		u64 size;
> +
> +		switch(p->mode) {
> +		case CXL_DECODER_MODE_RAM:
> +			size = cxl_memdev_get_ram_size(memdev);
> +			break;
> +		case CXL_DECODER_MODE_PMEM:
> +			size = cxl_memdev_get_pmem_size(memdev);
> +			break;
> +		default:
> +			/*
> +			 * This will 'poison' ep_min_size with a 0, and
> +			 * subsequently cause the region creation to fail.
> +			 */
> +			size = 0;
> +		}
>  
>  		if (!p->ep_min_size)
>  			p->ep_min_size = size;
> @@ -589,8 +604,15 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  				param.root_decoder);
>  			return -ENXIO;
>  		}
> +	} else if (p->mode == CXL_DECODER_MODE_RAM) {
> +		region = cxl_decoder_create_ram_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",
> +		log_err(&rl, "region type '%s' is not supported\n",
>  			param.type);
>  		return -EOPNOTSUPP;
>  	}
> @@ -602,10 +624,12 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  		goto out;
>  	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);
> +	if (p->mode == CXL_DECODER_MODE_PMEM) {
> +		uuid_generate(uuid);
> +		try(cxl_region, set_uuid, region, uuid);
> +	}
>  	try(cxl_region, set_size, region, size);
>  
>  	for (i = 0; i < p->ways; i++) {
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 9832d09..84f60ad 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -246,4 +246,5 @@ global:
>  LIBCXL_5 {
>  global:
>  	cxl_region_get_mode;
> +	cxl_decoder_create_ram_region;
>  } LIBCXL_4;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Verma, Vishal L]

On Fri, 2023-02-10 at 01:04 +0000, Fan Ni wrote:
> On Tue, Feb 07, 2023 at 12:16:29PM -0700, Vishal Verma wrote:
> > Add support in libcxl to create ram regions through a new
> > cxl_decoder_create_ram_region() API, which works similarly to its pmem
> > sibling.
> > 
> > Enable ram region creation in cxl-cli, with the only differences from
> > the pmem flow being:
> >   1/ Use the above create_ram_region API, and
> >   2/ Elide setting the UUID, since ram regions don't have one
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>

Hi Fan,

Would you mind responding on v2 of this series - b4 doesn't want to
pick up trailers from v1 now that v2 has been sent out.

> 
> One minor thing, there exists some code format inconsistency in
> cxl/region.c file (not introduced by the patch). For exmaple,
> the "switch" sometimes is followed with a space but sometime not.

Ah thanks, I'll take a look and send separate cleanup patches.

Re: [PATCH ndctl 3/7] cxl: add core plumbing for creation of ram regions

[Fan Ni]

On Fri, Feb 10, 2023 at 01:10:44AM +0000, Verma, Vishal L wrote:

> On Fri, 2023-02-10 at 01:04 +0000, Fan Ni wrote:
> > On Tue, Feb 07, 2023 at 12:16:29PM -0700, Vishal Verma wrote:
> > > Add support in libcxl to create ram regions through a new
> > > cxl_decoder_create_ram_region() API, which works similarly to its pmem
> > > sibling.
> > > 
> > > Enable ram region creation in cxl-cli, with the only differences from
> > > the pmem flow being:
> > >   1/ Use the above create_ram_region API, and
> > >   2/ Elide setting the UUID, since ram regions don't have one
> > > 
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > 
> > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> 
> Hi Fan,
> 
> Would you mind responding on v2 of this series - b4 doesn't want to
> pick up trailers from v1 now that v2 has been sent out.
Ah, almost missed v2. Will response on that. Thanks.
> 
> > 
> > One minor thing, there exists some code format inconsistency in
> > cxl/region.c file (not introduced by the patch). For exmaple,
> > the "switch" sometimes is followed with a space but sometime not.
> 
> Ah thanks, I'll take a look and send separate cleanup patches.
> 
> 
>