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

[Shiju Jose <shiju.jose@huawei.com>]

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