[PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1

shiju.jose@huawei.com posted 6 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by shiju.jose@huawei.com 1 year, 2 months ago
From: Shiju Jose <shiju.jose@huawei.com>

CXL spec rev 3.1 section 8.2.9.2.1.1 Table 8-45, General Media Event
Record has updated with following new fields and new types for Memory
Event Type and Transaction Type fields.
1. Advanced Programmable Corrected Memory Error Threshold Event Flags
2. Corrected Memory Error Count at Event
3. Memory Event Sub-Type

The format of component identifier has changed (CXL spec 3.1 section
8.2.9.2.1 Table 8-44).

Update the general media event record and general media trace event for
the above spec changes. The new fields are inserted in logical places.

Example trace log of cxl_general_media trace event,

cxl_general_media: memdev=mem0 host=0000:0f:00.0 serial=3 log=Fatal : \
time=45104947948 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=128 \
flags='0x1' handle=1 related_handle=0 maint_op_class=2 \
maint_op_sub_class=4 : dpa=0x30d40 dpa_flags=0x0 \
descriptor='UNCORRECTABLE_EVENT|THRESHOLD_EVENT|POISON_LIST_OVERFLOW' \
type='TE State Violation' sub_type=0x2 transaction_type=0x4 channel=3 \
rank=33 device=0x5 validity_flags=0x1f \
comp_id=03 74 c5 08 9a 1a 0b fc d2 7e 2f 31 9b 3c 81 4d \
pldm_entity_id=74 c5 08 9a 1a 0b pldm_resource_id=fc d2 7e 2f \
hpa=0xffffffffffffffff region= region_uuid=00000000-0000-0000-0000-000000000000 \
cme_threshold_ev_flags=0x3 cme_count=0x78

The number of decoded strings in TP_printk() caused  parsing error when
libtraceevent in userspace parses the CXL general media trace event for
rasdaemon. It was found that long decoded strings of field values in the
TP_printk() caused the issue. As a solution, decoding of some fields
in the TP_printk() were removed to accommodate the new fields.
Decoding of all these fields is added in the userspace tool rasdaemon.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/core/trace.h | 58 +++++++++++++++++++++++++++++-----------
 include/cxl/event.h      |  7 +++--
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 8e9d80e34a28..77055d66b56e 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -287,7 +287,7 @@ TRACE_EVENT(cxl_generic_event,
 
 /*
  * General Media Event Record - GMER
- * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
+ * CXL rev 3.1 Section 8.2.9.2.1.1; Table 8-45
  */
 #define CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT		BIT(0)
 #define CXL_GMER_EVT_DESC_THRESHOLD_EVENT		BIT(1)
@@ -301,10 +301,18 @@ TRACE_EVENT(cxl_generic_event,
 #define CXL_GMER_MEM_EVT_TYPE_ECC_ERROR			0x00
 #define CXL_GMER_MEM_EVT_TYPE_INV_ADDR			0x01
 #define CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR		0x02
-#define show_gmer_mem_event_type(type)	__print_symbolic(type,			\
-	{ CXL_GMER_MEM_EVT_TYPE_ECC_ERROR,		"ECC Error" },		\
-	{ CXL_GMER_MEM_EVT_TYPE_INV_ADDR,		"Invalid Address" },	\
-	{ CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,	"Data Path Error" }	\
+#define CXL_GMER_MEM_EVT_TYPE_TE_STATE_VIOLATION	0x03
+#define CXL_GMER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR	0x04
+#define CXL_GMER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE	0x05
+#define CXL_GMER_MEM_EVT_TYPE_CKID_VIOLATION		0x06
+#define show_gmer_mem_event_type(type)	__print_symbolic(type,				\
+	{ CXL_GMER_MEM_EVT_TYPE_ECC_ERROR,		"ECC Error" },			\
+	{ CXL_GMER_MEM_EVT_TYPE_INV_ADDR,		"Invalid Address" },		\
+	{ CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,	"Data Path Error" },		\
+	{ CXL_GMER_MEM_EVT_TYPE_TE_STATE_VIOLATION,	"TE State Violation" },		\
+	{ CXL_GMER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR,	"Scrub Media ECC Error" },	\
+	{ CXL_GMER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE,	"Adv Prog CME Counter Expiration" },	\
+	{ CXL_GMER_MEM_EVT_TYPE_CKID_VIOLATION,		"CKID Violation" }		\
 )
 
 #define CXL_GMER_TRANS_UNKNOWN				0x00
@@ -314,6 +322,8 @@ TRACE_EVENT(cxl_generic_event,
 #define CXL_GMER_TRANS_HOST_INJECT_POISON		0x04
 #define CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB		0x05
 #define CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT	0x06
+#define CXL_GMER_TRANS_INTERNAL_MEDIA_ECS		0x07
+#define CXL_GMER_TRANS_MEDIA_INITIALIZATION		0x08
 #define show_trans_type(type)	__print_symbolic(type,					\
 	{ CXL_GMER_TRANS_UNKNOWN,			"Unknown" },			\
 	{ CXL_GMER_TRANS_HOST_READ,			"Host Read" },			\
@@ -321,18 +331,22 @@ TRACE_EVENT(cxl_generic_event,
 	{ CXL_GMER_TRANS_HOST_SCAN_MEDIA,		"Host Scan Media" },		\
 	{ CXL_GMER_TRANS_HOST_INJECT_POISON,		"Host Inject Poison" },		\
 	{ CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB,		"Internal Media Scrub" },	\
-	{ CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT,	"Internal Media Management" }	\
+	{ CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT,	"Internal Media Management" },	\
+	{ CXL_GMER_TRANS_INTERNAL_MEDIA_ECS,		"Internal Media Error Check Scrub" },	\
+	{ CXL_GMER_TRANS_MEDIA_INITIALIZATION,		"Media Initialization" }	\
 )
 
 #define CXL_GMER_VALID_CHANNEL				BIT(0)
 #define CXL_GMER_VALID_RANK				BIT(1)
 #define CXL_GMER_VALID_DEVICE				BIT(2)
 #define CXL_GMER_VALID_COMPONENT			BIT(3)
+#define CXL_GMER_VALID_COMPONENT_ID_FORMAT		BIT(4)
 #define show_valid_flags(flags)	__print_flags(flags, "|",		   \
 	{ CXL_GMER_VALID_CHANNEL,			"CHANNEL"	}, \
 	{ CXL_GMER_VALID_RANK,				"RANK"		}, \
 	{ CXL_GMER_VALID_DEVICE,			"DEVICE"	}, \
-	{ CXL_GMER_VALID_COMPONENT,			"COMPONENT"	}  \
+	{ CXL_GMER_VALID_COMPONENT,			"COMPONENT"	}, \
+	{ CXL_GMER_VALID_COMPONENT_ID_FORMAT,		"COMPONENT PLDM FORMAT"	} \
 )
 
 TRACE_EVENT(cxl_general_media,
@@ -348,6 +362,7 @@ TRACE_EVENT(cxl_general_media,
 		__field(u64, dpa)
 		__field(u8, descriptor)
 		__field(u8, type)
+		__field(u8, sub_type)
 		__field(u8, transaction_type)
 		__field(u8, channel)
 		__field(u32, device)
@@ -359,6 +374,8 @@ TRACE_EVENT(cxl_general_media,
 		__field(u8, rank)
 		__field(u8, dpa_flags)
 		__string(region_name, cxlr ? dev_name(&cxlr->dev) : "")
+		__field(u8, cme_threshold_ev_flags)
+		__field(u32, cme_count)
 	),
 
 	TP_fast_assign(
@@ -372,6 +389,7 @@ TRACE_EVENT(cxl_general_media,
 		__entry->dpa &= CXL_DPA_MASK;
 		__entry->descriptor = rec->media_hdr.descriptor;
 		__entry->type = rec->media_hdr.type;
+		__entry->sub_type = rec->sub_type;
 		__entry->transaction_type = rec->media_hdr.transaction_type;
 		__entry->channel = rec->media_hdr.channel;
 		__entry->rank = rec->media_hdr.rank;
@@ -380,6 +398,8 @@ TRACE_EVENT(cxl_general_media,
 			CXL_EVENT_GEN_MED_COMP_ID_SIZE);
 		__entry->validity_flags = get_unaligned_le16(&rec->media_hdr.validity_flags);
 		__entry->hpa = hpa;
+		__entry->cme_threshold_ev_flags = rec->cme_threshold_ev_flags;
+		__entry->cme_count = get_unaligned_le24(rec->cme_count);
 		if (cxlr) {
 			__assign_str(region_name);
 			uuid_copy(&__entry->region_uuid, &cxlr->params.uuid);
@@ -389,18 +409,26 @@ TRACE_EVENT(cxl_general_media,
 		}
 	),
 
-	CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' " \
-		"descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u " \
-		"device=%x comp_id=%s validity_flags='%s' " \
-		"hpa=%llx region=%s region_uuid=%pUb",
-		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
+	CXL_EVT_TP_printk("dpa=0x%llx dpa_flags=0x%x " \
+		"descriptor='%s' type='%s' sub_type=0x%x " \
+		"transaction_type=0x%x channel=%u rank=%u " \
+		"device=0x%x validity_flags=0x%x " \
+		"comp_id=%s pldm_entity_id=%s pldm_resource_id=%s " \
+		"hpa=0x%llx region=%s region_uuid=%pUb " \
+		"cme_threshold_ev_flags=0x%x cme_count=0x%x ",
+		__entry->dpa, __entry->dpa_flags,
 		show_event_desc_flags(__entry->descriptor),
 		show_gmer_mem_event_type(__entry->type),
-		show_trans_type(__entry->transaction_type),
+		__entry->sub_type, __entry->transaction_type,
 		__entry->channel, __entry->rank, __entry->device,
+		__entry->validity_flags,
 		__print_hex(__entry->comp_id, CXL_EVENT_GEN_MED_COMP_ID_SIZE),
-		show_valid_flags(__entry->validity_flags),
-		__entry->hpa, __get_str(region_name), &__entry->region_uuid
+		show_pldm_entity_id(__entry->validity_flags, CXL_GMER_VALID_COMPONENT,
+				    CXL_GMER_VALID_COMPONENT_ID_FORMAT, __entry->comp_id),
+		show_pldm_resource_id(__entry->validity_flags, CXL_GMER_VALID_COMPONENT,
+				      CXL_GMER_VALID_COMPONENT_ID_FORMAT, __entry->comp_id),
+		__entry->hpa, __get_str(region_name), &__entry->region_uuid,
+		__entry->cme_threshold_ev_flags, __entry->cme_count
 	)
 );
 
diff --git a/include/cxl/event.h b/include/cxl/event.h
index e1d485ad376b..2b07adf39010 100644
--- a/include/cxl/event.h
+++ b/include/cxl/event.h
@@ -45,14 +45,17 @@ struct cxl_event_generic {
 
 /*
  * General Media Event Record
- * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
+ * CXL rev 3.1 Section 8.2.9.2.1.1; Table 8-45
  */
 #define CXL_EVENT_GEN_MED_COMP_ID_SIZE	0x10
 struct cxl_event_gen_media {
 	struct cxl_event_media_hdr media_hdr;
 	u8 device[3];
 	u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
-	u8 reserved[46];
+	u8 cme_threshold_ev_flags;
+	u8 cme_count[3];
+	u8 sub_type;
+	u8 reserved[41];
 } __packed;
 
 /*
-- 
2.43.0
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
CC  Steven Rostedt.

Hi Steven,

We are encountering a parsing error ("FAILED TO PARSE") from libtraceevent  when it
tries to parse some of the CXL trace events for the user-space tool rasdaemon. 
This issue appeared after new fields were added to the trace events. 
It was found that the issue does not occur when all or some of the decoded strings
for the event's data and flags are removed from the TP_printk() function in the kernel,
and only the values are printed instead.
https://elixir.bootlin.com/linux/v6.12/source/drivers/cxl/core/trace.h
https://lore.kernel.org/lkml/20241120093745.1847-1-shiju.jose@huawei.com/

Below is the information from the debugging in libtraceevent:
The failure occurs in the following functions and locations within libtraceevent:
File: src/event-parse.c
Function: event_read_format()
ret = event_read_fields(event->tep, event, &event->format.fields); if (ret < 0)
    return ret;

Function: event_read_fields()
if (test_type_token(type, token, TEP_EVENT_ITEM, "field"))
    goto fail;

Can you recognize if there are any limitations or issues  that would prevent
libtraceevent from parsing the trace event in the condition described above?

Thanks,
Shiju

>-----Original Message-----
>From: Shiju Jose <shiju.jose@huawei.com>
>Sent: 20 November 2024 09:38
>To: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
>Shiju Jose <shiju.jose@huawei.com>
>Subject: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL
>spec rev 3.1
>
>From: Shiju Jose <shiju.jose@huawei.com>
>
>CXL spec rev 3.1 section 8.2.9.2.1.1 Table 8-45, General Media Event Record has
>updated with following new fields and new types for Memory Event Type and
>Transaction Type fields.
>1. Advanced Programmable Corrected Memory Error Threshold Event Flags 2.
>Corrected Memory Error Count at Event 3. Memory Event Sub-Type
>
>The format of component identifier has changed (CXL spec 3.1 section
>8.2.9.2.1 Table 8-44).
>
>Update the general media event record and general media trace event for the
>above spec changes. The new fields are inserted in logical places.
>
>Example trace log of cxl_general_media trace event,
>
>cxl_general_media: memdev=mem0 host=0000:0f:00.0 serial=3 log=Fatal : \
>time=45104947948 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=128 \
>flags='0x1' handle=1 related_handle=0 maint_op_class=2 \
>maint_op_sub_class=4 : dpa=0x30d40 dpa_flags=0x0 \
>descriptor='UNCORRECTABLE_EVENT|THRESHOLD_EVENT|POISON_LIST_OVER
>FLOW' \ type='TE State Violation' sub_type=0x2 transaction_type=0x4 channel=3
>\
>rank=33 device=0x5 validity_flags=0x1f \
>comp_id=03 74 c5 08 9a 1a 0b fc d2 7e 2f 31 9b 3c 81 4d \
>pldm_entity_id=74 c5 08 9a 1a 0b pldm_resource_id=fc d2 7e 2f \
>hpa=0xffffffffffffffff region= region_uuid=00000000-0000-0000-0000-
>000000000000 \
>cme_threshold_ev_flags=0x3 cme_count=0x78
>
>The number of decoded strings in TP_printk() caused  parsing error when
>libtraceevent in userspace parses the CXL general media trace event for
>rasdaemon. It was found that long decoded strings of field values in the
>TP_printk() caused the issue. As a solution, decoding of some fields in the
>TP_printk() were removed to accommodate the new fields.
>Decoding of all these fields is added in the userspace tool rasdaemon.
>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>---
> drivers/cxl/core/trace.h | 58 +++++++++++++++++++++++++++++-----------
> include/cxl/event.h      |  7 +++--
> 2 files changed, 48 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h index
>8e9d80e34a28..77055d66b56e 100644
>--- a/drivers/cxl/core/trace.h
>+++ b/drivers/cxl/core/trace.h
>@@ -287,7 +287,7 @@ TRACE_EVENT(cxl_generic_event,
>
> /*
>  * General Media Event Record - GMER
>- * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
>+ * CXL rev 3.1 Section 8.2.9.2.1.1; Table 8-45
>  */
> #define CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT		BIT(0)
> #define CXL_GMER_EVT_DESC_THRESHOLD_EVENT		BIT(1)
>@@ -301,10 +301,18 @@ TRACE_EVENT(cxl_generic_event,
> #define CXL_GMER_MEM_EVT_TYPE_ECC_ERROR			0x00
> #define CXL_GMER_MEM_EVT_TYPE_INV_ADDR			0x01
> #define CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR		0x02
>-#define show_gmer_mem_event_type(type)	__print_symbolic(type,
>		\
>-	{ CXL_GMER_MEM_EVT_TYPE_ECC_ERROR,		"ECC Error" },
>		\
>-	{ CXL_GMER_MEM_EVT_TYPE_INV_ADDR,		"Invalid
>Address" },	\
>-	{ CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,	"Data Path
>Error" }	\
>+#define CXL_GMER_MEM_EVT_TYPE_TE_STATE_VIOLATION	0x03
>+#define CXL_GMER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR	0x04
>+#define CXL_GMER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE	0x05
>+#define CXL_GMER_MEM_EVT_TYPE_CKID_VIOLATION		0x06
>+#define show_gmer_mem_event_type(type)	__print_symbolic(type,
>			\
>+	{ CXL_GMER_MEM_EVT_TYPE_ECC_ERROR,		"ECC Error" },
>			\
>+	{ CXL_GMER_MEM_EVT_TYPE_INV_ADDR,		"Invalid
>Address" },		\
>+	{ CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,	"Data Path
>Error" },		\
>+	{ CXL_GMER_MEM_EVT_TYPE_TE_STATE_VIOLATION,	"TE State
>Violation" },		\
>+	{ CXL_GMER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR,	"Scrub
>Media ECC Error" },	\
>+	{ CXL_GMER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE,	"Adv
>Prog CME Counter Expiration" },	\
>+	{ CXL_GMER_MEM_EVT_TYPE_CKID_VIOLATION,		"CKID
>Violation" }		\
> )
>
> #define CXL_GMER_TRANS_UNKNOWN				0x00
>@@ -314,6 +322,8 @@ TRACE_EVENT(cxl_generic_event,
> #define CXL_GMER_TRANS_HOST_INJECT_POISON		0x04
> #define CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB		0x05
> #define CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT	0x06
>+#define CXL_GMER_TRANS_INTERNAL_MEDIA_ECS		0x07
>+#define CXL_GMER_TRANS_MEDIA_INITIALIZATION		0x08
> #define show_trans_type(type)	__print_symbolic(type,
>		\
> 	{ CXL_GMER_TRANS_UNKNOWN,			"Unknown" },
>			\
> 	{ CXL_GMER_TRANS_HOST_READ,			"Host Read" },
>			\
>@@ -321,18 +331,22 @@ TRACE_EVENT(cxl_generic_event,
> 	{ CXL_GMER_TRANS_HOST_SCAN_MEDIA,		"Host Scan
>Media" },		\
> 	{ CXL_GMER_TRANS_HOST_INJECT_POISON,		"Host Inject
>Poison" },		\
> 	{ CXL_GMER_TRANS_INTERNAL_MEDIA_SCRUB,		"Internal
>Media Scrub" },	\
>-	{ CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT,
>	"Internal Media Management" }	\
>+	{ CXL_GMER_TRANS_INTERNAL_MEDIA_MANAGEMENT,
>	"Internal Media Management" },	\
>+	{ CXL_GMER_TRANS_INTERNAL_MEDIA_ECS,		"Internal
>Media Error Check Scrub" },	\
>+	{ CXL_GMER_TRANS_MEDIA_INITIALIZATION,		"Media
>Initialization" }	\
> )
>
> #define CXL_GMER_VALID_CHANNEL				BIT(0)
> #define CXL_GMER_VALID_RANK				BIT(1)
> #define CXL_GMER_VALID_DEVICE				BIT(2)
> #define CXL_GMER_VALID_COMPONENT			BIT(3)
>+#define CXL_GMER_VALID_COMPONENT_ID_FORMAT		BIT(4)
> #define show_valid_flags(flags)	__print_flags(flags, "|",		   \
> 	{ CXL_GMER_VALID_CHANNEL,			"CHANNEL"	}, \
> 	{ CXL_GMER_VALID_RANK,				"RANK"
>	}, \
> 	{ CXL_GMER_VALID_DEVICE,			"DEVICE"	}, \
>-	{ CXL_GMER_VALID_COMPONENT,			"COMPONENT"
>	}  \
>+	{ CXL_GMER_VALID_COMPONENT,			"COMPONENT"
>	}, \
>+	{ CXL_GMER_VALID_COMPONENT_ID_FORMAT,
>	"COMPONENT PLDM FORMAT"	} \
> )
>
> TRACE_EVENT(cxl_general_media,
>@@ -348,6 +362,7 @@ TRACE_EVENT(cxl_general_media,
> 		__field(u64, dpa)
> 		__field(u8, descriptor)
> 		__field(u8, type)
>+		__field(u8, sub_type)
> 		__field(u8, transaction_type)
> 		__field(u8, channel)
> 		__field(u32, device)
>@@ -359,6 +374,8 @@ TRACE_EVENT(cxl_general_media,
> 		__field(u8, rank)
> 		__field(u8, dpa_flags)
> 		__string(region_name, cxlr ? dev_name(&cxlr->dev) : "")
>+		__field(u8, cme_threshold_ev_flags)
>+		__field(u32, cme_count)
> 	),
>
> 	TP_fast_assign(
>@@ -372,6 +389,7 @@ TRACE_EVENT(cxl_general_media,
> 		__entry->dpa &= CXL_DPA_MASK;
> 		__entry->descriptor = rec->media_hdr.descriptor;
> 		__entry->type = rec->media_hdr.type;
>+		__entry->sub_type = rec->sub_type;
> 		__entry->transaction_type = rec->media_hdr.transaction_type;
> 		__entry->channel = rec->media_hdr.channel;
> 		__entry->rank = rec->media_hdr.rank;
>@@ -380,6 +398,8 @@ TRACE_EVENT(cxl_general_media,
> 			CXL_EVENT_GEN_MED_COMP_ID_SIZE);
> 		__entry->validity_flags = get_unaligned_le16(&rec-
>>media_hdr.validity_flags);
> 		__entry->hpa = hpa;
>+		__entry->cme_threshold_ev_flags = rec-
>>cme_threshold_ev_flags;
>+		__entry->cme_count = get_unaligned_le24(rec->cme_count);
> 		if (cxlr) {
> 			__assign_str(region_name);
> 			uuid_copy(&__entry->region_uuid, &cxlr-
>>params.uuid); @@ -389,18 +409,26 @@ TRACE_EVENT(cxl_general_media,
> 		}
> 	),
>
>-	CXL_EVT_TP_printk("dpa=%llx dpa_flags='%s' " \
>-		"descriptor='%s' type='%s' transaction_type='%s' channel=%u
>rank=%u " \
>-		"device=%x comp_id=%s validity_flags='%s' " \
>-		"hpa=%llx region=%s region_uuid=%pUb",
>-		__entry->dpa, show_dpa_flags(__entry->dpa_flags),
>+	CXL_EVT_TP_printk("dpa=0x%llx dpa_flags=0x%x " \
>+		"descriptor='%s' type='%s' sub_type=0x%x " \
>+		"transaction_type=0x%x channel=%u rank=%u " \
>+		"device=0x%x validity_flags=0x%x " \
>+		"comp_id=%s pldm_entity_id=%s pldm_resource_id=%s " \
>+		"hpa=0x%llx region=%s region_uuid=%pUb " \
>+		"cme_threshold_ev_flags=0x%x cme_count=0x%x ",
>+		__entry->dpa, __entry->dpa_flags,
> 		show_event_desc_flags(__entry->descriptor),
> 		show_gmer_mem_event_type(__entry->type),
>-		show_trans_type(__entry->transaction_type),
>+		__entry->sub_type, __entry->transaction_type,
> 		__entry->channel, __entry->rank, __entry->device,
>+		__entry->validity_flags,
> 		__print_hex(__entry->comp_id,
>CXL_EVENT_GEN_MED_COMP_ID_SIZE),
>-		show_valid_flags(__entry->validity_flags),
>-		__entry->hpa, __get_str(region_name), &__entry->region_uuid
>+		show_pldm_entity_id(__entry->validity_flags,
>CXL_GMER_VALID_COMPONENT,
>+
>CXL_GMER_VALID_COMPONENT_ID_FORMAT, __entry->comp_id),
>+		show_pldm_resource_id(__entry->validity_flags,
>CXL_GMER_VALID_COMPONENT,
>+
>CXL_GMER_VALID_COMPONENT_ID_FORMAT, __entry->comp_id),
>+		__entry->hpa, __get_str(region_name), &__entry->region_uuid,
>+		__entry->cme_threshold_ev_flags, __entry->cme_count
> 	)
> );
>
>diff --git a/include/cxl/event.h b/include/cxl/event.h index
>e1d485ad376b..2b07adf39010 100644
>--- a/include/cxl/event.h
>+++ b/include/cxl/event.h
>@@ -45,14 +45,17 @@ struct cxl_event_generic {
>
> /*
>  * General Media Event Record
>- * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
>+ * CXL rev 3.1 Section 8.2.9.2.1.1; Table 8-45
>  */
> #define CXL_EVENT_GEN_MED_COMP_ID_SIZE	0x10
> struct cxl_event_gen_media {
> 	struct cxl_event_media_hdr media_hdr;
> 	u8 device[3];
> 	u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
>-	u8 reserved[46];
>+	u8 cme_threshold_ev_flags;
>+	u8 cme_count[3];
>+	u8 sub_type;
>+	u8 reserved[41];
> } __packed;
>
> /*
>--
>2.43.0
Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Steven Rostedt 1 year, 2 months ago
On Tue, 26 Nov 2024 11:51:23 +0000
Shiju Jose <shiju.jose@huawei.com> wrote:

> We are encountering a parsing error ("FAILED TO PARSE") from libtraceevent  when it
> tries to parse some of the CXL trace events for the user-space tool rasdaemon. 
> This issue appeared after new fields were added to the trace events. 
> It was found that the issue does not occur when all or some of the decoded strings
> for the event's data and flags are removed from the TP_printk() function in the kernel,
> and only the values are printed instead.
> https://elixir.bootlin.com/linux/v6.12/source/drivers/cxl/core/trace.h
> https://lore.kernel.org/lkml/20241120093745.1847-1-shiju.jose@huawei.com/
> 
> Below is the information from the debugging in libtraceevent:
> The failure occurs in the following functions and locations within libtraceevent:
> File: src/event-parse.c
> Function: event_read_format()
> ret = event_read_fields(event->tep, event, &event->format.fields); if (ret < 0)
>     return ret;
> 
> Function: event_read_fields()
> if (test_type_token(type, token, TEP_EVENT_ITEM, "field"))
>     goto fail;
> 
> Can you recognize if there are any limitations or issues  that would prevent
> libtraceevent from parsing the trace event in the condition described above?

Can you show me the output of the format files for the affected trace events:

 # cat /sys/kernel/tracing/cxl/<affected_event>/format

You can attach it too if your email does whitespace mangling.

Thanks,

-- Steve
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
>-----Original Message-----
>From: Steven Rostedt <rostedt@goodmis.org>
>Sent: 26 November 2024 17:03
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>On Tue, 26 Nov 2024 11:51:23 +0000
>Shiju Jose <shiju.jose@huawei.com> wrote:
>
>> We are encountering a parsing error ("FAILED TO PARSE") from
>> libtraceevent  when it tries to parse some of the CXL trace events for the user-
>space tool rasdaemon.
>> This issue appeared after new fields were added to the trace events.
>> It was found that the issue does not occur when all or some of the
>> decoded strings for the event's data and flags are removed from the
>> TP_printk() function in the kernel, and only the values are printed instead.
>> https://elixir.bootlin.com/linux/v6.12/source/drivers/cxl/core/trace.h
>> https://lore.kernel.org/lkml/20241120093745.1847-1-shiju.jose@huawei.c
>> om/
>>
>> Below is the information from the debugging in libtraceevent:
>> The failure occurs in the following functions and locations within libtraceevent:
>> File: src/event-parse.c
>> Function: event_read_format()
>> ret = event_read_fields(event->tep, event, &event->format.fields); if (ret < 0)
>>     return ret;
>>
>> Function: event_read_fields()
>> if (test_type_token(type, token, TEP_EVENT_ITEM, "field"))
>>     goto fail;
>>
>> Can you recognize if there are any limitations or issues  that would
>> prevent libtraceevent from parsing the trace event in the condition described
>above?
>
>Can you show me the output of the format files for the affected trace events:
>
> # cat /sys/kernel/tracing/cxl/<affected_event>/format
>
>You can attach it too if your email does whitespace mangling.

Hi Steve, 

Please find attached, output of  format file for the CXL general media trace event.

>
>Thanks,
>
>-- Steve
Thanks,
Shiju
root@localhost:~# cat /sys/kernel/debug/tracing/events/cxl/cxl_general_media/format 
name: cxl_general_media
ID: 1464
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:__data_loc char[] memdev;	offset:8;	size:4;	signed:0;
	field:__data_loc char[] host;	offset:12;	size:4;	signed:0;
	field:int log;	offset:16;	size:4;	signed:1;
	field:uuid_t hdr_uuid;	offset:20;	size:16;	signed:0;
	field:u64 serial;	offset:40;	size:8;	signed:0;
	field:u32 hdr_flags;	offset:48;	size:4;	signed:0;
	field:u16 hdr_handle;	offset:52;	size:2;	signed:0;
	field:u16 hdr_related_handle;	offset:54;	size:2;	signed:0;
	field:u64 hdr_timestamp;	offset:56;	size:8;	signed:0;
	field:u8 hdr_length;	offset:64;	size:1;	signed:0;
	field:u8 hdr_maint_op_class;	offset:65;	size:1;	signed:0;
	field:u8 hdr_maint_op_sub_class;	offset:66;	size:1;	signed:0;
	field:u64 dpa;	offset:72;	size:8;	signed:0;
	field:u8 descriptor;	offset:80;	size:1;	signed:0;
	field:u8 type;	offset:81;	size:1;	signed:0;
	field:u8 transaction_type;	offset:82;	size:1;	signed:0;
	field:u8 channel;	offset:83;	size:1;	signed:0;
	field:u32 device;	offset:84;	size:4;	signed:0;
	field:u8 comp_id[16];	offset:88;	size:16;	signed:0;
	field:u64 hpa;	offset:104;	size:8;	signed:0;
	field:uuid_t region_uuid;	offset:112;	size:16;	signed:0;
	field:u16 validity_flags;	offset:128;	size:2;	signed:0;
	field:u8 rank;	offset:130;	size:1;	signed:0;
	field:u8 dpa_flags;	offset:131;	size:1;	signed:0;
	field:__data_loc char[] region_name;	offset:132;	size:4;	signed:0;
	field:u8 sub_type;	offset:136;	size:1;	signed:0;
	field:u8 cme_threshold_ev_flags;	offset:137;	size:1;	signed:0;
	field:u32 cme_count;	offset:140;	size:4;	signed:0;

print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s' comp_id=%shpa=%llx region=%s region_uuid=%pUb sub_type=%u cme_threshold_ev_flags=%u cme_count=%u", __get_str(memdev), __get_str(host), REC->serial, __print_symbolic(REC->log, { CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN, "Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL, "Fatal" }), REC->hdr_timestamp, &REC->hdr_uuid, REC->hdr_length, __print_flags(REC->hdr_flags, " | ", { ((((1UL))) << (2)), "PERMANENT_CONDITION" }, { ((((1UL))) << (3)), "MAINTENANCE_NEEDED" }, { ((((1UL))) << (4)), "PERFORMANCE_DEGRADED" }, { ((((1UL))) << (5)), "HARDWARE_REPLACEMENT_NEEDED" }, { ((((1UL))) << (6)), "MAINT_OP_SUB_CLASS_VALID" } ), REC->hdr_handle, REC->hdr_related_handle, REC->hdr_maint_op_class, REC->hdr_maint_op_sub_class, REC->dpa, __print_flags(REC->dpa_flags, "|", { ((((1UL))) << (0)), "VOLATILE" }, { ((((1UL))) << (1)), "NOT_REPAIRABLE" } ), __print_flags(REC->descriptor, "|", { ((((1UL))) << (0)), "UNCORRECTABLE_EVENT" }, { ((((1UL))) << (1)), "THRESHOLD_EVENT" }, { ((((1UL))) << (2)), "POISON_LIST_OVERFLOW" } ), __print_symbolic(REC->type, { 0x00, "ECC Error" }, { 0x01, "Invalid Address" }, { 0x02, "Data Path Error" }, { 0x03, "TE State Violation" }, { 0x04, "Scrub Media ECC Error" }, { 0x05, "Adv Prog CME Counter Expiration" }, { 0x06, "CKID Violation" } ), __print_symbolic(REC->transaction_type, { 0x00, "Unknown" }, { 0x01, "Host Read" }, { 0x02, "Host Write" }, { 0x03, "Host Scan Media" }, { 0x04, "Host Inject Poison" }, { 0x05, "Internal Media Scrub" }, { 0x06, "Internal Media Management" }, { 0x07, "Internal Media Error Check Scrub" }, { 0x08, "Media Initialization" } ), REC->channel, REC->rank, REC->device, __print_flags(REC->validity_flags, "|", { ((((1UL))) << (0)), "CHANNEL" }, { ((((1UL))) << (1)), "RANK" }, { ((((1UL))) << (2)), "DEVICE" }, { ((((1UL))) << (3)), "COMPONENT" }, { ((((1UL))) << (4)), "COMPONENT PLDM FORMAT" } ), __print_hex(REC->comp_id, 0x10), REC->hpa, __get_str(region_name), &REC->region_uuid, REC->sub_type, REC->cme_threshold_ev_flags, REC->cme_count
root@localhost:~#

Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Steven Rostedt 1 year, 2 months ago
On Wed, 27 Nov 2024 10:12:12 +0000
Shiju Jose <shiju.jose@huawei.com> wrote:

> format:
> 	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
> 	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
> 	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
> 	field:int common_pid;	offset:4;	size:4;	signed:1;
> 
> 	field:__data_loc char[] memdev;	offset:8;	size:4;	signed:0;
> 	field:__data_loc char[] host;	offset:12;	size:4;	signed:0;
> 	field:int log;	offset:16;	size:4;	signed:1;

> 	field:uuid_t hdr_uuid;	offset:20;	size:16;	signed:0;

New type for me ;-)

> 	field:u64 serial;	offset:40;	size:8;	signed:0;
> 	field:u32 hdr_flags;	offset:48;	size:4;	signed:0;
> 	field:u16 hdr_handle;	offset:52;	size:2;	signed:0;
> 	field:u16 hdr_related_handle;	offset:54;	size:2;	signed:0;
> 	field:u64 hdr_timestamp;	offset:56;	size:8;	signed:0;
> 	field:u8 hdr_length;	offset:64;	size:1;	signed:0;
> 	field:u8 hdr_maint_op_class;	offset:65;	size:1;	signed:0;
> 	field:u8 hdr_maint_op_sub_class;	offset:66;	size:1;	signed:0;
> 	field:u64 dpa;	offset:72;	size:8;	signed:0;
> 	field:u8 descriptor;	offset:80;	size:1;	signed:0;
> 	field:u8 type;	offset:81;	size:1;	signed:0;
> 	field:u8 transaction_type;	offset:82;	size:1;	signed:0;
> 	field:u8 channel;	offset:83;	size:1;	signed:0;
> 	field:u32 device;	offset:84;	size:4;	signed:0;
> 	field:u8 comp_id[16];	offset:88;	size:16;	signed:0;
> 	field:u64 hpa;	offset:104;	size:8;	signed:0;
> 	field:uuid_t region_uuid;	offset:112;	size:16;	signed:0;
> 	field:u16 validity_flags;	offset:128;	size:2;	signed:0;
> 	field:u8 rank;	offset:130;	size:1;	signed:0;
> 	field:u8 dpa_flags;	offset:131;	size:1;	signed:0;
> 	field:__data_loc char[] region_name;	offset:132;	size:4;	signed:0;
> 	field:u8 sub_type;	offset:136;	size:1;	signed:0;
> 	field:u8 cme_threshold_ev_flags;	offset:137;	size:1;	signed:0;
> 	field:u32 cme_count;	offset:140;	size:4;	signed:0;
> 
> print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s' comp_id=%shpa=%llx region=%s region_uuid=%pUb sub_type=%u cme_threshold_ev_flags=%u cme_count=%u", __get_str(memdev), __get_str(host), REC->serial, __print_symbolic(REC->log, { CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN, "Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL, "Fatal" }), REC->hdr_timestamp,


> &REC->hdr_uuid,

libtraceevent doesn't know about taking an address with '&'.

If I remove it (and the other one below for region_uuid), it parses fine.

I'll have to add this to the library, as it should be able to handle this.
I bet I also have to add "%pUb" as well.

Thanks,

-- Steve



> REC->hdr_length, __print_flags(REC->hdr_flags, " | ", { ((((1UL))) << (2)), "PERMANENT_CONDITION" }, { ((((1UL))) << (3)), "MAINTENANCE_NEEDED" }, { ((((1UL))) << (4)), "PERFORMANCE_DEGRADED" }, { ((((1UL))) << (5)), "HARDWARE_REPLACEMENT_NEEDED" }, { ((((1UL))) << (6)), "MAINT_OP_SUB_CLASS_VALID" } ), REC->hdr_handle, REC->hdr_related_handle, REC->hdr_maint_op_class, REC->hdr_maint_op_sub_class, REC->dpa, __print_flags(REC->dpa_flags, "|", { ((((1UL))) << (0)), "VOLATILE" }, { ((((1UL))) << (1)), "NOT_REPAIRABLE" } ), __print_flags(REC->descriptor, "|", { ((((1UL))) << (0)), "UNCORRECTABLE_EVENT" }, { ((((1UL))) << (1)), "THRESHOLD_EVENT" }, { ((((1UL))) << (2)), "POISON_LIST_OVERFLOW" } ), __print_symbolic(REC->type, { 0x00, "ECC Error" }, { 0x01, "Invalid Address" }, { 0x02, "Data Path Error" }, { 0x03, "TE State Violation" }, { 0x04, "Scrub Media ECC Error" }, { 0x05, "Adv Prog CME Counter Expiration" }, { 0x06, "CKID Violation" } ), __print_symbolic(REC->transaction_type, { 0x00, "Unknown" }, { 0x01, "Host Read" }, { 0x02, "Host Write" }, { 0x03, "Host Scan Media" }, { 0x04, "Host Inject Poison" }, { 0x05, "Internal Media Scrub" }, { 0x06, "Internal Media Management" }, { 0x07, "Internal Media Error Check Scrub" }, { 0x08, "Media Initialization" } ), REC->channel, REC->rank, REC->device, __print_flags(REC->validity_flags, "|", { ((((1UL))) << (0)), "CHANNEL" }, { ((((1UL))) << (1)), "RANK" }, { ((((1UL))) << (2)), "DEVICE" }, { ((((1UL))) << (3)), "COMPONENT" }, { ((((1UL))) << (4)), "COMPONENT PLDM FORMAT" } ), __print_hex(REC->comp_id, 0x10), REC->hpa, __get_str(region_name), &REC->region_uuid, REC->sub_type, REC->cme_threshold_ev_flags, REC->cme_count
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
Hi Steve,

Thanks for the quick reply.
Please find reply inline.

>-----Original Message-----
>From: Steven Rostedt <rostedt@goodmis.org>
>Sent: 27 November 2024 15:42
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>On Wed, 27 Nov 2024 10:12:12 +0000
>Shiju Jose <shiju.jose@huawei.com> wrote:
>
>> format:
>> 	field:unsigned short common_type;	offset:0;	size:2;
>	signed:0;
>> 	field:unsigned char common_flags;	offset:2;	size:1;
>	signed:0;
>> 	field:unsigned char common_preempt_count;	offset:3;	size:1;
>	signed:0;
>> 	field:int common_pid;	offset:4;	size:4;	signed:1;
>>
>> 	field:__data_loc char[] memdev;	offset:8;	size:4;
>	signed:0;
>> 	field:__data_loc char[] host;	offset:12;	size:4;	signed:0;
>> 	field:int log;	offset:16;	size:4;	signed:1;
>
>> 	field:uuid_t hdr_uuid;	offset:20;	size:16;	signed:0;
>
>New type for me ;-)
>
>> 	field:u64 serial;	offset:40;	size:8;	signed:0;
>> 	field:u32 hdr_flags;	offset:48;	size:4;	signed:0;
>> 	field:u16 hdr_handle;	offset:52;	size:2;	signed:0;
>> 	field:u16 hdr_related_handle;	offset:54;	size:2;	signed:0;
>> 	field:u64 hdr_timestamp;	offset:56;	size:8;	signed:0;
>> 	field:u8 hdr_length;	offset:64;	size:1;	signed:0;
>> 	field:u8 hdr_maint_op_class;	offset:65;	size:1;	signed:0;
>> 	field:u8 hdr_maint_op_sub_class;	offset:66;	size:1;
>	signed:0;
>> 	field:u64 dpa;	offset:72;	size:8;	signed:0;
>> 	field:u8 descriptor;	offset:80;	size:1;	signed:0;
>> 	field:u8 type;	offset:81;	size:1;	signed:0;
>> 	field:u8 transaction_type;	offset:82;	size:1;	signed:0;
>> 	field:u8 channel;	offset:83;	size:1;	signed:0;
>> 	field:u32 device;	offset:84;	size:4;	signed:0;
>> 	field:u8 comp_id[16];	offset:88;	size:16;	signed:0;
>> 	field:u64 hpa;	offset:104;	size:8;	signed:0;
>> 	field:uuid_t region_uuid;	offset:112;	size:16;	signed:0;
>> 	field:u16 validity_flags;	offset:128;	size:2;	signed:0;
>> 	field:u8 rank;	offset:130;	size:1;	signed:0;
>> 	field:u8 dpa_flags;	offset:131;	size:1;	signed:0;
>> 	field:__data_loc char[] region_name;	offset:132;	size:4;
>	signed:0;
>> 	field:u8 sub_type;	offset:136;	size:1;	signed:0;
>> 	field:u8 cme_threshold_ev_flags;	offset:137;	size:1;
>	signed:0;
>> 	field:u32 cme_count;	offset:140;	size:4;	signed:0;
>>
>> print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb
>len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u
>maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s'
>transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s'
>comp_id=%shpa=%llx region=%s region_uuid=%pUb sub_type=%u
>cme_threshold_ev_flags=%u cme_count=%u", __get_str(memdev),
>__get_str(host), REC->serial, __print_symbolic(REC->log, {
>CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN,
>"Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL,
>"Fatal" }), REC->hdr_timestamp,
>
>
>> &REC->hdr_uuid,
>
>libtraceevent doesn't know about taking an address with '&'.
>
>If I remove it (and the other one below for region_uuid), it parses fine.
>
>I'll have to add this to the library, as it should be able to handle this.
>I bet I also have to add "%pUb" as well.
>
I tested removing hdr_uuid and region_uuid from the rasdaemon test setup
as you suggested. As a result, libtraceevent parses correctly, as you mentioned.

However, I encounter  similar parsing error ("FAILED TO PARSE") when I add two additional
decoded strings (%s) to the TP_printk, replacing (%u). Please see the attached format file,
"format_cxl_general_media_v3.1_basic", for your reference.

I've also attached another format file, "format_cxl_general_media_v3.1_full",
which contains the complete TP_printk() intended.
 
Can you please help or else can you share how to debug these errors in the
libtraceevent setup?

>Thanks,
>
>-- Steve
>
Thanks,
Shiju
root@localhost:~# cat /sys/kernel/debug/tracing/events/cxl/cxl_general_media/format
name: cxl_general_media
ID: 1464
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:__data_loc char[] memdev;	offset:8;	size:4;	signed:0;
	field:__data_loc char[] host;	offset:12;	size:4;	signed:0;
	field:int log;	offset:16;	size:4;	signed:1;
	field:u64 serial;	offset:24;	size:8;	signed:0;
	field:u32 hdr_flags;	offset:32;	size:4;	signed:0;
	field:u16 hdr_handle;	offset:36;	size:2;	signed:0;
	field:u16 hdr_related_handle;	offset:38;	size:2;	signed:0;
	field:u64 hdr_timestamp;	offset:40;	size:8;	signed:0;
	field:u8 hdr_length;	offset:48;	size:1;	signed:0;
	field:u8 hdr_maint_op_class;	offset:49;	size:1;	signed:0;
	field:u8 hdr_maint_op_sub_class;	offset:50;	size:1;	signed:0;
	field:u64 dpa;	offset:56;	size:8;	signed:0;
	field:u8 descriptor;	offset:64;	size:1;	signed:0;
	field:u8 type;	offset:65;	size:1;	signed:0;
	field:u8 sub_type;	offset:66;	size:1;	signed:0;
	field:u8 transaction_type;	offset:67;	size:1;	signed:0;
	field:u8 channel;	offset:68;	size:1;	signed:0;
	field:u32 device;	offset:72;	size:4;	signed:0;
	field:u8 comp_id[16];	offset:76;	size:16;	signed:0;
	field:u64 hpa;	offset:96;	size:8;	signed:0;
	field:u16 validity_flags;	offset:104;	size:2;	signed:0;
	field:u8 rank;	offset:106;	size:1;	signed:0;
	field:u8 dpa_flags;	offset:107;	size:1;	signed:0;
	field:u8 cme_threshold_ev_flags;	offset:108;	size:1;	signed:0;
	field:u32 cme_count;	offset:112;	size:4;	signed:0;
	field:__data_loc char[] region_name;	offset:116;	size:4;	signed:0;

print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' sub_type='%s' transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s' comp_id=%s comp_id_pldm_valid_flags='%s' pldm_entity_id=%s pldm_resource_id=%s hpa=%llx cme_threshold_ev_flags='%s' cme_count=%x region=%s", __get_str(memdev), __get_str(host), REC->serial, __print_symbolic(REC->log, { CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN, "Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL, "Fatal" }), REC->hdr_timestamp, REC->hdr_length, __print_flags(REC->hdr_flags, " | ", { ((((1UL))) << (2)), "PERMANENT_CONDITION" }, { ((((1UL))) << (3)), "MAINTENANCE_NEEDED" }, { ((((1UL))) << (4)), "PERFORMANCE_DEGRADED" }, { ((((1UL))) << (5)), "HARDWARE_REPLACEMENT_NEEDED" }, { ((((1UL))) << (6)), "MAINT_OP_SUB_CLASS_VALID" } ), REC->hdr_handle, REC->hdr_related_handle, REC->hdr_maint_op_class, REC->hdr_maint_op_sub_class, REC->dpa, __print_flags(REC->dpa_flags, "|", { ((((1UL))) << (0)), "VOLATILE" }, { ((((1UL))) << (1)), "NOT_REPAIRABLE" } ), __print_flags(REC->descriptor, "|", { ((((1UL))) << (0)), "UNCORRECTABLE_EVENT" }, { ((((1UL))) << (1)), "THRESHOLD_EVENT" }, { ((((1UL))) << (2)), "POISON_LIST_OVERFLOW" } ), __print_symbolic(REC->type, { 0x00, "ECC Error" }, { 0x01, "Invalid Address" }, { 0x02, "Data Path Error" }, { 0x03, "TE State Violation" }, { 0x04, "Scrub Media ECC Error" }, { 0x05, "Adv Prog CME Counter Expiration" }, { 0x06, "CKID Violation" } ), __print_symbolic(REC->sub_type, { 0x00, "Not Reported" }, { 0x01, "Internal Datapath Error" }, { 0x02, "Media Link Command Training Error" }, { 0x03, "Media Link Control Training Error" }, { 0x04, "Media Link Data Training Error" }, { 0x05, "Media Link CRC Error" } ), __print_symbolic(REC->transaction_type, { 0x00, "Unknown" }, { 0x01, "Host Read" }, { 0x02, "Host Write" }, { 0x03, "Host Scan Media" }, { 0x04, "Host Inject Poison" }, { 0x05, "Internal Media Scrub" }, { 0x06, "Internal Media Management" }, { 0x07, "Internal Media Error Check Scrub" }, { 0x08, "Media Initialization" } ), REC->channel, REC->rank, REC->device, __print_flags(REC->validity_flags, "|", { ((((1UL))) << (0)), "CHANNEL" }, { ((((1UL))) << (1)), "RANK" }, { ((((1UL))) << (2)), "DEVICE" }, { ((((1UL))) << (3)), "COMPONENT" }, { ((((1UL))) << (4)), "COMPONENT PLDM FORMAT" } ), __print_hex(REC->comp_id, 0x10), __print_flags(REC->comp_id[0], " | ", { ((((1UL))) << (0)), "PLDM Entity ID" }, { ((((1UL))) << (1)), "Resource ID" } ), (REC->validity_flags & ((((1UL))) << (3)) && REC->validity_flags & ((((1UL))) << (4))) ? (REC->comp_id[0] & ((((1UL))) << (0))) ? __print_hex(&REC->comp_id[1], 6) : "0x00" : "0x00", (REC->validity_flags & ((((1UL))) << (3)) && REC->validity_flags & ((((1UL))) << (4))) ? (REC->comp_id[0] & ((((1UL))) << (1))) ? __print_hex(&REC->comp_id[7], 4) : "0x00" : "0x00", REC->hpa, __print_flags(REC->cme_threshold_ev_flags, "|", { ((((1UL))) << (0)), "Corrected Memory Errors in Multiple Media Components" }, { ((((1UL))) << (1)), "Exceeded Programmable Threshold" } ), REC->cme_count, __get_str(region_name)

root@localhost:~#  cat /sys/kernel/debug/tracing/events/cxl/cxl_general_media/format 
name: cxl_general_media
ID: 1464
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:__data_loc char[] memdev;	offset:8;	size:4;	signed:0;
	field:__data_loc char[] host;	offset:12;	size:4;	signed:0;
	field:int log;	offset:16;	size:4;	signed:1;
	field:u64 serial;	offset:24;	size:8;	signed:0;
	field:u32 hdr_flags;	offset:32;	size:4;	signed:0;
	field:u16 hdr_handle;	offset:36;	size:2;	signed:0;
	field:u16 hdr_related_handle;	offset:38;	size:2;	signed:0;
	field:u64 hdr_timestamp;	offset:40;	size:8;	signed:0;
	field:u8 hdr_length;	offset:48;	size:1;	signed:0;
	field:u8 hdr_maint_op_class;	offset:49;	size:1;	signed:0;
	field:u8 hdr_maint_op_sub_class;	offset:50;	size:1;	signed:0;
	field:u64 dpa;	offset:56;	size:8;	signed:0;
	field:u8 descriptor;	offset:64;	size:1;	signed:0;
	field:u8 type;	offset:65;	size:1;	signed:0;
	field:u8 transaction_type;	offset:66;	size:1;	signed:0;
	field:u8 channel;	offset:67;	size:1;	signed:0;
	field:u32 device;	offset:68;	size:4;	signed:0;
	field:u8 comp_id[16];	offset:72;	size:16;	signed:0;
	field:u64 hpa;	offset:88;	size:8;	signed:0;
	field:u16 validity_flags;	offset:96;	size:2;	signed:0;
	field:u8 rank;	offset:98;	size:1;	signed:0;
	field:u8 dpa_flags;	offset:99;	size:1;	signed:0;
	field:__data_loc char[] region_name;	offset:100;	size:4;	signed:0;
	field:u8 sub_type;	offset:104;	size:1;	signed:0;
	field:u8 cme_threshold_ev_flags;	offset:105;	size:1;	signed:0;
	field:u32 cme_count;	offset:108;	size:4;	signed:0;

print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s' comp_id=%s hpa=%llx region=%s sub_type='%s' cme_threshold_ev_flags='%s' cme_count=%u", __get_str(memdev), __get_str(host), REC->serial, __print_symbolic(REC->log, { CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN, "Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL, "Fatal" }), REC->hdr_timestamp, REC->hdr_length, __print_flags(REC->hdr_flags, " | ", { ((((1UL))) << (2)), "PERMANENT_CONDITION" }, { ((((1UL))) << (3)), "MAINTENANCE_NEEDED" }, { ((((1UL))) << (4)), "PERFORMANCE_DEGRADED" }, { ((((1UL))) << (5)), "HARDWARE_REPLACEMENT_NEEDED" }, { ((((1UL))) << (6)), "MAINT_OP_SUB_CLASS_VALID" } ), REC->hdr_handle, REC->hdr_related_handle, REC->hdr_maint_op_class, REC->hdr_maint_op_sub_class, REC->dpa, __print_flags(REC->dpa_flags, "|", { ((((1UL))) << (0)), "VOLATILE" }, { ((((1UL))) << (1)), "NOT_REPAIRABLE" } ), __print_flags(REC->descriptor, "|", { ((((1UL))) << (0)), "UNCORRECTABLE_EVENT" }, { ((((1UL))) << (1)), "THRESHOLD_EVENT" }, { ((((1UL))) << (2)), "POISON_LIST_OVERFLOW" } ), __print_symbolic(REC->type, { 0x00, "ECC Error" }, { 0x01, "Invalid Address" }, { 0x02, "Data Path Error" }, { 0x03, "TE State Violation" }, { 0x04, "Scrub Media ECC Error" }, { 0x05, "Adv Prog CME Counter Expiration" }, { 0x06, "CKID Violation" } ), __print_symbolic(REC->transaction_type, { 0x00, "Unknown" }, { 0x01, "Host Read" }, { 0x02, "Host Write" }, { 0x03, "Host Scan Media" }, { 0x04, "Host Inject Poison" }, { 0x05, "Internal Media Scrub" }, { 0x06, "Internal Media Management" }, { 0x07, "Internal Media Error Check Scrub" }, { 0x08, "Media Initialization" } ), REC->channel, REC->rank, REC->device, __print_flags(REC->validity_flags, "|", { ((((1UL))) << (0)), "CHANNEL" }, { ((((1UL))) << (1)), "RANK" }, { ((((1UL))) << (2)), "DEVICE" }, { ((((1UL))) << (3)), "COMPONENT" }, { ((((1UL))) << (4)), "COMPONENT PLDM FORMAT" } ), __print_hex(REC->comp_id, 0x10), REC->hpa, __get_str(region_name), __print_symbolic(REC->sub_type, { 0x00, "Not Reported" }, { 0x01, "Internal Datapath Error" }, { 0x02, "Media Link Command Training Error" }, { 0x03, "Media Link Control Training Error" }, { 0x04, "Media Link Data Training Error" }, { 0x05, "Media Link CRC Error" } ), __print_flags(REC->cme_threshold_ev_flags, "|", { ((((1UL))) << (0)), "Corrected Memory Errors in Multiple Media Components" }, { ((((1UL))) << (1)), "Exceeded Programmable Threshold" } ), REC->cme_count
root@localhost:~#
Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Steven Rostedt 1 year, 2 months ago
On Wed, 27 Nov 2024 18:20:26 +0000
Shiju Jose <shiju.jose@huawei.com> wrote:

> I tested removing hdr_uuid and region_uuid from the rasdaemon test setup
> as you suggested. As a result, libtraceevent parses correctly, as you mentioned.
> 
> However, I encounter  similar parsing error ("FAILED TO PARSE") when I add two additional
> decoded strings (%s) to the TP_printk, replacing (%u). Please see the attached format file,
> "format_cxl_general_media_v3.1_basic", for your reference.

Are you sure. I don't see anything wrong with that one. Which version of
libtraceevent do you have?

> 
> I've also attached another format file, "format_cxl_general_media_v3.1_full",
> which contains the complete TP_printk() intended.

This one has some complex arguments and also uses the '&' address of an
argument.

>  
> Can you please help or else can you share how to debug these errors in the
> libtraceevent setup?

Basically, I use the attached program (that just links to libtraceevent).

Note, I need to delete the first line of your files, which has the "cat"
command. But you can run this on the file itself:

  ./tep_load_event /sys/kernel/tracing/events/cxl/cxl_general_media/format

But you may need to be root to do that. If root just copies that file, you
can then run it as non-root.

 # cp /sys/kernel/tracing/events/cxl/cxl_general_media/format /tmp
 $ ./tep_load_event /tmp/format

I run it under gdb and see where it fails. But it should let you know if it
will pass or not. I put a breakpoint on tep_warning and when it gets hit, I
examine what it did up to that point.

-- Steve

RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
>-----Original Message-----
>From: Steven Rostedt <rostedt@goodmis.org>
>Sent: 27 November 2024 18:34
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: Re: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>On Wed, 27 Nov 2024 18:20:26 +0000
>Shiju Jose <shiju.jose@huawei.com> wrote:
>
>> I tested removing hdr_uuid and region_uuid from the rasdaemon test
>> setup as you suggested. As a result, libtraceevent parses correctly, as you
>mentioned.
>>
>> However, I encounter  similar parsing error ("FAILED TO PARSE") when I
>> add two additional decoded strings (%s) to the TP_printk, replacing
>> (%u). Please see the attached format file,
>"format_cxl_general_media_v3.1_basic", for your reference.
>
>Are you sure. I don't see anything wrong with that one. Which version of
>libtraceevent do you have?

libtraceevent source code version 1.8.4,  build for arm64.

>
>>
>> I've also attached another format file,
>> "format_cxl_general_media_v3.1_full",
>> which contains the complete TP_printk() intended.
>
>This one has some complex arguments and also uses the '&' address of an
>argument.
Thanks.
I will debug this when basic one is working.
>
>>
>> Can you please help or else can you share how to debug these errors in
>> the libtraceevent setup?
>
>Basically, I use the attached program (that just links to libtraceevent).
>
>Note, I need to delete the first line of your files, which has the "cat"
>command. But you can run this on the file itself:
>
>  ./tep_load_event /sys/kernel/tracing/events/cxl/cxl_general_media/format
>
>But you may need to be root to do that. If root just copies that file, you can then
>run it as non-root.
>
> # cp /sys/kernel/tracing/events/cxl/cxl_general_media/format /tmp  $
>./tep_load_event /tmp/format
>
>I run it under gdb and see where it fails. But it should let you know if it will pass
>or not. I put a breakpoint on tep_warning and when it gets hit, I examine what it
>did up to that point.

Thanks Steve for the instructions. I will try this.
>
>-- Steve

Thanks,
Shiju
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
>-----Original Message-----
>From: Shiju Jose <shiju.jose@huawei.com>
>Sent: 28 November 2024 10:02
>To: Steven Rostedt <rostedt@goodmis.org>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>>-----Original Message-----
>>From: Steven Rostedt <rostedt@goodmis.org>
>>Sent: 27 November 2024 18:34
>>To: Shiju Jose <shiju.jose@huawei.com>
>>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
>><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>>dave@stgolabs.net; linux-cxl@vger.kernel.org;
>>linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>>tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
>><prime.zeng@hisilicon.com>
>>Subject: Re: [PATCH v4 3/6] cxl/events: Update General Media Event
>>Record to CXL spec rev 3.1
>>
>>On Wed, 27 Nov 2024 18:20:26 +0000
>>Shiju Jose <shiju.jose@huawei.com> wrote:
>>
>>> I tested removing hdr_uuid and region_uuid from the rasdaemon test
>>> setup as you suggested. As a result, libtraceevent parses correctly,
>>> as you
>>mentioned.
>>>
>>> However, I encounter  similar parsing error ("FAILED TO PARSE") when
>>> I add two additional decoded strings (%s) to the TP_printk, replacing
>>> (%u). Please see the attached format file,
>>"format_cxl_general_media_v3.1_basic", for your reference.
>>
>>Are you sure. I don't see anything wrong with that one. Which version
>>of libtraceevent do you have?
>
>libtraceevent source code version 1.8.4,  build for arm64.

Hi Steve,

I debug this case and please find the info,
1. rasdaemon :  read() from format file return around 1/3rd of the 
    actual data in the file only when the total size of the format's file
    is above 4K bytes (page size), For example, in this case, the total size was 4512 bytes,
    but read only 1674 bytes.
    This partial data resulted  tep_parse_event() in libtraceevent failed internally in the parse_format()     
    and  since __parse_event() does not return error when parse_format() fail,
    thus initialization of the event does not fail.
     
   The following  solution in rasdaemon solved the issue, 
   (provided if no other fix for the above issue with read()), 
    1. read() and accumulate content of format file until EOF reached.
    2. Increased the buffer size from 4K bytes to  8K bytes.
    3. May be __parse_event()  in libtraceevent  and thus tep_parse_event()  return error
        when parse_format() fail so that the initialization would fail if the input data is
        corrupted or partial?   
>
>>
>>>
>>> I've also attached another format file,
>>> "format_cxl_general_media_v3.1_full",
>>> which contains the complete TP_printk() intended.
>>
>>This one has some complex arguments and also uses the '&' address of an
>>argument.
>Thanks.
>I will debug this when basic one is working.

This case too worked  with above workaround after removing '&' address
from the TP_printk().
>>
>>>
>>> Can you please help or else can you share how to debug these errors
>>> in the libtraceevent setup?
>>
>>Basically, I use the attached program (that just links to libtraceevent).
>>
>>Note, I need to delete the first line of your files, which has the "cat"
>>command. But you can run this on the file itself:
>>
>>  ./tep_load_event
>> /sys/kernel/tracing/events/cxl/cxl_general_media/format
>>
>>But you may need to be root to do that. If root just copies that file,
>>you can then run it as non-root.
>>
>> # cp /sys/kernel/tracing/events/cxl/cxl_general_media/format /tmp  $
>>./tep_load_event /tmp/format
>>
>>I run it under gdb and see where it fails. But it should let you know
>>if it will pass or not. I put a breakpoint on tep_warning and when it
>>gets hit, I examine what it did up to that point.
>
>Thanks Steve for the instructions. I will try this.

I built tep_load_event.c for arm64 and it worked after copy
format file to the /tmp/ folder as you said.
>>
>>-- Steve
>

Thanks,
Shiju
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
>-----Original Message-----
>From: Shiju Jose <shiju.jose@huawei.com>
>Sent: 29 November 2024 13:23
>To: Shiju Jose <shiju.jose@huawei.com>; Steven Rostedt <rostedt@goodmis.org>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>>-----Original Message-----
>>From: Shiju Jose <shiju.jose@huawei.com>
>>Sent: 28 November 2024 10:02
>>To: Steven Rostedt <rostedt@goodmis.org>
>>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
>><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>>dave@stgolabs.net; linux-cxl@vger.kernel.org;
>>linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>>tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
>><prime.zeng@hisilicon.com>
>>Subject: RE: [PATCH v4 3/6] cxl/events: Update General Media Event
>>Record to CXL spec rev 3.1
>>
[...]
>
>Hi Steve,
>
>I debug this case and please find the info, 1. rasdaemon :  read() from format
>file return around 1/3rd of the
>    actual data in the file only when the total size of the format's file
>    is above 4K bytes (page size), For example, in this case, the total size was 4512
>bytes,
>    but read only 1674 bytes.
>    This partial data resulted  tep_parse_event() in libtraceevent failed internally
>in the parse_format()
>    and  since __parse_event() does not return error when parse_format() fail,
>    thus initialization of the event does not fail.
>
>   The following  solution in rasdaemon solved the issue,
>   (provided if no other fix for the above issue with read()),
>    1. read() and accumulate content of format file until EOF reached.
>    2. Increased the buffer size from 4K bytes to  8K bytes.
>    3. May be __parse_event()  in libtraceevent  and thus tep_parse_event()
>return error
>        when parse_format() fail so that the initialization would fail if the input
>data is
>        corrupted or partial?

Hi Steve,

Identified the root cause of this issue in the kernel:
The read() function for the event's format file calls seq_read() and seq_read_iter() in the kernel, 
which allocates a buffer of PAGE_SIZE (4 KB) for sequential reads. However, it
detects an overflow in the following marked call (seq_has_overflowed()) and 
returns with partial data read.

=====================================

File : https://elixir.bootlin.com/linux/v6.13-rc1/source/fs/seq_file.c

ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
	struct seq_file *m = iocb->ki_filp->private_data;
[...]
	/* grab buffer if we didn't have one */
	if (!m->buf) {
--------->		m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
		if (!m->buf)
			goto Enomem;
	}
	// something left in the buffer - copy it out first
[...]
	// get a non-empty record in the buffer
	m->from = 0;
	p = m->op->start(m, &m->index);
	while (1) {
		err = PTR_ERR(p);
		if (!p || IS_ERR(p))	// EOF or an error
			break;
[...]
	}
	// EOF or an error
	m->op->stop(m, p);
	m->count = 0;
	goto Done;
Fill:
	// one non-empty record is in the buffer; if they want more,
	// try to fit more in, but in any case we need to advance
	// the iterator once for every record shown.
	while (1) {
		size_t offs = m->count;
		loff_t pos = m->index;

[...]
		err = m->op->show(m, p);
		if (err > 0) {		// ->show() says "skip it"
			m->count = offs;
---------->	} else if (err || seq_has_overflowed(m)) {
			m->count = offs;
			break;
		}
	}
	m->op->stop(m, p);
	n = copy_to_iter(m->buf, m->count, iter);
	copied += n;
	m->count -= n;
	m->from = n;
Done:
[...]
	goto Done;
}
EXPORT_SYMBOL(seq_read_iter);
====================================

The following fix in the kernel's seq_read_iter() allows userspace to read the
content of the format file etc in one go if the requested size exceeds PAGE_SIZE,
and resolves the parsing error caused by the event's format file exceeding PAGE_SIZE.

====================================
diff --git a/fs/seq_file.c b/fs/seq_file.c index e676c8b0cf5d..f1f1af180562 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -207,7 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 
        /* grab buffer if we didn't have one */
        if (!m->buf) {
-               m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
+               if (iter->count % PAGE_SIZE)
+                       m->size = ((iter->count / PAGE_SIZE) + 1) * PAGE_SIZE;
+               else
+                       m->size = (iter->count / PAGE_SIZE) * PAGE_SIZE;
+                m->buf = seq_buf_alloc(m->size);
                if (!m->buf)
                        goto Enomem;
        }
====================================

Can you suggest which fix is the right one,  kernel based fix or rasdaemon based fix (which
mentioned in the previous email)? 

Thanks,
Shiju
>>
>>>
>>>>
>>>> I've also attached another format file,
>>>> "format_cxl_general_media_v3.1_full",
>>>> which contains the complete TP_printk() intended.
>>>
[...]
>>>-- Steve
>>
>
RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to CXL spec rev 3.1
Posted by Shiju Jose 1 year, 2 months ago
>-----Original Message-----
>From: Shiju Jose <shiju.jose@huawei.com>
>Sent: 03 December 2024 15:22
>To: Steven Rostedt <rostedt@goodmis.org>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; alison.schofield@intel.com;
>nifan.cxl@gmail.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org;
>Linuxarm <linuxarm@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>;
>Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: RE: [PATCH v4 3/6] cxl/events: Update General Media Event Record to
>CXL spec rev 3.1
>
>[...]
>>
>>Hi Steve,
>>
>>I debug this case and please find the info, 1. rasdaemon :  read() from
>>format file return around 1/3rd of the
>>    actual data in the file only when the total size of the format's file
>>    is above 4K bytes (page size), For example, in this case, the total
>>size was 4512 bytes,
>>    but read only 1674 bytes.
>>    This partial data resulted  tep_parse_event() in libtraceevent
>>failed internally in the parse_format()
>>    and  since __parse_event() does not return error when parse_format() fail,
>>    thus initialization of the event does not fail.
>>
>>   The following  solution in rasdaemon solved the issue,
>>   (provided if no other fix for the above issue with read()),
>>    1. read() and accumulate content of format file until EOF reached.
>>    2. Increased the buffer size from 4K bytes to  8K bytes.
>>    3. May be __parse_event()  in libtraceevent  and thus
>>tep_parse_event() return error
>>        when parse_format() fail so that the initialization would fail
>>if the input data is
>>        corrupted or partial?
>
>Hi Steve,
>
>Identified the root cause of this issue in the kernel:
>The read() function for the event's format file calls seq_read() and
>seq_read_iter() in the kernel, which allocates a buffer of PAGE_SIZE (4 KB) for
>sequential reads. However, it detects an overflow in the following marked call
>(seq_has_overflowed()) and returns with partial data read.
>
>=====================================
>
>File : https://elixir.bootlin.com/linux/v6.13-rc1/source/fs/seq_file.c
>
>ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter) {
>	struct seq_file *m = iocb->ki_filp->private_data; [...]
>	/* grab buffer if we didn't have one */
>	if (!m->buf) {
>--------->		m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
>		if (!m->buf)
>			goto Enomem;
>	}
>	// something left in the buffer - copy it out first [...]
>	// get a non-empty record in the buffer
>	m->from = 0;
>	p = m->op->start(m, &m->index);
>	while (1) {
>		err = PTR_ERR(p);
>		if (!p || IS_ERR(p))	// EOF or an error
>			break;
>[...]
>	}
>	// EOF or an error
>	m->op->stop(m, p);
>	m->count = 0;
>	goto Done;
>Fill:
>	// one non-empty record is in the buffer; if they want more,
>	// try to fit more in, but in any case we need to advance
>	// the iterator once for every record shown.
>	while (1) {
>		size_t offs = m->count;
>		loff_t pos = m->index;
>
>[...]
>		err = m->op->show(m, p);
>		if (err > 0) {		// ->show() says "skip it"
>			m->count = offs;
>---------->	} else if (err || seq_has_overflowed(m)) {
>			m->count = offs;
>			break;
>		}
>	}
>	m->op->stop(m, p);
>	n = copy_to_iter(m->buf, m->count, iter);
>	copied += n;
>	m->count -= n;
>	m->from = n;
>Done:
>[...]
>	goto Done;
>}
>EXPORT_SYMBOL(seq_read_iter);
>====================================
>
>The following fix in the kernel's seq_read_iter() allows userspace to read the
>content of the format file etc in one go if the requested size exceeds PAGE_SIZE,
>and resolves the parsing error caused by the event's format file exceeding
>PAGE_SIZE.
>
>====================================
>diff --git a/fs/seq_file.c b/fs/seq_file.c index e676c8b0cf5d..f1f1af180562
>100644
>--- a/fs/seq_file.c
>+++ b/fs/seq_file.c
>@@ -207,7 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter
>*iter)
>
>        /* grab buffer if we didn't have one */
>        if (!m->buf) {
>-               m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
>+               if (iter->count % PAGE_SIZE)
>+                       m->size = ((iter->count / PAGE_SIZE) + 1) * PAGE_SIZE;
>+               else
>+                       m->size = (iter->count / PAGE_SIZE) * PAGE_SIZE;
>+                m->buf = seq_buf_alloc(m->size);
>                if (!m->buf)
>                        goto Enomem;
>        }
>====================================
>
>Can you suggest which fix is the right one,  kernel based fix or rasdaemon based
>fix (which mentioned in the previous email)?

Following change for reading trace event's format file in the kernel worked fine to fix
the parsing error,  which seems better than changing the common seq_read(). 
============================================
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 7266ec2a4eea..9cb1c5a1f070 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1674,6 +1674,35 @@ static int trace_format_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+/**
+ *	trace_format_read - read() method for format file.
+ *	@file: the file to read from
+ *	@buf: the buffer to read to
+ *	@size: the maximum number of bytes to read
+ *	@ppos: the current position in the file
+ *
+ *  * Return:
+ *  * %0  - No bytes copied (EOF).
+ *  * %>0 - Number of bytes copied.
+ *  * %<0 - Error code.
+ */
+static ssize_t trace_format_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
+{
+	ssize_t copied = 0;
+	ssize_t ret;
+
+	do {
+		ret = seq_read(file, buf + copied, size - copied, ppos);
+		if (ret < 0)
+			return ret;
+		copied += ret;
+		if (copied >= size)
+			break;
+	} while (ret);
+
+	return copied;
+}
+
 #ifdef CONFIG_PERF_EVENTS
 static ssize_t
 event_id_read(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)
@@ -2157,7 +2186,7 @@ static const struct file_operations ftrace_enable_fops = {
 
 static const struct file_operations ftrace_event_format_fops = {
 	.open = trace_format_open,
-	.read = seq_read,
+	.read = trace_format_read,
 	.llseek = seq_lseek,
 	.release = seq_release,
 };
=============================================
>
>[...]
>>>>-- Steve
>>>
>>
>

Thanks,
Shiju