[ndctl PATCH v10 0/7] Support poison list retrieval

[ndctl PATCH v10 0/7] Support poison list retrieval

[alison.schofield]

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

Changes since v9:
- Replace the multi-use 'name' var, with multiple descriptive
  flavors: memdev_name, region_name, decoder_name (DaveJ)
- Use a static string table for poison source lookup (DaveJ)
- Rebased on latest pending
Link to v9: https://lore.kernel.org/r/cover.1709253898.git.alison.schofield@intel.com/


Add the option to add a memory devices poison list to the cxl-list
json output. Offer the option by memdev and by region. Sample usage:

# cxl list -m mem1 --media-errors
[
  {
    "memdev":"mem1",
    "pmem_size":1073741824,
    "ram_size":1073741824,
    "serial":1,
    "numa_node":1,
    "host":"cxl_mem.1",
    "media_errors":[
      {
        "dpa":0,
        "length":64,
        "source":"Internal"
      },
      {
        "decoder":"decoder10.0",
        "hpa":1035355557888,
        "dpa":1073741824,
        "length":64,
        "source":"External"
      },
      {
        "decoder":"decoder10.0",
        "hpa":1035355566080,
        "dpa":1073745920,
        "length":64,
        "source":"Injected"
      }
    ]
  }
]

# cxl list -r region5 --media-errors
[
  {
    "region":"region5",
    "resource":1035355553792,
    "size":2147483648,
    "type":"pmem",
    "interleave_ways":2,
    "interleave_granularity":4096,
    "decode_state":"commit",
    "media_errors":[
      {
        "decoder":"decoder10.0",
        "hpa":1035355557888,
        "dpa":1073741824,
        "length":64,
        "source":"External"
      },
      {
        "decoder":"decoder8.1",
        "hpa":1035355566080,
        "dpa":1073745920,
        "length":64,
        "source":"Internal"
      }
    ]
  }
]

Alison Schofield (7):
  libcxl: add interfaces for GET_POISON_LIST mailbox commands
  cxl: add an optional pid check to event parsing
  cxl/event_trace: add a private context for private parsers
  cxl/event_trace: add helpers to retrieve tep fields by type
  cxl/list: collect and parse media_error records
  cxl/list: add --media-errors option to cxl list
  cxl/test: add cxl-poison.sh unit test

 Documentation/cxl/cxl-list.txt |  79 +++++++++-
 cxl/event_trace.c              |  82 ++++++++++-
 cxl/event_trace.h              |  14 +-
 cxl/filter.h                   |   3 +
 cxl/json.c                     | 257 +++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.c               |  47 ++++++
 cxl/lib/libcxl.sym             |   2 +
 cxl/libcxl.h                   |   2 +
 cxl/list.c                     |   3 +
 test/cxl-poison.sh             | 137 ++++++++++++++++++
 test/meson.build               |   2 +
 11 files changed, 624 insertions(+), 4 deletions(-)
 create mode 100644 test/cxl-poison.sh


base-commit: e0d0680bd3e554bd5f211e989480c5a13a023b2d
-- 
2.37.3

[ndctl PATCH v10 1/7] libcxl: add interfaces for GET_POISON_LIST mailbox commands

[alison.schofield]

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

CXL devices maintain a list of locations that are poisoned or result
in poison if the addresses are accessed by the host.

Per the spec (CXL 3.1 8.2.9.9.4.1), the device returns the Poison
List as a set of  Media Error Records that include the source of the
error, the starting device physical address and length.

Trigger the retrieval of the poison list by writing to the memory
device sysfs attribute: trigger_poison_list. The CXL driver only
offers triggering per memdev, so the trigger by region interface
offered here is a convenience API that triggers a poison list
retrieval for each memdev contributing to a region.

int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev);
int cxl_region_trigger_poison_list(struct cxl_region *region);

The resulting poison records are logged as kernel trace events
named 'cxl_poison'.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/lib/libcxl.c   | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  2 ++
 cxl/libcxl.h       |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index ff27cdf7c44a..73db8f15c704 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1761,6 +1761,53 @@ CXL_EXPORT int cxl_memdev_disable_invalidate(struct cxl_memdev *memdev)
 	return 0;
 }
 
+CXL_EXPORT int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev)
+{
+	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
+	char *path = memdev->dev_buf;
+	int len = memdev->buf_len, rc;
+
+	if (snprintf(path, len, "%s/trigger_poison_list",
+		     memdev->dev_path) >= len) {
+		err(ctx, "%s: buffer too small\n",
+		    cxl_memdev_get_devname(memdev));
+		return -ENXIO;
+	}
+	rc = sysfs_write_attr(ctx, path, "1\n");
+	if (rc < 0) {
+		fprintf(stderr,
+			"%s: Failed write sysfs attr trigger_poison_list\n",
+			cxl_memdev_get_devname(memdev));
+		return rc;
+	}
+	return 0;
+}
+
+CXL_EXPORT int cxl_region_trigger_poison_list(struct cxl_region *region)
+{
+	struct cxl_memdev_mapping *mapping;
+	int rc;
+
+	cxl_mapping_foreach(region, mapping) {
+		struct cxl_decoder *decoder;
+		struct cxl_memdev *memdev;
+
+		decoder = cxl_mapping_get_decoder(mapping);
+		if (!decoder)
+			continue;
+
+		memdev = cxl_decoder_get_memdev(decoder);
+		if (!memdev)
+			continue;
+
+		rc = cxl_memdev_trigger_poison_list(memdev);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 CXL_EXPORT int cxl_memdev_enable(struct cxl_memdev *memdev)
 {
 	struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index de2cd84b2960..3f709c60db3d 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -280,4 +280,6 @@ global:
 	cxl_memdev_get_pmem_qos_class;
 	cxl_memdev_get_ram_qos_class;
 	cxl_region_qos_class_mismatch;
+	cxl_memdev_trigger_poison_list;
+	cxl_region_trigger_poison_list;
 } LIBCXL_6;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index a6af3fb04693..29165043ca3f 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -467,6 +467,8 @@ enum cxl_setpartition_mode {
 
 int cxl_cmd_partition_set_mode(struct cxl_cmd *cmd,
 		enum cxl_setpartition_mode mode);
+int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev);
+int cxl_region_trigger_poison_list(struct cxl_region *region);
 
 int cxl_cmd_alert_config_set_life_used_prog_warn_threshold(struct cxl_cmd *cmd,
 							   int threshold);
-- 
2.37.3

Re: [ndctl PATCH v10 1/7] libcxl: add interfaces for GET_POISON_LIST mailbox commands

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> CXL devices maintain a list of locations that are poisoned or result
> in poison if the addresses are accessed by the host.
> 
> Per the spec (CXL 3.1 8.2.9.9.4.1), the device returns the Poison
> List as a set of  Media Error Records that include the source of the
> error, the starting device physical address and length.
> 
> Trigger the retrieval of the poison list by writing to the memory
> device sysfs attribute: trigger_poison_list. The CXL driver only
> offers triggering per memdev, so the trigger by region interface
> offered here is a convenience API that triggers a poison list
> retrieval for each memdev contributing to a region.
> 
> int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev);
> int cxl_region_trigger_poison_list(struct cxl_region *region);
> 
> The resulting poison records are logged as kernel trace events
> named 'cxl_poison'.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Looks good,

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

[ndctl PATCH v10 2/7] cxl: add an optional pid check to event parsing

[alison.schofield]

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

When parsing CXL events, callers may only be interested in events
that originate from the current process. Introduce an optional
argument to the event trace context: event_pid. When event_pid is
present, simply skip the parsing of events without a matching pid.
It is not a failure to see other, non matching events.

The initial use case for this is device poison listings where
only the media-error records requested by this process are wanted.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c | 5 +++++
 cxl/event_trace.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index 1b5aa09de8b2..93a95f9729fd 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -214,6 +214,11 @@ static int cxl_event_parse(struct tep_event *event, struct tep_record *record,
 			return 0;
 	}
 
+	if (event_ctx->event_pid) {
+		if (event_ctx->event_pid != tep_data_pid(event->tep, record))
+			return 0;
+	}
+
 	if (event_ctx->parse_event)
 		return event_ctx->parse_event(event, record,
 					      &event_ctx->jlist_head);
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index ec6267202c8b..7f7773b2201f 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -15,6 +15,7 @@ struct event_ctx {
 	const char *system;
 	struct list_head jlist_head;
 	const char *event_name; /* optional */
+	int event_pid; /* optional */
 	int (*parse_event)(struct tep_event *event, struct tep_record *record,
 			   struct list_head *jlist_head); /* optional */
 };
-- 
2.37.3

Re: [ndctl PATCH v10 2/7] cxl: add an optional pid check to event parsing

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> When parsing CXL events, callers may only be interested in events
> that originate from the current process. Introduce an optional
> argument to the event trace context: event_pid. When event_pid is
> present, simply skip the parsing of events without a matching pid.
> It is not a failure to see other, non matching events.
> 
> The initial use case for this is device poison listings where
> only the media-error records requested by this process are wanted.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Looks good,

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

[ndctl PATCH v10 3/7] cxl/event_trace: add a private context for private parsers

[alison.schofield]

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

CXL event tracing provides helpers to iterate through a trace
buffer and extract events of interest. It offers two parsing
options: a default parser that adds every field of an event to
a json object, and a private parsing option where the caller can
parse each event as it wishes.

Although the private parser can do some conditional parsing based
on field values, it has no method to receive additional information
needed to make parsing decisions in the callback.

Add a private_ctx field to the existing 'struct event_context'.
Replace the jlist_head parameter, used in the default parser,
with the private_ctx.

This is in preparation for adding a private parser requiring
additional context for cxl_poison events.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c | 2 +-
 cxl/event_trace.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index 93a95f9729fd..bdad0c19dbd4 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -221,7 +221,7 @@ static int cxl_event_parse(struct tep_event *event, struct tep_record *record,
 
 	if (event_ctx->parse_event)
 		return event_ctx->parse_event(event, record,
-					      &event_ctx->jlist_head);
+					      event_ctx->private_ctx);
 
 	return cxl_event_to_json(event, record, &event_ctx->jlist_head);
 }
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 7f7773b2201f..ec61962abbc6 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -16,8 +16,9 @@ struct event_ctx {
 	struct list_head jlist_head;
 	const char *event_name; /* optional */
 	int event_pid; /* optional */
+	void *private_ctx; /* required with parse_event() */
 	int (*parse_event)(struct tep_event *event, struct tep_record *record,
-			   struct list_head *jlist_head); /* optional */
+			   void *private_ctx);/* optional */
 };
 
 int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
-- 
2.37.3

Re: [ndctl PATCH v10 3/7] cxl/event_trace: add a private context for private parsers

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> CXL event tracing provides helpers to iterate through a trace
> buffer and extract events of interest. It offers two parsing
> options: a default parser that adds every field of an event to
> a json object, and a private parsing option where the caller can
> parse each event as it wishes.
> 
> Although the private parser can do some conditional parsing based
> on field values, it has no method to receive additional information
> needed to make parsing decisions in the callback.
> 
> Add a private_ctx field to the existing 'struct event_context'.
> Replace the jlist_head parameter, used in the default parser,
> with the private_ctx.
> 
> This is in preparation for adding a private parser requiring
> additional context for cxl_poison events.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c | 2 +-
>  cxl/event_trace.h | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index 93a95f9729fd..bdad0c19dbd4 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -221,7 +221,7 @@ static int cxl_event_parse(struct tep_event *event, struct tep_record *record,
>  
>  	if (event_ctx->parse_event)
>  		return event_ctx->parse_event(event, record,
> -					      &event_ctx->jlist_head);
> +					      event_ctx->private_ctx);

Given ->parse_event() is already a method of an event_ctx object, might
as will pass the entirety of event_ctx to its own method as a typical
'this' pointer.

You could then also use container_of() to get to event_ctx creator data
and skip the type-unsafety of a 'void *' pointer. However, I say that
without having looked to see how feasible it is to wrap private data
around an event_ctx instance.

Re: [ndctl PATCH v10 3/7] cxl/event_trace: add a private context for private parsers

[Alison Schofield]

On Wed, Mar 06, 2024 at 03:36:32PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > CXL event tracing provides helpers to iterate through a trace
> > buffer and extract events of interest. It offers two parsing
> > options: a default parser that adds every field of an event to
> > a json object, and a private parsing option where the caller can
> > parse each event as it wishes.
> > 
> > Although the private parser can do some conditional parsing based
> > on field values, it has no method to receive additional information
> > needed to make parsing decisions in the callback.
> > 
> > Add a private_ctx field to the existing 'struct event_context'.
> > Replace the jlist_head parameter, used in the default parser,
> > with the private_ctx.
> > 
> > This is in preparation for adding a private parser requiring
> > additional context for cxl_poison events.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  cxl/event_trace.c | 2 +-
> >  cxl/event_trace.h | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> > index 93a95f9729fd..bdad0c19dbd4 100644
> > --- a/cxl/event_trace.c
> > +++ b/cxl/event_trace.c
> > @@ -221,7 +221,7 @@ static int cxl_event_parse(struct tep_event *event, struct tep_record *record,
> >  
> >  	if (event_ctx->parse_event)
> >  		return event_ctx->parse_event(event, record,
> > -					      &event_ctx->jlist_head);
> > +					      event_ctx->private_ctx);
> 
> Given ->parse_event() is already a method of an event_ctx object, might
> as will pass the entirety of event_ctx to its own method as a typical
> 'this' pointer.

Thanks, done!
Now passing event_ctx struct as a param to its parse_event method.

> 
> You could then also use container_of() to get to event_ctx creator data
> and skip the type-unsafety of a 'void *' pointer. However, I say that
> without having looked to see how feasible it is to wrap private data
> around an event_ctx instance.

Got rid of the void but didn't go down above path.

First off, I don't see need to find an event_ctx field from private_ctx.
I can see the use, but not the need. There are 2 users presently, one with
no private data and one with private data (cxl_poison).

If there were multiple users, or multiple users in sight, adding a flexible
array member to a private context struct and using that in container_of to
find the event_ctx might be useful. 

Next rev simply includes the poison_ctx directly in the event_ctx,
avoiding the void ptr usage.

Please take a look at next rev.

Thanks,
Alison

[ndctl PATCH v10 4/7] cxl/event_trace: add helpers to retrieve tep fields by type

[alison.schofield]

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

Add helpers to extract the value of an event record field given the
field name. This is useful when the user knows the name and format
of the field and simply needs to get it.

Since this is in preparation for adding a cxl_poison private parser
for 'cxl list --media-errors' support, add those specific required
types: u8, u32, u64, char*

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 cxl/event_trace.h | 10 ++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index bdad0c19dbd4..6cc9444f3204 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -15,6 +15,81 @@
 #define _GNU_SOURCE
 #include <string.h>
 
+static struct tep_format_field *__find_field(struct tep_event *event,
+					     const char *name)
+{
+	struct tep_format_field **fields;
+
+	fields = tep_event_fields(event);
+	if (!fields)
+		return NULL;
+
+	for (int i = 0; fields[i]; i++) {
+		struct tep_format_field *f = fields[i];
+
+		if (strcmp(f->name, name) != 0)
+			continue;
+
+		return f;
+	}
+	return NULL;
+}
+
+u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
+		      const char *name)
+{
+	struct tep_format_field *f;
+	unsigned char *val;
+	int len;
+
+	f = __find_field(event, name);
+	if (!f)
+		return ULLONG_MAX;
+
+	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+	if (!val)
+		return ULLONG_MAX;
+
+	return *(u64 *)val;
+}
+
+char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
+			   const char *name)
+{
+	struct tep_format_field *f;
+	int len;
+
+	f = __find_field(event, name);
+	if (!f)
+		return NULL;
+
+	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+}
+
+u32 cxl_get_field_u32(struct tep_event *event, struct tep_record *record,
+		      const char *name)
+{
+	char *val;
+
+	val = cxl_get_field_string(event, record, name);
+	if (!val)
+		return UINT_MAX;
+
+	return *(u32 *)val;
+}
+
+u8 cxl_get_field_u8(struct tep_event *event, struct tep_record *record,
+		    const char *name)
+{
+	char *val;
+
+	val = cxl_get_field_string(event, record, name);
+	if (!val)
+		return UCHAR_MAX;
+
+	return *(u8 *)val;
+}
+
 static struct json_object *num_to_json(void *num, int elem_size, unsigned long flags)
 {
 	bool sign = flags & TEP_FIELD_IS_SIGNED;
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index ec61962abbc6..bbdea3b896e0 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -5,6 +5,7 @@
 
 #include <json-c/json.h>
 #include <ccan/list/list.h>
+#include <ccan/short_types/short_types.h>
 
 struct jlist_node {
 	struct json_object *jobj;
@@ -25,5 +26,12 @@ int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
 int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system,
 		const char *event);
 int cxl_event_tracing_disable(struct tracefs_instance *inst);
-
+char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
+			   const char *name);
+u8 cxl_get_field_u8(struct tep_event *event, struct tep_record *record,
+		    const char *name);
+u32 cxl_get_field_u32(struct tep_event *event, struct tep_record *record,
+		      const char *name);
+u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
+		      const char *name);
 #endif
-- 
2.37.3

Re: [ndctl PATCH v10 4/7] cxl/event_trace: add helpers to retrieve tep fields by type

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add helpers to extract the value of an event record field given the
> field name. This is useful when the user knows the name and format
> of the field and simply needs to get it.
> 
> Since this is in preparation for adding a cxl_poison private parser
> for 'cxl list --media-errors' support, add those specific required
> types: u8, u32, u64, char*
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/event_trace.h | 10 ++++++-
>  2 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index bdad0c19dbd4..6cc9444f3204 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -15,6 +15,81 @@
>  #define _GNU_SOURCE
>  #include <string.h>
>  
> +static struct tep_format_field *__find_field(struct tep_event *event,
> +					     const char *name)
> +{
> +	struct tep_format_field **fields;
> +
> +	fields = tep_event_fields(event);
> +	if (!fields)
> +		return NULL;
> +
> +	for (int i = 0; fields[i]; i++) {
> +		struct tep_format_field *f = fields[i];
> +
> +		if (strcmp(f->name, name) != 0)
> +			continue;
> +
> +		return f;
> +	}
> +	return NULL;
> +}

Is this open-coded tep_find_field()?

> +
> +u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
> +		      const char *name)
> +{
> +	struct tep_format_field *f;
> +	unsigned char *val;
> +	int len;
> +
> +	f = __find_field(event, name);
> +	if (!f)
> +		return ULLONG_MAX;
> +
> +	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +	if (!val)
> +		return ULLONG_MAX;
> +
> +	return *(u64 *)val;
> +}

Is this just open-coded tep_get_any_field_val()?

> +
> +char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
> +			   const char *name)

Return a 'const char *'?

> +{
> +	struct tep_format_field *f;
> +	int len;
> +
> +	f = __find_field(event, name);
> +	if (!f)
> +		return NULL;
> +
> +	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);

Is this guaranteed to be a string? ...and guaranteed to be NULL
terminated?

Re: [ndctl PATCH v10 4/7] cxl/event_trace: add helpers to retrieve tep fields by type

[Alison Schofield]

On Wed, Mar 06, 2024 at 03:53:57PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add helpers to extract the value of an event record field given the
> > field name. This is useful when the user knows the name and format
> > of the field and simply needs to get it.
> > 
> > Since this is in preparation for adding a cxl_poison private parser
> > for 'cxl list --media-errors' support, add those specific required
> > types: u8, u32, u64, char*
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
> >  cxl/event_trace.h | 10 ++++++-
> >  2 files changed, 84 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> > index bdad0c19dbd4..6cc9444f3204 100644
> > --- a/cxl/event_trace.c
> > +++ b/cxl/event_trace.c
> > @@ -15,6 +15,81 @@
> >  #define _GNU_SOURCE
> >  #include <string.h>
> >  
> > +static struct tep_format_field *__find_field(struct tep_event *event,
> > +					     const char *name)
> > +{
> > +	struct tep_format_field **fields;
> > +
> > +	fields = tep_event_fields(event);
> > +	if (!fields)
> > +		return NULL;
> > +
> > +	for (int i = 0; fields[i]; i++) {
> > +		struct tep_format_field *f = fields[i];
> > +
> > +		if (strcmp(f->name, name) != 0)
> > +			continue;
> > +
> > +		return f;
> > +	}
> > +	return NULL;
> > +}
> 
> Is this open-coded tep_find_field()?

Yes it is and now it is gone.

> 
> > +
> > +u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
> > +		      const char *name)
> > +{
> > +	struct tep_format_field *f;
> > +	unsigned char *val;
> > +	int len;
> > +
> > +	f = __find_field(event, name);
> > +	if (!f)
> > +		return ULLONG_MAX;
> > +
> > +	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> > +	if (!val)
> > +		return ULLONG_MAX;
> > +
> > +	return *(u64 *)val;
> > +}
> 
> Is this just open-coded tep_get_any_field_val()?

It's a bit more. It returns ULLONG_MAX and casts to the u64 which
makes the call site cleaner. 

I did change it to use tep_get_field_val(). Please look at next rev.

> 
> > +
> > +char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
> > +			   const char *name)
> 
> Return a 'const char *'?
> 
> > +{
> > +	struct tep_format_field *f;
> > +	int len;
> > +
> > +	f = __find_field(event, name);
> > +	if (!f)
> > +		return NULL;
> > +
> > +	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> 
> Is this guaranteed to be a string? ...and guaranteed to be NULL
> terminated?
>
It's gone. Using tep_get_field_raw() directly for str.

Thanks for reviewing!

[ndctl PATCH v10 5/7] cxl/list: collect and parse media_error records

[alison.schofield]

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

Media_error records are logged as events in the kernel tracing
subsystem. To prepare the media_error records for cxl list, enable
tracing, trigger the poison list read, and parse the generated
cxl_poison events into a json representation.

Use the event_trace private parsing option to customize the json
representation based on cxl-list calling options and event field
settings.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/json.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 257 insertions(+)

diff --git a/cxl/json.c b/cxl/json.c
index fbe41c78e82a..435747de384d 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -1,16 +1,20 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2015-2021 Intel Corporation. All rights reserved.
 #include <limits.h>
+#include <errno.h>
 #include <util/json.h>
+#include <util/bitmap.h>
 #include <uuid/uuid.h>
 #include <cxl/libcxl.h>
 #include <json-c/json.h>
 #include <json-c/printbuf.h>
 #include <ccan/short_types/short_types.h>
+#include <tracefs/tracefs.h>
 
 #include "filter.h"
 #include "json.h"
 #include "../daxctl/json.h"
+#include "event_trace.h"
 
 #define CXL_FW_VERSION_STR_LEN	16
 #define CXL_FW_MAX_SLOTS	4
@@ -571,6 +575,247 @@ err_jobj:
 	return NULL;
 }
 
+/* CXL Spec 3.1 Table 8-140 Media Error Record */
+#define CXL_POISON_SOURCE_MAX 7
+static const char *poison_source[] = { "Unknown",  "External", "Internal",
+				       "Injected", "Reserved", "Reserved",
+				       "Reserved", "Vendor" };
+
+/* CXL Spec 3.1 Table 8-139 Get Poison List Output Payload */
+#define CXL_POISON_FLAG_MORE BIT(0)
+#define CXL_POISON_FLAG_OVERFLOW BIT(1)
+#define CXL_POISON_FLAG_SCANNING BIT(2)
+
+struct poison_ctx {
+	struct json_object *jpoison;
+	struct cxl_region *region;
+	struct cxl_memdev *memdev;
+	unsigned long flags;
+};
+
+static struct cxl_memdev *find_memdev(struct cxl_region *region,
+				      const char *memdev_name)
+{
+	struct cxl_memdev_mapping *mapping;
+	struct cxl_decoder *decoder;
+	struct cxl_memdev *memdev;
+
+	cxl_mapping_foreach(region, mapping)
+	{
+		decoder = cxl_mapping_get_decoder(mapping);
+		if (!decoder)
+			continue;
+
+		memdev = cxl_decoder_get_memdev(decoder);
+		if (strcmp(memdev_name, cxl_memdev_get_devname(memdev)) == 0)
+			break;
+
+		memdev = NULL;
+	}
+	return memdev;
+}
+
+static const char *find_decoder_name(struct poison_ctx *ctx,
+				     const char *memdev_name, u64 addr)
+{
+	struct cxl_memdev *memdev = ctx->memdev;
+	const char *decoder_name = NULL;
+	struct cxl_endpoint *endpoint;
+	struct cxl_decoder *decoder;
+	struct cxl_port *port;
+	u64 start, end;
+
+	if (!memdev)
+		memdev = find_memdev(ctx->region, memdev_name);
+
+	if (!memdev)
+		return NULL;
+
+	endpoint = cxl_memdev_get_endpoint(memdev);
+	port = cxl_endpoint_get_port(endpoint);
+
+	cxl_decoder_foreach(port, decoder) {
+		start =  cxl_decoder_get_resource(decoder);
+		end = start + cxl_decoder_get_size(decoder) - 1;
+		if (start <= addr && addr <= end) {
+			decoder_name = cxl_decoder_get_devname(decoder);
+			break;
+		}
+	}
+	return decoder_name;
+}
+
+static int poison_event_to_json(struct tep_event *event,
+				struct tep_record *record, void *ctx)
+{
+	struct poison_ctx *p_ctx = (struct poison_ctx *)ctx;
+	struct json_object *jobj, *jp, *jpoison = p_ctx->jpoison;
+	unsigned long flags = p_ctx->flags;
+	const char *decoder_name = NULL;
+	const char *region_name = NULL;
+	const char *memdev_name = NULL;
+	char flag_str[32] = { '\0' };
+	bool overflow = false;
+	u8 source, pflags;
+	u64 addr, ts;
+	u32 length;
+	char *str;
+
+	jp = json_object_new_object();
+	if (!jp)
+		return -ENOMEM;
+
+	/* Skip records not in this region when listing by region */
+	if (p_ctx->region)
+		region_name = cxl_region_get_devname(p_ctx->region);
+	if (region_name)
+		str = cxl_get_field_string(event, record, "region");
+
+	if ((region_name) && (strcmp(region_name, str) != 0)) {
+		json_object_put(jp);
+		return 0;
+	}
+
+	/* Include endpoint decoder name with hpa, when present */
+	addr = cxl_get_field_u64(event, record, "hpa");
+	if (addr != ULLONG_MAX) {
+		memdev_name = cxl_get_field_string(event, record, "memdev");
+		decoder_name = find_decoder_name(p_ctx, memdev_name, addr);
+	}
+	if (decoder_name) {
+		jobj = json_object_new_string(decoder_name);
+		if (jobj)
+			json_object_object_add(jp, "decoder", jobj);
+
+		jobj = util_json_object_hex(addr, flags);
+		if (jobj)
+			json_object_object_add(jp, "hpa", jobj);
+	}
+
+	addr = cxl_get_field_u64(event, record, "dpa");
+	jobj = util_json_object_hex(addr, flags);
+	if (jobj)
+		json_object_object_add(jp, "dpa", jobj);
+
+	length = cxl_get_field_u32(event, record, "dpa_length");
+	jobj = util_json_object_size(length, flags);
+	if (jobj)
+		json_object_object_add(jp, "length", jobj);
+
+	source = cxl_get_field_u8(event, record, "source");
+	if (source <= CXL_POISON_SOURCE_MAX)
+		jobj = json_object_new_string(poison_source[source]);
+	else
+		jobj = json_object_new_string("Reserved");
+
+	if (jobj)
+		json_object_object_add(jp, "source", jobj);
+
+	pflags = cxl_get_field_u8(event, record, "flags");
+	if (pflags && pflags < UCHAR_MAX) {
+		if (pflags & CXL_POISON_FLAG_MORE)
+			strcat(flag_str, "More,");
+		if (pflags & CXL_POISON_FLAG_SCANNING)
+			strcat(flag_str, "Scanning,");
+		if (pflags & CXL_POISON_FLAG_OVERFLOW) {
+			strcat(flag_str, "Overflow,");
+			overflow = true;
+		}
+		jobj = json_object_new_string(flag_str);
+		if (jobj)
+			json_object_object_add(jp, "flags", jobj);
+	}
+
+	if (overflow) {
+		ts = cxl_get_field_u64(event, record, "overflow_ts");
+		jobj = util_json_object_hex(ts, flags);
+		if (jobj)
+			json_object_object_add(jp, "overflow_t", jobj);
+	}
+	json_object_array_add(jpoison, jp);
+
+	return 0;
+}
+
+static struct json_object *
+util_cxl_poison_events_to_json(struct tracefs_instance *inst,
+			       struct poison_ctx *p_ctx)
+{
+	struct event_ctx ectx = {
+		.event_name = "cxl_poison",
+		.event_pid = getpid(),
+		.system = "cxl",
+		.private_ctx = p_ctx,
+		.parse_event = poison_event_to_json,
+	};
+	int rc = 0;
+
+	p_ctx->jpoison = json_object_new_array();
+	if (!p_ctx->jpoison)
+		return NULL;
+
+	rc = cxl_parse_events(inst, &ectx);
+	if (rc < 0) {
+		fprintf(stderr, "Failed to parse events: %d\n", rc);
+		goto put_jobj;
+	}
+	if (json_object_array_length(p_ctx->jpoison) == 0)
+		goto put_jobj;
+
+	return p_ctx->jpoison;
+
+put_jobj:
+	json_object_put(p_ctx->jpoison);
+	return NULL;
+}
+
+static struct json_object *
+util_cxl_poison_list_to_json(struct cxl_region *region,
+			     struct cxl_memdev *memdev,
+			     unsigned long flags)
+{
+	struct json_object *jpoison = NULL;
+	struct poison_ctx p_ctx;
+	struct tracefs_instance *inst;
+	int rc;
+
+	inst = tracefs_instance_create("cxl list");
+	if (!inst) {
+		fprintf(stderr, "tracefs_instance_create() failed\n");
+		return NULL;
+	}
+
+	rc = cxl_event_tracing_enable(inst, "cxl", "cxl_poison");
+	if (rc < 0) {
+		fprintf(stderr, "Failed to enable trace: %d\n", rc);
+		goto err_free;
+	}
+
+	if (region)
+		rc = cxl_region_trigger_poison_list(region);
+	else
+		rc = cxl_memdev_trigger_poison_list(memdev);
+	if (rc)
+		goto err_free;
+
+	rc = cxl_event_tracing_disable(inst);
+	if (rc < 0) {
+		fprintf(stderr, "Failed to disable trace: %d\n", rc);
+		goto err_free;
+	}
+
+	p_ctx = (struct poison_ctx) {
+		.region = region,
+		.memdev = memdev,
+		.flags = flags,
+	};
+	jpoison = util_cxl_poison_events_to_json(inst, &p_ctx);
+
+err_free:
+	tracefs_instance_free(inst);
+	return jpoison;
+}
+
 struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 		unsigned long flags)
 {
@@ -664,6 +909,12 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 			json_object_object_add(jdev, "firmware", jobj);
 	}
 
+	if (flags & UTIL_JSON_MEDIA_ERRORS) {
+		jobj = util_cxl_poison_list_to_json(NULL, memdev, flags);
+		if (jobj)
+			json_object_object_add(jdev, "media_errors", jobj);
+	}
+
 	json_object_set_userdata(jdev, memdev, NULL);
 	return jdev;
 }
@@ -1012,6 +1263,12 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 			json_object_object_add(jregion, "state", jobj);
 	}
 
+	if (flags & UTIL_JSON_MEDIA_ERRORS) {
+		jobj = util_cxl_poison_list_to_json(region, NULL, flags);
+		if (jobj)
+			json_object_object_add(jregion, "media_errors", jobj);
+	}
+
 	util_cxl_mappings_append_json(jregion, region, flags);
 
 	if (flags & UTIL_JSON_DAX) {
-- 
2.37.3

Re: [ndctl PATCH v10 5/7] cxl/list: collect and parse media_error records

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Media_error records are logged as events in the kernel tracing
> subsystem. To prepare the media_error records for cxl list, enable
> tracing, trigger the poison list read, and parse the generated
> cxl_poison events into a json representation.
> 
> Use the event_trace private parsing option to customize the json
> representation based on cxl-list calling options and event field
> settings.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/json.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 257 insertions(+)

This look clean to me, just the comment on translating dpa and hpa to
offset depending on which object the list is being parsed against.

Also the note below about using container_of() in ->parse_event() to get
back to private context data.

> 
> +static struct json_object *
> +util_cxl_poison_events_to_json(struct tracefs_instance *inst,
> +			       struct poison_ctx *p_ctx)
> +{
> +	struct event_ctx ectx = {
> +		.event_name = "cxl_poison",
> +		.event_pid = getpid(),
> +		.system = "cxl",
> +		.private_ctx = p_ctx,
> +		.parse_event = poison_event_to_json,
> +	};

Looks like you could definitely embed an event_ctx in poison_ctx and
skip the need for private_ctx in event_ctx.

[ndctl PATCH v10 6/7] cxl/list: add --media-errors option to cxl list

[alison.schofield]

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

The --media-errors option to 'cxl list' retrieves poison lists from
memory devices supporting the capability and displays the returned
media_error records in the cxl list json. This option can apply to
memdevs or regions.

Include media-errors in the -vvv verbose option.

Example usage in the Documentation/cxl/cxl-list.txt update.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/cxl/cxl-list.txt | 79 +++++++++++++++++++++++++++++++++-
 cxl/filter.h                   |  3 ++
 cxl/list.c                     |  3 ++
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index 838de4086678..5c20614ef579 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -415,6 +415,83 @@ OPTIONS
 --region::
 	Specify CXL region device name(s), or device id(s), to filter the listing.
 
+-L::
+--media-errors::
+	Include media-error information. The poison list is retrieved from the
+	device(s) and media_error records are added to the listing. Apply this
+	option to memdevs and regions where devices support the poison list
+	capability.
+
+	"decoder" and "hpa" are included when the media-error is in a mapped
+        address.
+
+	"source" will be one of: External, Internal, Injected, Vendor Specific,
+        or Unknown, as defined in CXL Specification v3.1 Table 8-140.
+
+----
+# cxl list -m mem1 --media-errors
+[
+  {
+    "memdev":"mem1",
+    "pmem_size":1073741824,
+    "ram_size":1073741824,
+    "serial":1,
+    "numa_node":1,
+    "host":"cxl_mem.1",
+    "media_errors":[
+      {
+        "dpa":0,
+        "length":64,
+        "source":"Internal"
+      },
+      {
+        "decoder":"decoder10.0",
+        "hpa":1035355557888,
+        "dpa":1073741824,
+        "length":64,
+        "source":"External"
+      },
+      {
+        "decoder":"decoder10.0",
+        "hpa":1035355566080,
+        "dpa":1073745920,
+        "length":64,
+        "source":"Injected"
+      }
+    ]
+  }
+]
+
+# cxl list -r region5 --media-errors
+[
+  {
+    "region":"region5",
+    "resource":1035355553792,
+    "size":2147483648,
+    "type":"pmem",
+    "interleave_ways":2,
+    "interleave_granularity":4096,
+    "decode_state":"commit",
+    "media_errors":[
+      {
+        "decoder":"decoder10.0",
+        "hpa":1035355557888,
+        "dpa":1073741824,
+        "length":64,
+        "source":"External"
+      },
+      {
+        "decoder":"decoder8.1",
+        "hpa":1035355553792,
+        "dpa":1073741824,
+        "length":64,
+        "source":"Internal"
+      }
+    ]
+  }
+]
+----
+
 -v::
 --verbose::
 	Increase verbosity of the output. This can be specified
@@ -431,7 +508,7 @@ OPTIONS
 	  devices with --idle.
 	- *-vvv*
 	  Everything *-vv* provides, plus enable
-	  --health and --partition.
+	  --health, --partition, --media-errors.
 
 --debug::
 	If the cxl tool was built with debug enabled, turn on debug
diff --git a/cxl/filter.h b/cxl/filter.h
index 3f65990f835a..956a46e0c7a9 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -30,6 +30,7 @@ struct cxl_filter_params {
 	bool fw;
 	bool alert_config;
 	bool dax;
+	bool media_errors;
 	int verbose;
 	struct log_ctx ctx;
 };
@@ -88,6 +89,8 @@ static inline unsigned long cxl_filter_to_flags(struct cxl_filter_params *param)
 		flags |= UTIL_JSON_ALERT_CONFIG;
 	if (param->dax)
 		flags |= UTIL_JSON_DAX | UTIL_JSON_DAX_DEVS;
+	if (param->media_errors)
+		flags |= UTIL_JSON_MEDIA_ERRORS;
 	return flags;
 }
 
diff --git a/cxl/list.c b/cxl/list.c
index 93ba51ef895c..0b25d78248d5 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -57,6 +57,8 @@ static const struct option options[] = {
 		    "include memory device firmware information"),
 	OPT_BOOLEAN('A', "alert-config", &param.alert_config,
 		    "include alert configuration information"),
+	OPT_BOOLEAN('L', "media-errors", &param.media_errors,
+		    "include media-error information "),
 	OPT_INCR('v', "verbose", &param.verbose, "increase output detail"),
 #ifdef ENABLE_DEBUG
 	OPT_BOOLEAN(0, "debug", &debug, "debug list walk"),
@@ -121,6 +123,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.fw = true;
 		param.alert_config = true;
 		param.dax = true;
+		param.media_errors = true;
 		/* fallthrough */
 	case 2:
 		param.idle = true;
-- 
2.37.3

Re: [ndctl PATCH v10 6/7] cxl/list: add --media-errors option to cxl list

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> The --media-errors option to 'cxl list' retrieves poison lists from
> memory devices supporting the capability and displays the returned
> media_error records in the cxl list json. This option can apply to
> memdevs or regions.
> 
> Include media-errors in the -vvv verbose option.
> 
> Example usage in the Documentation/cxl/cxl-list.txt update.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Looks good, just the comment on switching from "dpa" + "hpa" to an {
offset, length } tuple.

Other than that, for this one and patch 7/7:

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

[ndctl PATCH v10 7/7] cxl/test: add cxl-poison.sh unit test

[alison.schofield]

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

Exercise cxl list, libcxl, and driver pieces of the get poison list
pathway. Inject and clear poison using debugfs and use cxl-cli to
read the poison list by memdev and by region.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 test/cxl-poison.sh | 137 +++++++++++++++++++++++++++++++++++++++++++++
 test/meson.build   |   2 +
 2 files changed, 139 insertions(+)
 create mode 100644 test/cxl-poison.sh

diff --git a/test/cxl-poison.sh b/test/cxl-poison.sh
new file mode 100644
index 000000000000..af2e9dcd1a11
--- /dev/null
+++ b/test/cxl-poison.sh
@@ -0,0 +1,137 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 Intel Corporation. All rights reserved.
+
+. "$(dirname "$0")"/common
+
+rc=77
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+
+rc=1
+
+# THEORY OF OPERATION: Exercise cxl-cli and cxl driver ability to
+# inject, clear, and get the poison list. Do it by memdev and by region.
+
+find_memdev()
+{
+	readarray -t capable_mems < <("$CXL" list -b "$CXL_TEST_BUS" -M |
+		jq -r ".[] | select(.pmem_size != null) |
+		select(.ram_size != null) | .memdev")
+
+	if [ ${#capable_mems[@]} == 0 ]; then
+		echo "no memdevs found for test"
+		err "$LINENO"
+	fi
+
+	memdev=${capable_mems[0]}
+}
+
+create_x2_region()
+{
+	# Find an x2 decoder
+	decoder="$($CXL list -b "$CXL_TEST_BUS" -D -d root | jq -r ".[] |
+		select(.pmem_capable == true) |
+		select(.nr_targets == 2) |
+		.decoder")"
+
+	# Find a memdev for each host-bridge interleave position
+	port_dev0="$($CXL list -T -d "$decoder" | jq -r ".[] |
+		.targets | .[] | select(.position == 0) | .target")"
+	port_dev1="$($CXL list -T -d "$decoder" | jq -r ".[] |
+		.targets | .[] | select(.position == 1) | .target")"
+	mem0="$($CXL list -M -p "$port_dev0" | jq -r ".[0].memdev")"
+	mem1="$($CXL list -M -p "$port_dev1" | jq -r ".[0].memdev")"
+
+	region="$($CXL create-region -d "$decoder" -m "$mem0" "$mem1" |
+		jq -r ".region")"
+	if [[ ! $region ]]; then
+		echo "create-region failed for $decoder"
+		err "$LINENO"
+	fi
+	echo "$region"
+}
+
+# When cxl-cli support for inject and clear arrives, replace
+# the writes to /sys/kernel/debug with the new cxl commands.
+
+inject_poison_sysfs()
+{
+	memdev="$1"
+	addr="$2"
+
+	echo "$addr" > /sys/kernel/debug/cxl/"$memdev"/inject_poison
+}
+
+clear_poison_sysfs()
+{
+	memdev="$1"
+	addr="$2"
+
+	echo "$addr" > /sys/kernel/debug/cxl/"$memdev"/clear_poison
+}
+
+validate_poison_found()
+{
+	list_by="$1"
+	nr_expect="$2"
+
+	poison_list="$($CXL list "$list_by" --media-errors |
+		jq -r '.[].media_errors')"
+	if [[ ! $poison_list ]]; then
+		nr_found=0
+	else
+		nr_found=$(jq "length" <<< "$poison_list")
+	fi
+	if [ "$nr_found" -ne "$nr_expect" ]; then
+		echo "$nr_expect poison records expected, $nr_found found"
+		err "$LINENO"
+	fi
+}
+
+test_poison_by_memdev()
+{
+	find_memdev
+	inject_poison_sysfs "$memdev" "0x40000000"
+	inject_poison_sysfs "$memdev" "0x40001000"
+	inject_poison_sysfs "$memdev" "0x600"
+	inject_poison_sysfs "$memdev" "0x0"
+	validate_poison_found "-m $memdev" 4
+
+	clear_poison_sysfs "$memdev" "0x40000000"
+	clear_poison_sysfs "$memdev" "0x40001000"
+	clear_poison_sysfs "$memdev" "0x600"
+	clear_poison_sysfs "$memdev" "0x0"
+	validate_poison_found "-m $memdev" 0
+}
+
+test_poison_by_region()
+{
+	create_x2_region
+	inject_poison_sysfs "$mem0" "0x40000000"
+	inject_poison_sysfs "$mem1" "0x40000000"
+	validate_poison_found "-r $region" 2
+
+	clear_poison_sysfs "$mem0" "0x40000000"
+	clear_poison_sysfs "$mem1" "0x40000000"
+	validate_poison_found "-r $region" 0
+}
+
+# Turn tracing on. Note that 'cxl list --poison' does toggle the tracing.
+# Turning it on here allows the test user to also view inject and clear
+# trace events.
+echo 1 > /sys/kernel/tracing/events/cxl/cxl_poison/enable
+
+test_poison_by_memdev
+test_poison_by_region
+
+check_dmesg "$LINENO"
+
+modprobe -r cxl-test
diff --git a/test/meson.build b/test/meson.build
index a965a79fd6cb..d871e28e17ce 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -160,6 +160,7 @@ cxl_events = find_program('cxl-events.sh')
 cxl_sanitize = find_program('cxl-sanitize.sh')
 cxl_destroy_region = find_program('cxl-destroy-region.sh')
 cxl_qos_class = find_program('cxl-qos-class.sh')
+cxl_poison = find_program('cxl-poison.sh')
 
 tests = [
   [ 'libndctl',               libndctl,		  'ndctl' ],
@@ -192,6 +193,7 @@ tests = [
   [ 'cxl-sanitize.sh',        cxl_sanitize,       'cxl'   ],
   [ 'cxl-destroy-region.sh',  cxl_destroy_region, 'cxl'   ],
   [ 'cxl-qos-class.sh',       cxl_qos_class,      'cxl'   ],
+  [ 'cxl-poison.sh',          cxl_poison,         'cxl'   ],
 ]
 
 if get_option('destructive').enabled()
-- 
2.37.3

Re: [ndctl PATCH v10 0/7] Support poison list retrieval

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Changes since v9:
> - Replace the multi-use 'name' var, with multiple descriptive
>   flavors: memdev_name, region_name, decoder_name (DaveJ)
> - Use a static string table for poison source lookup (DaveJ)
> - Rebased on latest pending
> Link to v9: https://lore.kernel.org/r/cover.1709253898.git.alison.schofield@intel.com/
> 
> 
> Add the option to add a memory devices poison list to the cxl-list
> json output. Offer the option by memdev and by region. Sample usage:
> 
> # cxl list -m mem1 --media-errors
> [
>   {
>     "memdev":"mem1",
>     "pmem_size":1073741824,
>     "ram_size":1073741824,
>     "serial":1,
>     "numa_node":1,
>     "host":"cxl_mem.1",
>     "media_errors":[
>       {
>         "dpa":0,
>         "length":64,
>         "source":"Internal"
>       },
>       {
>         "decoder":"decoder10.0",
>         "hpa":1035355557888,
>         "dpa":1073741824,
>         "length":64,
>         "source":"External"
>       },
>       {
>         "decoder":"decoder10.0",
>         "hpa":1035355566080,
>         "dpa":1073745920,
>         "length":64,
>         "source":"Injected"
>       }
>     ]
>   }
> ]
> 
> # cxl list -r region5 --media-errors
> [
>   {
>     "region":"region5",
>     "resource":1035355553792,
>     "size":2147483648,
>     "type":"pmem",
>     "interleave_ways":2,
>     "interleave_granularity":4096,
>     "decode_state":"commit",
>     "media_errors":[
>       {
>         "decoder":"decoder10.0",
>         "hpa":1035355557888,
>         "dpa":1073741824,
>         "length":64,

I notice that the ndctl --media-errors records are:

{ offset, length }

...it is not clear to me that "dpa" and "hpa" have much meaning to
userspace by default. Physical address information is privileged, so if
these records were { offset, length } tuples there is the possibility
that they can be provided to non-root.

"Offset" is region relative "hpa" when listing region media errors, and
"offset" is memdev relative "dpa" while listing memdev relative media
errors.

Re: [ndctl PATCH v10 0/7] Support poison list retrieval

[Alison Schofield]

On Wed, Mar 06, 2024 at 03:03:40PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Changes since v9:
> > - Replace the multi-use 'name' var, with multiple descriptive
> >   flavors: memdev_name, region_name, decoder_name (DaveJ)
> > - Use a static string table for poison source lookup (DaveJ)
> > - Rebased on latest pending
> > Link to v9: https://lore.kernel.org/r/cover.1709253898.git.alison.schofield@intel.com/
> > 
> > 
> > Add the option to add a memory devices poison list to the cxl-list
> > json output. Offer the option by memdev and by region. Sample usage:
> > 
> > # cxl list -m mem1 --media-errors
> > [
> >   {
> >     "memdev":"mem1",
> >     "pmem_size":1073741824,
> >     "ram_size":1073741824,
> >     "serial":1,
> >     "numa_node":1,
> >     "host":"cxl_mem.1",
> >     "media_errors":[
> >       {
> >         "dpa":0,
> >         "length":64,
> >         "source":"Internal"
> >       },
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355557888,
> >         "dpa":1073741824,
> >         "length":64,
> >         "source":"External"
> >       },
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355566080,
> >         "dpa":1073745920,
> >         "length":64,
> >         "source":"Injected"
> >       }
> >     ]
> >   }
> > ]
> > 
> > # cxl list -r region5 --media-errors
> > [
> >   {
> >     "region":"region5",
> >     "resource":1035355553792,
> >     "size":2147483648,
> >     "type":"pmem",
> >     "interleave_ways":2,
> >     "interleave_granularity":4096,
> >     "decode_state":"commit",
> >     "media_errors":[
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355557888,
> >         "dpa":1073741824,
> >         "length":64,
> 
> I notice that the ndctl --media-errors records are:
> 
> { offset, length }
> 
> ...it is not clear to me that "dpa" and "hpa" have much meaning to
> userspace by default. Physical address information is privileged, so if
> these records were { offset, length } tuples there is the possibility
> that they can be provided to non-root.
> 
> "Offset" is region relative "hpa" when listing region media errors, and
> "offset" is memdev relative "dpa" while listing memdev relative media
> errors.

Done. memdev relative dpa is just dpa right? Unless you are thinking
offset into a partition? I don't think so.

Re: [ndctl PATCH v10 0/7] Support poison list retrieval

[Dan Williams]

Alison Schofield wrote:
[..]
> > I notice that the ndctl --media-errors records are:
> > 
> > { offset, length }
> > 
> > ...it is not clear to me that "dpa" and "hpa" have much meaning to
> > userspace by default. Physical address information is privileged, so if
> > these records were { offset, length } tuples there is the possibility
> > that they can be provided to non-root.
> > 
> > "Offset" is region relative "hpa" when listing region media errors, and
> > "offset" is memdev relative "dpa" while listing memdev relative media
> > errors.
> 
> Done. memdev relative dpa is just dpa right? Unless you are thinking
> offset into a partition? I don't think so.

Right,
	memdev offset == absolute device dpa

	region offset == region base relative

Re: [ndctl PATCH v10 0/7] Support poison list retrieval

[Alison Schofield]

On Wed, Mar 06, 2024 at 03:03:40PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Changes since v9:
> > - Replace the multi-use 'name' var, with multiple descriptive
> >   flavors: memdev_name, region_name, decoder_name (DaveJ)
> > - Use a static string table for poison source lookup (DaveJ)
> > - Rebased on latest pending
> > Link to v9: https://lore.kernel.org/r/cover.1709253898.git.alison.schofield@intel.com/
> > 
> > 
> > Add the option to add a memory devices poison list to the cxl-list
> > json output. Offer the option by memdev and by region. Sample usage:
> > 
> > # cxl list -m mem1 --media-errors
> > [
> >   {
> >     "memdev":"mem1",
> >     "pmem_size":1073741824,
> >     "ram_size":1073741824,
> >     "serial":1,
> >     "numa_node":1,
> >     "host":"cxl_mem.1",
> >     "media_errors":[
> >       {
> >         "dpa":0,
> >         "length":64,
> >         "source":"Internal"
> >       },
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355557888,
> >         "dpa":1073741824,
> >         "length":64,
> >         "source":"External"
> >       },
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355566080,
> >         "dpa":1073745920,
> >         "length":64,
> >         "source":"Injected"
> >       }

Dan,

In cleaning up the man pages, I need to follow up on this offset, length
notation.

A default by memdev list now looks like this-

{
 "offset" :
 "length" :
 "source" :
}

Which means dropping the 'decoder' even if it can be discovered from
the trace event. Recall previously a region was listed if present,
then we changed that to a decoder is listed if present.  Now, with
no 'hpa' listing I've dropped the decoder too. That leaves no hint
in the by memdev listing that this poison is in a region. 

Decoders will only be included in the by region list:
{
 "decoder":
 "offset" :
 "length" :
 "source" :
}

OK ?


> >   }
> > ]
> > 
> > # cxl list -r region5 --media-errors
> > [
> >   {
> >     "region":"region5",
> >     "resource":1035355553792,
> >     "size":2147483648,
> >     "type":"pmem",
> >     "interleave_ways":2,
> >     "interleave_granularity":4096,
> >     "decode_state":"commit",
> >     "media_errors":[
> >       {
> >         "decoder":"decoder10.0",
> >         "hpa":1035355557888,
> >         "dpa":1073741824,
> >         "length":64,
> 
> I notice that the ndctl --media-errors records are:
> 
> { offset, length }
> 
> ...it is not clear to me that "dpa" and "hpa" have much meaning to
> userspace by default. Physical address information is privileged, so if
> these records were { offset, length } tuples there is the possibility
> that they can be provided to non-root.
> 
> "Offset" is region relative "hpa" when listing region media errors, and
> "offset" is memdev relative "dpa" while listing memdev relative media
> errors.