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

[PATCH ndctl v2 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>
---
Changes in v2:
- Fix typos in the commit message of patch 1 (Fan)
- Gate the type attr in region listings on mode != 'none' (Dan)
- Clarify unreachability of the default case in collect_minsize() (Ira)
- Simplify the mode setup in set_type_from_decoder() (Dan)
- Fix typo in the commit message of Patch 7 (Dan)
- Remove unneeded daxctl/json.h include from cxl/filter.c (Dan)
- Link to v1: https://lore.kernel.org/r/20230120-vv-volatile-regions-v1-0-b42b21ee8d0b@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/json.c                              | 23 +++++++++++
 cxl/list.c                              |  3 ++
 cxl/region.c                            | 66 +++++++++++++++++++++++++++---
 cxl/lib/libcxl.sym                      |  7 ++++
 cxl/lib/meson.build                     |  1 +
 cxl/meson.build                         |  3 ++
 13 files changed, 217 insertions(+), 11 deletions(-)
---
base-commit: 08720628d2ba469e203a18c0b1ffbd90f4bfab1d
change-id: 20230120-vv-volatile-regions-063950cef590

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

[PATCH ndctl v2 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 confusing 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 returning 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: 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

[PATCH ndctl v2 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>
Reviewed-by: 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                       |  7 +++++++
 cxl/lib/libcxl.sym               |  5 +++++
 6 files changed, 26 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..16b6cb8 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,12 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 			json_object_object_add(jregion, "size", jobj);
 	}
 
+	if (mode != CXL_DECODER_MODE_NONE) {
+		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 v2 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>
Reviewed-by: 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                            | 28 ++++++++++++++++++++++++----
 cxl/lib/libcxl.sym                      |  1 +
 5 files changed, 47 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..c69cb9a 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -380,7 +380,18 @@ 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 = 0;
+
+		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:
+			/* Shouldn't ever get here */ ;
+		}
 
 		if (!p->ep_min_size)
 			p->ep_min_size = size;
@@ -589,8 +600,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 +620,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 v2 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>
Reviewed-by: 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 c69cb9a..5c908bb 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;
 }
 
@@ -562,7 +578,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)
@@ -623,8 +638,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 v2 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>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 Documentation/cxl/cxl-create-region.txt |  3 ++-
 cxl/region.c                            | 18 ++++++++++++++++++
 2 files changed, 20 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 5c908bb..07ce4a3 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -444,6 +444,22 @@ static int validate_decoder(struct cxl_decoder *decoder,
 	return 0;
 }
 
+static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
+{
+	/* if param.type was explicitly specified, nothing to do here */
+	if (param.type)
+		return;
+
+	/*
+	 * default to pmem if both types are set, otherwise the single
+	 * capability dominates.
+	 */
+	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;
+}
+
 static int create_region_validate_config(struct cxl_ctx *ctx,
 					 struct parsed_params *p)
 {
@@ -477,6 +493,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 v2 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>
Reviewed-by: Ira Weiny <ira.weiny@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 v2 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: move the daxctl/json.h include from cxl/filter.c to cxl/json.c]
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
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/json.c                       | 16 ++++++++++++++++
 cxl/list.c                       |  2 ++
 cxl/lib/libcxl.sym               |  1 +
 cxl/lib/meson.build              |  1 +
 cxl/meson.build                  |  3 +++
 11 files changed, 105 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/json.c b/cxl/json.c
index 16b6cb8..e87bdd4 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)
@@ -891,7 +892,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 v2 0/7] cxl: add support for listing and creating volatile regions

[Brice Goglin]

Le 08/02/2023 à 21:00, Vishal Verma a écrit :
> 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


Hello Vishal

I am trying to play with this but all my attempts failed so far. Could 
you provide Qemu and cxl-cli command-lines to get a volatile region 
enabled in a Qemu VM?

Thanks

Brice

Re: [PATCH ndctl v2 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>
> Reviewed-by: 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/cxl-create-region.txt |  3 ++-
>  cxl/lib/libcxl.c                        | 22 +++++++++++++++++++---
>  cxl/libcxl.h                            |  1 +
>  cxl/region.c                            | 28 ++++++++++++++++++++++++----
>  cxl/lib/libcxl.sym                      |  1 +
>  5 files changed, 47 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..c69cb9a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -380,7 +380,18 @@ 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 = 0;
> +
> +		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:
> +			/* Shouldn't ever get here */ ;
> +		}
>  
>  		if (!p->ep_min_size)
>  			p->ep_min_size = size;
> @@ -589,8 +600,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 +620,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 v2 0/7] cxl: add support for listing and creating volatile regions

[Verma, Vishal L]

On Thu, 2023-02-09 at 12:04 +0100, Brice Goglin wrote:
> 
> Le 08/02/2023 à 21:00, Vishal Verma a écrit :
> > 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
> 
> 
> Hello Vishal
> 
> I am trying to play with this but all my attempts failed so far. Could 
> you provide Qemu and cxl-cli command-lines to get a volatile region 
> enabled in a Qemu VM?

Hi Brice,

Greg had posted his working config in another thread:
https://lore.kernel.org/linux-cxl/Y9sMs0FGulQSIe9t@memverge.com/

I've also pasted below, the qemu command line generated by the run_qemu
script I referenced. (Note that this adds a bunch of stuff not strictly
needed for a minimal CXL configuration - you can certainly trim a lot
of that out - this is just the default setup that is generated and I
usually run).

Feel free to post what errors / problems you're hitting and we can
debug further from there.

Thanks
Vishal


$ run_qemu.sh -g --cxl --cxl-debug --rw -r none --cmdline
/home/vverma7/git/qemu/build/qemu-system-x86_64 
-machine q35,accel=kvm,nvdimm=on,cxl=on 
-m 8192M,slots=4,maxmem=40964M 
-smp 8,sockets=2,cores=2,threads=2 
-enable-kvm 
-display none 
-nographic 
-drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly=on 
-drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd 
-debugcon file:uefi_debug.log 
-global isa-debugcon.iobase=0x402 
-drive file=root.img,format=raw,media=disk 
-kernel ./mkosi.extra/lib/modules/6.2.0-rc6+/vmlinuz 
-initrd mkosi.extra/boot/initramfs-6.2.0-rc2+.img 
-append selinux=0 audit=0 console=tty0 console=ttyS0 root=/dev/sda2 ignore_loglevel rw cxl_acpi.dyndbg=+fplm cxl_pci.dyndbg=+fplm cxl_core.dyndbg=+fplm cxl_mem.dyndbg=+fplm cxl_pmem.dyndbg=+fplm cxl_port.dyndbg=+fplm cxl_region.dyndbg=+fplm cxl_test.dyndbg=+fplm cxl_mock.dyndbg=+fplm cxl_mock_mem.dyndbg=+fplm memmap=2G!4G efi_fake_mem=2G@6G:0x40000 
-device e1000,netdev=net0,mac=52:54:00:12:34:56 
-netdev user,id=net0,hostfwd=tcp::10022-:22 
-object memory-backend-file,id=cxl-mem0,share=on,mem-path=cxltest0.raw,size=256M 
-object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxltest1.raw,size=256M 
-object memory-backend-file,id=cxl-mem2,share=on,mem-path=cxltest2.raw,size=256M 
-object memory-backend-file,id=cxl-mem3,share=on,mem-path=cxltest3.raw,size=256M 
-object memory-backend-ram,id=cxl-mem4,share=on,size=256M 
-object memory-backend-ram,id=cxl-mem5,share=on,size=256M 
-object memory-backend-ram,id=cxl-mem6,share=on,size=256M 
-object memory-backend-ram,id=cxl-mem7,share=on,size=256M 
-object memory-backend-file,id=cxl-lsa0,share=on,mem-path=lsa0.raw,size=1K 
-object memory-backend-file,id=cxl-lsa1,share=on,mem-path=lsa1.raw,size=1K 
-object memory-backend-file,id=cxl-lsa2,share=on,mem-path=lsa2.raw,size=1K 
-object memory-backend-file,id=cxl-lsa3,share=on,mem-path=lsa3.raw,size=1K 
-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=53 
-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=191 
-device cxl-rp,id=hb0rp0,bus=cxl.0,chassis=0,slot=0,port=0 
-device cxl-rp,id=hb0rp1,bus=cxl.0,chassis=0,slot=1,port=1 
-device cxl-rp,id=hb0rp2,bus=cxl.0,chassis=0,slot=2,port=2 
-device cxl-rp,id=hb0rp3,bus=cxl.0,chassis=0,slot=3,port=3 
-device cxl-rp,id=hb1rp0,bus=cxl.1,chassis=0,slot=4,port=0 
-device cxl-rp,id=hb1rp1,bus=cxl.1,chassis=0,slot=5,port=1 
-device cxl-rp,id=hb1rp2,bus=cxl.1,chassis=0,slot=6,port=2 
-device cxl-rp,id=hb1rp3,bus=cxl.1,chassis=0,slot=7,port=3 
-device cxl-type3,bus=hb0rp0,memdev=cxl-mem0,id=cxl-dev0,lsa=cxl-lsa0 
-device cxl-type3,bus=hb0rp1,memdev=cxl-mem1,id=cxl-dev1,lsa=cxl-lsa1 
-device cxl-type3,bus=hb1rp0,memdev=cxl-mem2,id=cxl-dev2,lsa=cxl-lsa2 
-device cxl-type3,bus=hb1rp1,memdev=cxl-mem3,id=cxl-dev3,lsa=cxl-lsa3 
-device cxl-type3,bus=hb0rp2,volatile-memdev=cxl-mem4,id=cxl-dev4 
-device cxl-type3,bus=hb0rp3,volatile-memdev=cxl-mem5,id=cxl-dev5 
-device cxl-type3,bus=hb1rp2,volatile-memdev=cxl-mem6,id=cxl-dev6 
-device cxl-type3,bus=hb1rp3,volatile-memdev=cxl-mem7,id=cxl-dev7 
-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,cxl-fmw.1.targets.0=cxl.0,cxl-fmw.1.targets.1=cxl.1,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k 
-object memory-backend-ram,id=mem0,size=2048M 
-numa node,nodeid=0,memdev=mem0, 
-numa cpu,node-id=0,socket-id=0 
-object memory-backend-ram,id=mem1,size=2048M 
-numa node,nodeid=1,memdev=mem1, 
-numa cpu,node-id=1,socket-id=1 
-object memory-backend-ram,id=mem2,size=2048M 
-numa node,nodeid=2,memdev=mem2, 
-object memory-backend-ram,id=mem3,size=2048M 
-numa node,nodeid=3,memdev=mem3, 
-numa node,nodeid=4, 
-object memory-backend-file,id=nvmem0,share=on,mem-path=nvdimm-0,size=16384M,align=1G 
-device nvdimm,memdev=nvmem0,id=nv0,label-size=2M,node=4 
-numa node,nodeid=5, 
-object memory-backend-file,id=nvmem1,share=on,mem-path=nvdimm-1,size=16384M,align=1G 
-device nvdimm,memdev=nvmem1,id=nv1,label-size=2M,node=5 
-numa dist,src=0,dst=0,val=10 
-numa dist,src=0,dst=1,val=21 
-numa dist,src=0,dst=2,val=12 
-numa dist,src=0,dst=3,val=21 
-numa dist,src=0,dst=4,val=17 
-numa dist,src=0,dst=5,val=28 
-numa dist,src=1,dst=1,val=10 
-numa dist,src=1,dst=2,val=21 
-numa dist,src=1,dst=3,val=12 
-numa dist,src=1,dst=4,val=28 
-numa dist,src=1,dst=5,val=17 
-numa dist,src=2,dst=2,val=10 
-numa dist,src=2,dst=3,val=21 
-numa dist,src=2,dst=4,val=28 
-numa dist,src=2,dst=5,val=28 
-numa dist,src=3,dst=3,val=10 
-numa dist,src=3,dst=4,val=28 
-numa dist,src=3,dst=5,val=28 
-numa dist,src=4,dst=4,val=10 
-numa dist,src=4,dst=5,val=28 
-numa dist,src=5,dst=5,val=10 

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

[Fan Ni]

On Wed, Feb 08, 2023 at 01:00:31PM -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>
> Reviewed-by: 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                            | 28 ++++++++++++++++++++++++----
>  cxl/lib/libcxl.sym                      |  1 +
>  5 files changed, 47 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..c69cb9a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -380,7 +380,18 @@ 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 = 0;
> +
> +		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:
> +			/* Shouldn't ever get here */ ;
> +		}
>  
>  		if (!p->ep_min_size)
>  			p->ep_min_size = size;
> @@ -589,8 +600,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 +620,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 v2 0/7] cxl: add support for listing and creating volatile regions

[Brice Goglin]

[-- Attachment #1.1.1: Type: text/plain, Size: 6498 bytes --]


Le 09/02/2023 à 20:17, Verma, Vishal L a écrit :
> On Thu, 2023-02-09 at 12:04 +0100, Brice Goglin wrote:
>> Hello Vishal
>>
>> I am trying to play with this but all my attempts failed so far. Could
>> you provide Qemu and cxl-cli command-lines to get a volatile region
>> enabled in a Qemu VM?
> Hi Brice,
>
> Greg had posted his working config in another thread:
> https://lore.kernel.org/linux-cxl/Y9sMs0FGulQSIe9t@memverge.com/
>
> I've also pasted below, the qemu command line generated by the run_qemu
> script I referenced. (Note that this adds a bunch of stuff not strictly
> needed for a minimal CXL configuration - you can certainly trim a lot
> of that out - this is just the default setup that is generated and I
> usually run).


Hello Vishal

Thanks a lot, things were failing because my kernel didn't have
CONFIG_CXL_REGION_INVALIDATION_TEST=y. Now I am able to create a single
ram region, either with a single device or multiple interleaved ones.

However I can't get multiple separate ram regions. If I boot a config
like yours below, I get 4 ram devices. How can I create one region for
each? Once I create the first one, others fail saying something like
below. I tried using other decoders but it didn't help (I still need
to read more CXL docs about decoders, why new ones appear when creating
a region, etc).

cxl region: collect_memdevs: no active memdevs found: decoder: decoder0.0 filter: mem3

By the way, once configured in system ram, my CXL ram is merged into an
existing "normal" NUMA node. How do I tell Qemu that a CXL region should
be part of a new NUMA node? I assume that's what's going to happen on
real hardware?

Thanks

Brice



>
>
> $ run_qemu.sh -g --cxl --cxl-debug --rw -r none --cmdline
> /home/vverma7/git/qemu/build/qemu-system-x86_64
> -machine q35,accel=kvm,nvdimm=on,cxl=on
> -m 8192M,slots=4,maxmem=40964M
> -smp 8,sockets=2,cores=2,threads=2
> -enable-kvm
> -display none
> -nographic
> -drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly=on
> -drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd
> -debugconfile:uefi_debug.log  
> -global isa-debugcon.iobase=0x402
> -drive file=root.img,format=raw,media=disk
> -kernel ./mkosi.extra/lib/modules/6.2.0-rc6+/vmlinuz
> -initrd mkosi.extra/boot/initramfs-6.2.0-rc2+.img
> -append selinux=0 audit=0 console=tty0 console=ttyS0 root=/dev/sda2 ignore_loglevel rw cxl_acpi.dyndbg=+fplm cxl_pci.dyndbg=+fplm cxl_core.dyndbg=+fplm cxl_mem.dyndbg=+fplm cxl_pmem.dyndbg=+fplm cxl_port.dyndbg=+fplm cxl_region.dyndbg=+fplm cxl_test.dyndbg=+fplm cxl_mock.dyndbg=+fplm cxl_mock_mem.dyndbg=+fplm memmap=2G!4G efi_fake_mem=2G@6G:0x40000
> -device e1000,netdev=net0,mac=52:54:00:12:34:56
> -netdev user,id=net0,hostfwd=tcp::10022-:22
> -object memory-backend-file,id=cxl-mem0,share=on,mem-path=cxltest0.raw,size=256M
> -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxltest1.raw,size=256M
> -object memory-backend-file,id=cxl-mem2,share=on,mem-path=cxltest2.raw,size=256M
> -object memory-backend-file,id=cxl-mem3,share=on,mem-path=cxltest3.raw,size=256M
> -object memory-backend-ram,id=cxl-mem4,share=on,size=256M
> -object memory-backend-ram,id=cxl-mem5,share=on,size=256M
> -object memory-backend-ram,id=cxl-mem6,share=on,size=256M
> -object memory-backend-ram,id=cxl-mem7,share=on,size=256M
> -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=lsa0.raw,size=1K
> -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=lsa1.raw,size=1K
> -object memory-backend-file,id=cxl-lsa2,share=on,mem-path=lsa2.raw,size=1K
> -object memory-backend-file,id=cxl-lsa3,share=on,mem-path=lsa3.raw,size=1K
> -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=53
> -device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=191
> -device cxl-rp,id=hb0rp0,bus=cxl.0,chassis=0,slot=0,port=0
> -device cxl-rp,id=hb0rp1,bus=cxl.0,chassis=0,slot=1,port=1
> -device cxl-rp,id=hb0rp2,bus=cxl.0,chassis=0,slot=2,port=2
> -device cxl-rp,id=hb0rp3,bus=cxl.0,chassis=0,slot=3,port=3
> -device cxl-rp,id=hb1rp0,bus=cxl.1,chassis=0,slot=4,port=0
> -device cxl-rp,id=hb1rp1,bus=cxl.1,chassis=0,slot=5,port=1
> -device cxl-rp,id=hb1rp2,bus=cxl.1,chassis=0,slot=6,port=2
> -device cxl-rp,id=hb1rp3,bus=cxl.1,chassis=0,slot=7,port=3
> -device cxl-type3,bus=hb0rp0,memdev=cxl-mem0,id=cxl-dev0,lsa=cxl-lsa0
> -device cxl-type3,bus=hb0rp1,memdev=cxl-mem1,id=cxl-dev1,lsa=cxl-lsa1
> -device cxl-type3,bus=hb1rp0,memdev=cxl-mem2,id=cxl-dev2,lsa=cxl-lsa2
> -device cxl-type3,bus=hb1rp1,memdev=cxl-mem3,id=cxl-dev3,lsa=cxl-lsa3
> -device cxl-type3,bus=hb0rp2,volatile-memdev=cxl-mem4,id=cxl-dev4
> -device cxl-type3,bus=hb0rp3,volatile-memdev=cxl-mem5,id=cxl-dev5
> -device cxl-type3,bus=hb1rp2,volatile-memdev=cxl-mem6,id=cxl-dev6
> -device cxl-type3,bus=hb1rp3,volatile-memdev=cxl-mem7,id=cxl-dev7
> -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,cxl-fmw.1.targets.0=cxl.0,cxl-fmw.1.targets.1=cxl.1,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k
> -object memory-backend-ram,id=mem0,size=2048M
> -numa node,nodeid=0,memdev=mem0,
> -numa cpu,node-id=0,socket-id=0
> -object memory-backend-ram,id=mem1,size=2048M
> -numa node,nodeid=1,memdev=mem1,
> -numa cpu,node-id=1,socket-id=1
> -object memory-backend-ram,id=mem2,size=2048M
> -numa node,nodeid=2,memdev=mem2,
> -object memory-backend-ram,id=mem3,size=2048M
> -numa node,nodeid=3,memdev=mem3,
> -numa node,nodeid=4,
> -object memory-backend-file,id=nvmem0,share=on,mem-path=nvdimm-0,size=16384M,align=1G
> -device nvdimm,memdev=nvmem0,id=nv0,label-size=2M,node=4
> -numa node,nodeid=5,
> -object memory-backend-file,id=nvmem1,share=on,mem-path=nvdimm-1,size=16384M,align=1G
> -device nvdimm,memdev=nvmem1,id=nv1,label-size=2M,node=5
> -numa dist,src=0,dst=0,val=10
> -numa dist,src=0,dst=1,val=21
> -numa dist,src=0,dst=2,val=12
> -numa dist,src=0,dst=3,val=21
> -numa dist,src=0,dst=4,val=17
> -numa dist,src=0,dst=5,val=28
> -numa dist,src=1,dst=1,val=10
> -numa dist,src=1,dst=2,val=21
> -numa dist,src=1,dst=3,val=12
> -numa dist,src=1,dst=4,val=28
> -numa dist,src=1,dst=5,val=17
> -numa dist,src=2,dst=2,val=10
> -numa dist,src=2,dst=3,val=21
> -numa dist,src=2,dst=4,val=28
> -numa dist,src=2,dst=5,val=28
> -numa dist,src=3,dst=3,val=10
> -numa dist,src=3,dst=4,val=28
> -numa dist,src=3,dst=5,val=28
> -numa dist,src=4,dst=4,val=10
> -numa dist,src=4,dst=5,val=28
> -numa dist,src=5,dst=5,val=10
>

[-- Attachment #1.1.2: Type: text/html, Size: 7399 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Jonathan Cameron]

On Fri, 10 Feb 2023 11:15:44 +0100
Brice Goglin <Brice.Goglin@inria.fr> wrote:

> Le 09/02/2023 à 20:17, Verma, Vishal L a écrit :
> > On Thu, 2023-02-09 at 12:04 +0100, Brice Goglin wrote:  
> >> Hello Vishal
> >>
> >> I am trying to play with this but all my attempts failed so far. Could
> >> you provide Qemu and cxl-cli command-lines to get a volatile region
> >> enabled in a Qemu VM?  
> > Hi Brice,
> >
> > Greg had posted his working config in another thread:
> > https://lore.kernel.org/linux-cxl/Y9sMs0FGulQSIe9t@memverge.com/
> >
> > I've also pasted below, the qemu command line generated by the run_qemu
> > script I referenced. (Note that this adds a bunch of stuff not strictly
> > needed for a minimal CXL configuration - you can certainly trim a lot
> > of that out - this is just the default setup that is generated and I
> > usually run).  
> 
> 
> Hello Vishal
> 
> Thanks a lot, things were failing because my kernel didn't have
> CONFIG_CXL_REGION_INVALIDATION_TEST=y. Now I am able to create a single
> ram region, either with a single device or multiple interleaved ones.
> 
> However I can't get multiple separate ram regions. If I boot a config
> like yours below, I get 4 ram devices. How can I create one region for
> each? Once I create the first one, others fail saying something like
> below. I tried using other decoders but it didn't help (I still need
> to read more CXL docs about decoders, why new ones appear when creating
> a region, etc).
> 
> cxl region: collect_memdevs: no active memdevs found: decoder: decoder0.0 filter: mem3

Hi Brice,

QEMU emulation currently only supports single HDM decoder at each level,
so HB, Switch USP, EP (with exception of the CFMWS top level ones as shown
in the example which has two of those). We should fix that...

For now, you should be able to do it with multiple pxb-cxl instances with
appropriate CFMWS entries for each one. Which is horrible but might work
for you in the meantime.  

> 
> By the way, once configured in system ram, my CXL ram is merged into an
> existing "normal" NUMA node. How do I tell Qemu that a CXL region should
> be part of a new NUMA node? I assume that's what's going to happen on
> real hardware?

We don't yet have kernel code to deal with assigning a new NUMA node.
Was on the todo list in last sync call I think.

> 
> Thanks
> 
> Brice
> 
> 
> 
> >
> >
> > $ run_qemu.sh -g --cxl --cxl-debug --rw -r none --cmdline
> > /home/vverma7/git/qemu/build/qemu-system-x86_64
> > -machine q35,accel=kvm,nvdimm=on,cxl=on
> > -m 8192M,slots=4,maxmem=40964M
> > -smp 8,sockets=2,cores=2,threads=2
> > -enable-kvm
> > -display none
> > -nographic
> > -drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly=on
> > -drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd
> > -debugconfile:uefi_debug.log  
> > -global isa-debugcon.iobase=0x402
> > -drive file=root.img,format=raw,media=disk
> > -kernel ./mkosi.extra/lib/modules/6.2.0-rc6+/vmlinuz
> > -initrd mkosi.extra/boot/initramfs-6.2.0-rc2+.img
> > -append selinux=0 audit=0 console=tty0 console=ttyS0 root=/dev/sda2 ignore_loglevel rw cxl_acpi.dyndbg=+fplm cxl_pci.dyndbg=+fplm cxl_core.dyndbg=+fplm cxl_mem.dyndbg=+fplm cxl_pmem.dyndbg=+fplm cxl_port.dyndbg=+fplm cxl_region.dyndbg=+fplm cxl_test.dyndbg=+fplm cxl_mock.dyndbg=+fplm cxl_mock_mem.dyndbg=+fplm memmap=2G!4G efi_fake_mem=2G@6G:0x40000
> > -device e1000,netdev=net0,mac=52:54:00:12:34:56
> > -netdev user,id=net0,hostfwd=tcp::10022-:22
> > -object memory-backend-file,id=cxl-mem0,share=on,mem-path=cxltest0.raw,size=256M
> > -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxltest1.raw,size=256M
> > -object memory-backend-file,id=cxl-mem2,share=on,mem-path=cxltest2.raw,size=256M
> > -object memory-backend-file,id=cxl-mem3,share=on,mem-path=cxltest3.raw,size=256M
> > -object memory-backend-ram,id=cxl-mem4,share=on,size=256M
> > -object memory-backend-ram,id=cxl-mem5,share=on,size=256M
> > -object memory-backend-ram,id=cxl-mem6,share=on,size=256M
> > -object memory-backend-ram,id=cxl-mem7,share=on,size=256M
> > -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=lsa0.raw,size=1K
> > -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=lsa1.raw,size=1K
> > -object memory-backend-file,id=cxl-lsa2,share=on,mem-path=lsa2.raw,size=1K
> > -object memory-backend-file,id=cxl-lsa3,share=on,mem-path=lsa3.raw,size=1K
> > -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=53
> > -device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=191
> > -device cxl-rp,id=hb0rp0,bus=cxl.0,chassis=0,slot=0,port=0
> > -device cxl-rp,id=hb0rp1,bus=cxl.0,chassis=0,slot=1,port=1
> > -device cxl-rp,id=hb0rp2,bus=cxl.0,chassis=0,slot=2,port=2
> > -device cxl-rp,id=hb0rp3,bus=cxl.0,chassis=0,slot=3,port=3
> > -device cxl-rp,id=hb1rp0,bus=cxl.1,chassis=0,slot=4,port=0
> > -device cxl-rp,id=hb1rp1,bus=cxl.1,chassis=0,slot=5,port=1
> > -device cxl-rp,id=hb1rp2,bus=cxl.1,chassis=0,slot=6,port=2
> > -device cxl-rp,id=hb1rp3,bus=cxl.1,chassis=0,slot=7,port=3
> > -device cxl-type3,bus=hb0rp0,memdev=cxl-mem0,id=cxl-dev0,lsa=cxl-lsa0
> > -device cxl-type3,bus=hb0rp1,memdev=cxl-mem1,id=cxl-dev1,lsa=cxl-lsa1
> > -device cxl-type3,bus=hb1rp0,memdev=cxl-mem2,id=cxl-dev2,lsa=cxl-lsa2
> > -device cxl-type3,bus=hb1rp1,memdev=cxl-mem3,id=cxl-dev3,lsa=cxl-lsa3
> > -device cxl-type3,bus=hb0rp2,volatile-memdev=cxl-mem4,id=cxl-dev4
> > -device cxl-type3,bus=hb0rp3,volatile-memdev=cxl-mem5,id=cxl-dev5
> > -device cxl-type3,bus=hb1rp2,volatile-memdev=cxl-mem6,id=cxl-dev6
> > -device cxl-type3,bus=hb1rp3,volatile-memdev=cxl-mem7,id=cxl-dev7
> > -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,cxl-fmw.1.targets.0=cxl.0,cxl-fmw.1.targets.1=cxl.1,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k
> > -object memory-backend-ram,id=mem0,size=2048M
> > -numa node,nodeid=0,memdev=mem0,
> > -numa cpu,node-id=0,socket-id=0
> > -object memory-backend-ram,id=mem1,size=2048M
> > -numa node,nodeid=1,memdev=mem1,
> > -numa cpu,node-id=1,socket-id=1
> > -object memory-backend-ram,id=mem2,size=2048M
> > -numa node,nodeid=2,memdev=mem2,
> > -object memory-backend-ram,id=mem3,size=2048M
> > -numa node,nodeid=3,memdev=mem3,
> > -numa node,nodeid=4,
> > -object memory-backend-file,id=nvmem0,share=on,mem-path=nvdimm-0,size=16384M,align=1G
> > -device nvdimm,memdev=nvmem0,id=nv0,label-size=2M,node=4
> > -numa node,nodeid=5,
> > -object memory-backend-file,id=nvmem1,share=on,mem-path=nvdimm-1,size=16384M,align=1G
> > -device nvdimm,memdev=nvmem1,id=nv1,label-size=2M,node=5
> > -numa dist,src=0,dst=0,val=10
> > -numa dist,src=0,dst=1,val=21
> > -numa dist,src=0,dst=2,val=12
> > -numa dist,src=0,dst=3,val=21
> > -numa dist,src=0,dst=4,val=17
> > -numa dist,src=0,dst=5,val=28
> > -numa dist,src=1,dst=1,val=10
> > -numa dist,src=1,dst=2,val=21
> > -numa dist,src=1,dst=3,val=12
> > -numa dist,src=1,dst=4,val=28
> > -numa dist,src=1,dst=5,val=17
> > -numa dist,src=2,dst=2,val=10
> > -numa dist,src=2,dst=3,val=21
> > -numa dist,src=2,dst=4,val=28
> > -numa dist,src=2,dst=5,val=28
> > -numa dist,src=3,dst=3,val=10
> > -numa dist,src=3,dst=4,val=28
> > -numa dist,src=3,dst=5,val=28
> > -numa dist,src=4,dst=4,val=10
> > -numa dist,src=4,dst=5,val=28
> > -numa dist,src=5,dst=5,val=10
> >  

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

[Brice Goglin]

[-- Attachment #1.1: Type: text/plain, Size: 2278 bytes --]

Le 10/02/2023 à 13:43, Jonathan Cameron a écrit :
>
>> Hello Vishal
>>
>> Thanks a lot, things were failing because my kernel didn't have
>> CONFIG_CXL_REGION_INVALIDATION_TEST=y. Now I am able to create a single
>> ram region, either with a single device or multiple interleaved ones.
>>
>> However I can't get multiple separate ram regions. If I boot a config
>> like yours below, I get 4 ram devices. How can I create one region for
>> each? Once I create the first one, others fail saying something like
>> below. I tried using other decoders but it didn't help (I still need
>> to read more CXL docs about decoders, why new ones appear when creating
>> a region, etc).
>>
>> cxl region: collect_memdevs: no active memdevs found: decoder: decoder0.0 filter: mem3
> Hi Brice,
>
> QEMU emulation currently only supports single HDM decoder at each level,
> so HB, Switch USP, EP (with exception of the CFMWS top level ones as shown
> in the example which has two of those). We should fix that...
>
> For now, you should be able to do it with multiple pxb-cxl instances with
> appropriate CFMWS entries for each one. Which is horrible but might work
> for you in the meantime.


Thanks Jonathan, this works fine:

   -object memory-backend-ram,id=vmem0,share=on,size=256M \
   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
   -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
   -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,id=cxl-vmem0 \
   -object memory-backend-ram,id=vmem1,share=on,size=256M \
   -device pxb-cxl,bus_nr=14,bus=pcie.0,id=cxl.2 \
   -device cxl-rp,port=0,bus=cxl.2,id=root_port14,chassis=1,slot=2 \
   -device cxl-type3,bus=root_port14,volatile-memdev=vmem1,id=cxl-vmem1 \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.1.targets.0=cxl.2,cxl-fmw.1.size=4G


>> By the way, once configured in system ram, my CXL ram is merged into an
>> existing "normal" NUMA node. How do I tell Qemu that a CXL region should
>> be part of a new NUMA node? I assume that's what's going to happen on
>> real hardware?
> We don't yet have kernel code to deal with assigning a new NUMA node.
> Was on the todo list in last sync call I think.


Good to known, thanks again.

Brice



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Dan Williams]

Brice Goglin wrote:
[..]
> >> By the way, once configured in system ram, my CXL ram is merged into an
> >> existing "normal" NUMA node. How do I tell Qemu that a CXL region should
> >> be part of a new NUMA node? I assume that's what's going to happen on
> >> real hardware?
> > We don't yet have kernel code to deal with assigning a new NUMA node.
> > Was on the todo list in last sync call I think.
> 
> 
> Good to known, thanks again.

In fact, there is no plan to support "new" NUMA node creation. A node
can only be onlined / populated from set of static nodes defined by
platform-firmware. The set of static nodes is defined by the union of
all the proximity domain numbers in the SRAT as well as a node per
CFMWS / QTG id. See:

    fd49f99c1809 ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT

...for the CXL node enumeration scheme.

Once you have a node per CFMWS then it is up to CDAT and the QTG DSM to
group devices by window. This scheme attempts to be as simple as
possible, but no simpler. If more granularity is necessary in practice,
that would be a good discussion to have soonish.. LSF/MM comes to mind.

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

[Brice Goglin]

[-- Attachment #1.1: Type: text/plain, Size: 1627 bytes --]

Le 11/02/2023 à 02:53, Dan Williams a écrit :

> Brice Goglin wrote:
> [..]
>>>> By the way, once configured in system ram, my CXL ram is merged into an
>>>> existing "normal" NUMA node. How do I tell Qemu that a CXL region should
>>>> be part of a new NUMA node? I assume that's what's going to happen on
>>>> real hardware?
>>> We don't yet have kernel code to deal with assigning a new NUMA node.
>>> Was on the todo list in last sync call I think.
>>
> In fact, there is no plan to support "new" NUMA node creation. A node
> can only be onlined / populated from set of static nodes defined by
> platform-firmware. The set of static nodes is defined by the union of
> all the proximity domain numbers in the SRAT as well as a node per
> CFMWS / QTG id. See:
>
>      fd49f99c1809 ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT
>
> ...for the CXL node enumeration scheme.
>
> Once you have a node per CFMWS then it is up to CDAT and the QTG DSM to
> group devices by window. This scheme attempts to be as simple as
> possible, but no simpler. If more granularity is necessary in practice,
> that would be a good discussion to have soonish.. LSF/MM comes to mind.

Actually I was mistaken, there's already a new NUMA node when creating
a region under Qemu, but my tools ignored it because it's empty.
After daxctl online-memory, things look good.

Can you clarify your above sentences on a real node? If I connect two
memory expanders on two slots of the same CPU, do I get a single CFMWS or two?
What if I connect two devices to a single slot across a CXL switch?

Brice


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Dan Williams]

Brice Goglin wrote:
> Le 11/02/2023 à 02:53, Dan Williams a écrit :
> 
> > Brice Goglin wrote:
> > [..]
> >>>> By the way, once configured in system ram, my CXL ram is merged into an
> >>>> existing "normal" NUMA node. How do I tell Qemu that a CXL region should
> >>>> be part of a new NUMA node? I assume that's what's going to happen on
> >>>> real hardware?
> >>> We don't yet have kernel code to deal with assigning a new NUMA node.
> >>> Was on the todo list in last sync call I think.
> >>
> > In fact, there is no plan to support "new" NUMA node creation. A node
> > can only be onlined / populated from set of static nodes defined by
> > platform-firmware. The set of static nodes is defined by the union of
> > all the proximity domain numbers in the SRAT as well as a node per
> > CFMWS / QTG id. See:
> >
> >      fd49f99c1809 ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT
> >
> > ...for the CXL node enumeration scheme.
> >
> > Once you have a node per CFMWS then it is up to CDAT and the QTG DSM to
> > group devices by window. This scheme attempts to be as simple as
> > possible, but no simpler. If more granularity is necessary in practice,
> > that would be a good discussion to have soonish.. LSF/MM comes to mind.
> 
> Actually I was mistaken, there's already a new NUMA node when creating
> a region under Qemu, but my tools ignored it because it's empty.
> After daxctl online-memory, things look good.
> 
> Can you clarify your above sentences on a real node? If I connect two
> memory expanders on two slots of the same CPU, do I get a single CFMWS or two?
> What if I connect two devices to a single slot across a CXL switch?

Ultimately the answer is "ask your platform vendor", because this is a
firmware decision. However, my expectation is that since the ACPI HMAT
requires a proximity domain per distinct performance class, and because
the ACPI HMAT needs to distinguish the memory that is "attached" to a
CPU initiator domain, that CXL will at a minimum be described in a
proximity domain distinct from "local DRAM".

The number of CFMWS windows published is gated by the degrees of freedom
platform-firmware wants to give the OS relative to the number of CXL
host-bridges in the system. One scheme that seems plausible is one CFMWS
window for each host-bridge / x1 interleave (to maximize RAS) and one
CFMWS with all host-bridges interleaved together (to maximize
performance).

The above is just my personal opinion as a Linux kernel developer, a
platform implementation is free to be as restrictive or generous as it
wants with CFMWS resources.