alison.schofield@ wrote:
> From: Alison Schofield <alison.schofi...@intel.com>
>
> Poison list records are logged as events in the kernel tracing
> subsystem. To prepare the poison list for cxl list, enable tracing,
> trigger the poison list read, and parse the generated cxl_poison
> events into a json representation.
>
> Signed-off-by: Alison Schofield <alison.schofi...@intel.com>
> ---
> cxl/json.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 211 insertions(+)
>
> diff --git a/cxl/json.c b/cxl/json.c
> index 7678d02020b6..6fb17582a1cb 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -2,15 +2,19 @@
> // Copyright (C) 2015-2021 Intel Corporation. All rights reserved.
> #include <limits.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 <traceevent/event-parse.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,201 @@ err_jobj:
> return NULL;
> }
>
> +/* CXL Spec 3.1 Table 8-140 Media Error Record */
> +#define CXL_POISON_SOURCE_UNKNOWN 0
> +#define CXL_POISON_SOURCE_EXTERNAL 1
> +#define CXL_POISON_SOURCE_INTERNAL 2
> +#define CXL_POISON_SOURCE_INJECTED 3
> +#define CXL_POISON_SOURCE_VENDOR 7
> +
> +/* 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)
> +
> +static struct json_object *
> +util_cxl_poison_events_to_json(struct tracefs_instance *inst,
> + const char *region_name, unsigned long flags)
> +{
> + struct json_object *jerrors, *jpoison, *jobj = NULL;
> + struct jlist_node *jnode, *next;
> + struct event_ctx ectx = {
> + .event_name = "cxl_poison",
> + .event_pid = getpid(),
> + .system = "cxl",
> + };
> + int rc, count = 0;
> +
> + list_head_init(&ectx.jlist_head);
> + rc = cxl_parse_events(inst, &ectx);
This pattern really feels like it wants a cxl_for_each_event() -style
helper rather than require the end caller to open code list usage.
Basically cxl_parse_events() is a helper that should stay local to
cxl/monitor.c. This new cxl_for_each_event() would become used
internally by cxl_parse_events() and let
util_cxl_poison_events_to_json() do its own per-event iteration.
> + if (rc < 0) {
> + fprintf(stderr, "Failed to parse events: %d\n", rc);
> + return NULL;
> + }
> + /* Add nr_records:0 to json */
> + if (list_empty(&ectx.jlist_head))
> + goto out;
> +
> + jerrors = json_object_new_array();
> + if (!jerrors)
> + return NULL;
> +
> + list_for_each_safe(&ectx.jlist_head, jnode, next, list) {
> + struct json_object *jp, *jval;
> + int source, pflags = 0;
> + u64 addr, len;
> +
> + jp = json_object_new_object();
> + if (!jp)
> + return NULL;
> +
> + /* Skip records not in this region when listing by region */
> + if (json_object_object_get_ex(jnode->jobj, "region", &jval)) {
> + const char *name;
So we're building a json_object internal to cxl_parse_events() only to
turn around and extract details out of that object that tell us this
event was not of interest, or to create yet another json object?
I think this implementation has a chance to be significantly less
complicated if the event list can be iterated directly without this
temporary json_object parsing.
