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

Shiju Jose
2023-01-26T10:04:24Z

Hi Ira,

Thank you for the feedback.

>-----Original Message-----
>From: Ira Weiny <ira.weiny@intel.com>
>Sent: 25 January 2023 22:35
>To: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>cxl@vger.kernel.org; mchehab@kernel.org
>Cc: Jonathan Cameron <jonathan.cameron@huawei.com>; Linuxarm
><linuxarm@huawei.com>; Shiju Jose <shiju.jose@huawei.com>
>Subject: Re: [PATCH V2 2/4] rasdaemon: Add support for the CXL poison
>events
>
>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/de11785ff05844299b40b100f8e0f56c7eef
>> 7f08.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]

This file ras-cxl-handler.c included in the build only if configure with --enable-cxl.
With --enable-cxl build scripts define HAVE_CXL.

>
>> --- 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]

Thanks,
Shiju