Re: [PATCH V2 2/4] rasdaemon: Add support for the CXL poison events

Ira Weiny
2023-01-25T22:35:04Z

shiju.jose@ wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add support to log and record the CXL poison events.
>
> The corresponding Kernel patches here:
> https://lore.kernel.org/linux-cxl/de11785ff05844299b40b100f8e0f56c7eef7f08.1674070170.git.alison.schofield@intel.com/
>
> Presently RFC draft version for logging, could be extended for the policy
> based recovery action for the frequent poison events depending on the above
> kernel patches.
>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[snip]

> +
> +int ras_cxl_poison_event_handler(struct trace_seq *s,
> + struct tep_record *record,
> + struct tep_event *event, void *context)
> +{
> + int len;
> + unsigned long long val;
> + struct ras_events *ras = context;
> + time_t now;
> + struct tm *tm;
> + struct ras_cxl_poison_event ev;
> +
> + now = record->ts/user_hz + ras->uptime_diff;
> + tm = localtime(&now);
> + if (tm)
> + strftime(ev.timestamp, sizeof(ev.timestamp),
> + "%Y-%m-%d %H:%M:%S %z", tm);
> + else
> + strncpy(ev.timestamp, "1970-01-01 00:00:00 +0000", sizeof(ev.timestamp));
> + trace_seq_printf(s, "%s ", ev.timestamp);
> +
> + ev.memdev = tep_get_field_raw(s, event, "memdev",
> + record, &len, 1);
> + if (!ev.memdev)
> + return -1;
> + trace_seq_printf(s, "memdev:%s ", ev.memdev);
> +
> + ev.pcidev = tep_get_field_raw(s, event, "pcidev",
> + record, &len, 1);
> + if (!ev.pcidev)
> + return -1;
> + trace_seq_printf(s, "pcidev:%s ", ev.pcidev);
> +
> + ev.region = tep_get_field_raw(s, event, "region",
> + record, &len, 1);
> + if (!ev.region)
> + return -1;
> + trace_seq_printf(s, "region:%s ", ev.region);
> +
> + ev.uuid = tep_get_field_raw(s, event, "uuid",
> + record, &len, 1);
> + if (!ev.uuid)
> + return -1;
> + trace_seq_printf(s, "region_uuid:%s ", ev.uuid);
> +
> + if (tep_get_field_val(s, event, "hpa", record, &val, 1) < 0)
> + return -1;
> + ev.hpa = val;
> + trace_seq_printf(s, "poison list: hpa:0x%llx ", (unsigned long long)ev.hpa);
> +
> + if (tep_get_field_val(s, event, "dpa", record, &val, 1) < 0)
> + return -1;
> + ev.dpa = val;
> + trace_seq_printf(s, "dpa:0x%llx ", (unsigned long long)ev.dpa);
> +
> + if (tep_get_field_val(s, event, "length", record, &val, 1) < 0)
> + return -1;
> + ev.length = val;
> + trace_seq_printf(s, "length:%d ", ev.length);
> +
> + if (tep_get_field_val(s, event, "source", record, &val, 1) < 0)
> + return -1;
> +
> + switch (val) {
> + case CXL_POISON_SOURCE_UNKNOWN:
> + ev.source = "Unknown";
> + break;
> + case CXL_POISON_SOURCE_EXTERNAL:
> + ev.source = "External";
> + break;
> + case CXL_POISON_SOURCE_INTERNAL:
> + ev.source = "Internal";
> + break;
> + case CXL_POISON_SOURCE_INJECTED:
> + ev.source = "Injected";
> + break;
> + case CXL_POISON_SOURCE_VENDOR:
> + ev.source = "Vendor";
> + break;
> + default:
> + ev.source = "Invalid";
> + }
> + trace_seq_printf(s, "source:%s ", ev.source);
> +
> + if (tep_get_field_val(s, event, "flags", record, &val, 1) < 0)
> + return -1;
> + ev.flags = val;
> + trace_seq_printf(s, "flags:%d ", ev.flags);
> +
> + if (ev.flags & CXL_POISON_FLAG_OVERFLOW) {
> + if (tep_get_field_val(s, event, "overflow_t", record, &val, 1) < 0)
> + return -1;
> + if (val) {
> + /* CXL Specification 3.0
> + * Overflow timestamp - The number of unsigned nanoseconds
> + * that have elapsed since midnight, 01-Jan-1970 UTC
> + */
> + time_t ovf_ts_secs = val / 1000000000ULL;
> +
> + tm = localtime(&ovf_ts_secs);
> + if (tm) {
> + strftime(ev.overflow_ts, sizeof(ev.overflow_ts),
> + "%Y-%m-%d %H:%M:%S %z", tm);
> + }
> + }
> + if (!val || !tm)
> + strncpy(ev.overflow_ts, "1970-01-01 00:00:00 +0000",
> + sizeof(ev.overflow_ts));
> + } else
> + strncpy(ev.overflow_ts, "1970-01-01 00:00:00 +0000", sizeof(ev.overflow_ts));
> + trace_seq_printf(s, "overflow timestamp:%s ", ev.overflow_ts);
> + trace_seq_printf(s, "\n");
> +
> + /* Insert data into the SGBD */
> +#ifdef HAVE_SQLITE3
> + ras_store_cxl_poison_event(ras, &ev);
> +#endif

I know nothing about the rasdaemon build system but it seems like this
needs a ifdef HAVE_CXL around it?

[snip]

> --- a/ras-record.c
> +++ b/ras-record.c
> @@ -559,6 +559,67 @@ int ras_store_mf_event(struct ras_events *ras, struct ras_mf_event *ev)
> }
> #endif
>
> +#ifdef HAVE_CXL
> +/*
> + * Table and functions to handle cxl:cxl_poison
> + */
> +static const struct db_fields cxl_poison_event_fields[] = {
> + { .name = "id", .type = "INTEGER PRIMARY KEY" },
> + { .name = "timestamp", .type = "TEXT" },
> + { .name = "memdev", .type = "TEXT" },
> + { .name = "pcidev", .type = "TEXT" },
> + { .name = "region", .type = "TEXT" },
> + { .name = "region_uuid", .type = "TEXT" },
> + { .name = "hpa", .type = "INTEGER" },
> + { .name = "dpa", .type = "INTEGER" },
> + { .name = "length", .type = "INTEGER" },
> + { .name = "source", .type = "TEXT" },
> + { .name = "flags", .type = "INTEGER" },
> + { .name = "overflow_ts", .type = "TEXT" },
> +};
> +
> +static const struct db_table_descriptor cxl_poison_event_tab = {
> + .name = "cxl_poison_event",
> + .fields = cxl_poison_event_fields,
> + .num_fields = ARRAY_SIZE(cxl_poison_event_fields),
> +};
> +
> +int ras_store_cxl_poison_event(struct ras_events *ras, struct ras_cxl_poison_event *ev)

Because I believe this is not defined if (!HAVE_CXL and HAVE_SQLITE3)

[snip]

>
> #ifdef HAVE_SQLITE3
>
> @@ -155,6 +170,9 @@ struct sqlite3_priv {
> #ifdef HAVE_MEMORY_FAILURE
> sqlite3_stmt *stmt_mf_event;
> #endif
> +#ifdef HAVE_CXL
> + sqlite3_stmt *stmt_cxl_poison_event;
> +#endif
> };
>
> struct db_fields {
> @@ -182,6 +200,7 @@ int ras_store_arm_record(struct ras_events *ras, struct ras_arm_event *ev);
> int ras_store_devlink_event(struct ras_events *ras, struct devlink_event *ev);
> int ras_store_diskerror_event(struct ras_events *ras, struct diskerror_event *ev);
> int ras_store_mf_event(struct ras_events *ras, struct ras_mf_event *ev);
> +int ras_store_cxl_poison_event(struct ras_events *ras, struct ras_cxl_poison_event *ev);
>
> #else
> static inline int ras_mc_event_opendb(unsigned cpu, struct ras_events *ras) { return 0; };
> @@ -195,6 +214,7 @@ static inline int ras_store_arm_record(struct ras_events *ras, struct ras_arm_ev
> static inline int ras_store_devlink_event(struct ras_events *ras, struct devlink_event *ev) { return 0; };
> static inline int ras_store_diskerror_event(struct ras_events *ras, struct diskerror_event *ev) { return 0; };
> static inline int ras_store_mf_event(struct ras_events *ras, struct ras_mf_event *ev) { return 0; };
> +static inline int ras_store_cxl_poison_event(struct ras_events *ras, struct ras_cxl_poison_event *ev) { return 0; };

But I could be missing something.

Ira

[snip]