From: Ira Weiny <[email protected]>
Event records are defined for CXL devices. Each record is reported in
one event log. Devices are required to support the storage of at least
one event record in each event log type.
Devices track event log overflow by incrementing a counter and tracking
the time of the first and last overflow event seen.
Software queries events via the Get Event Record mailbox command; CXL
rev 3.0 section 8.2.9.2.2.
Issue the Get Event Record mailbox command on driver load. Trace each
record found, as well as any overflow conditions. Only 1 event is
requested for each query. Optimization of multiple record queries is
deferred.
This patch traces a raw event record only and leaves the specific event
record types to subsequent patches.
Macros are created to use for the common CXL Event header fields.
Cc: Steven Rostedt <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
Change from RFC:
Remove redundant error message in get event records loop
s/EVENT_RECORD_DATA_LENGTH/CXL_EVENT_RECORD_DATA_LENGTH
Use hdr_uuid for the header UUID field
Use cxl_event_log_type_str() for the trace events
Create macros for the header fields and common entries of each event
Add reserved buffer output dump
Report error if event query fails
Remove unused record_cnt variable
Steven - reorder overflow record
Remove NOTE about checkpatch
Jonathan
check for exactly 1 record
s/v3.0/rev 3.0
Use 3 byte fields for 24bit fields
Add 3.0 Maintenance Operation Class
Add Dynamic Capacity log type
Fix spelling
Dave Jiang/Dan/Alison
s/cxl-event/cxl
trace/events/cxl-events => trace/events/cxl.h
s/cxl_event_overflow/overflow
s/cxl_event/generic_event
---
MAINTAINERS | 1 +
drivers/cxl/core/mbox.c | 53 ++++++++++++++
drivers/cxl/cxlmem.h | 75 ++++++++++++++++++++
include/trace/events/cxl.h | 130 +++++++++++++++++++++++++++++++++++
include/uapi/linux/cxl_mem.h | 1 +
5 files changed, 260 insertions(+)
create mode 100644 include/trace/events/cxl.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 7547ffce5032..49bec9cc8bda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5219,6 +5219,7 @@ M: Dan Williams <[email protected]>
L: [email protected]
S: Maintained
F: drivers/cxl/
+F: include/trace/events/cxl.h
F: include/uapi/linux/cxl_mem.h
CONEXANT ACCESSRUNNER USB DRIVER
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 6c4d024ad0e8..5f258c3f2190 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -7,6 +7,9 @@
#include <cxlmem.h>
#include <cxl.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/cxl.h>
+
#include "core.h"
static bool cxl_raw_allow_all;
@@ -48,6 +51,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
CXL_CMD(RAW, CXL_VARIABLE_PAYLOAD, CXL_VARIABLE_PAYLOAD, 0),
#endif
CXL_CMD(GET_SUPPORTED_LOGS, 0, CXL_VARIABLE_PAYLOAD, CXL_CMD_FLAG_FORCE_ENABLE),
+ CXL_CMD(GET_EVENT_RECORD, 1, CXL_VARIABLE_PAYLOAD, 0),
CXL_CMD(GET_FW_INFO, 0, 0x50, 0),
CXL_CMD(GET_PARTITION_INFO, 0, 0x20, 0),
CXL_CMD(GET_LSA, 0x8, CXL_VARIABLE_PAYLOAD, 0),
@@ -707,6 +711,55 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
}
EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
+static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
+ enum cxl_event_log_type type)
+{
+ struct cxl_get_event_payload payload;
+
+ do {
+ u8 log_type = type;
+ int rc;
+
+ rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_EVENT_RECORD,
+ &log_type, sizeof(log_type),
+ &payload, sizeof(payload));
+ if (rc) {
+ dev_err(cxlds->dev, "Event log '%s': Failed to query event records : %d",
+ cxl_event_log_type_str(type), rc);
+ return;
+ }
+
+ if (le16_to_cpu(payload.record_count) == 1)
+ trace_generic_event(dev_name(cxlds->dev), type,
+ &payload.record);
+
+ if (payload.flags & CXL_GET_EVENT_FLAG_OVERFLOW)
+ trace_overflow(dev_name(cxlds->dev), type, &payload);
+
+ } while (payload.flags & CXL_GET_EVENT_FLAG_MORE_RECORDS);
+}
+
+/**
+ * cxl_mem_get_event_records - Get Event Records from the device
+ * @cxlds: The device data for the operation
+ *
+ * Retrieve all event records available on the device and report them as trace
+ * events.
+ *
+ * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
+ */
+void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
+{
+ enum cxl_event_log_type log_type;
+
+ dev_dbg(cxlds->dev, "Reading event logs\n");
+
+ for (log_type = CXL_EVENT_TYPE_INFO;
+ log_type < CXL_EVENT_TYPE_MAX; log_type++)
+ cxl_mem_get_records_log(cxlds, log_type);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
+
/**
* cxl_mem_get_partition_info - Get partition info
* @cxlds: The device data for the operation
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 88e3a8e54b6a..fa8d248fb299 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -4,6 +4,7 @@
#define __CXL_MEM_H__
#include <uapi/linux/cxl_mem.h>
#include <linux/cdev.h>
+#include <linux/uuid.h>
#include "cxl.h"
/* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */
@@ -253,6 +254,7 @@ struct cxl_dev_state {
enum cxl_opcode {
CXL_MBOX_OP_INVALID = 0x0000,
CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID,
+ CXL_MBOX_OP_GET_EVENT_RECORD = 0x0100,
CXL_MBOX_OP_GET_FW_INFO = 0x0200,
CXL_MBOX_OP_ACTIVATE_FW = 0x0202,
CXL_MBOX_OP_GET_SUPPORTED_LOGS = 0x0400,
@@ -322,6 +324,78 @@ struct cxl_mbox_identify {
u8 qos_telemetry_caps;
} __packed;
+/*
+ * Common Event Record Format
+ * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
+ */
+#define CXL_EVENT_REC_HDR_RES_LEN 0xf
+struct cxl_event_record_hdr {
+ uuid_t id;
+ u8 length;
+ u8 flags[3];
+ __le16 handle;
+ __le16 related_handle;
+ __le64 timestamp;
+ u8 maint_op_class;
+ u8 reserved[CXL_EVENT_REC_HDR_RES_LEN];
+} __packed;
+
+#define CXL_EVENT_RECORD_DATA_LENGTH 0x50
+struct cxl_event_record_raw {
+ struct cxl_event_record_hdr hdr;
+ u8 data[CXL_EVENT_RECORD_DATA_LENGTH];
+} __packed;
+
+/*
+ * Get Event Records output payload
+ * CXL rev 3.0 section 8.2.9.2.2; Table 8-50
+ *
+ * Space given for 1 record
+ */
+#define CXL_GET_EVENT_FLAG_OVERFLOW BIT(0)
+#define CXL_GET_EVENT_FLAG_MORE_RECORDS BIT(1)
+struct cxl_get_event_payload {
+ u8 flags;
+ u8 reserved1;
+ __le16 overflow_err_count;
+ __le64 first_overflow_timestamp;
+ __le64 last_overflow_timestamp;
+ __le16 record_count;
+ u8 reserved2[0xa];
+ struct cxl_event_record_raw record;
+} __packed;
+
+/*
+ * CXL rev 3.0 section 8.2.9.2.2; Table 8-49
+ */
+enum cxl_event_log_type {
+ CXL_EVENT_TYPE_INFO = 0x00,
+ CXL_EVENT_TYPE_WARN,
+ CXL_EVENT_TYPE_FAIL,
+ CXL_EVENT_TYPE_FATAL,
+ CXL_EVENT_TYPE_DYNAMIC_CAP,
+ CXL_EVENT_TYPE_MAX
+};
+
+static inline const char *cxl_event_log_type_str(enum cxl_event_log_type type)
+{
+ switch (type) {
+ case CXL_EVENT_TYPE_INFO:
+ return "Informational";
+ case CXL_EVENT_TYPE_WARN:
+ return "Warning";
+ case CXL_EVENT_TYPE_FAIL:
+ return "Failure";
+ case CXL_EVENT_TYPE_FATAL:
+ return "Fatal";
+ case CXL_EVENT_TYPE_DYNAMIC_CAP:
+ return "Dynamic Capacity";
+ default:
+ break;
+ }
+ return "<unknown>";
+}
+
struct cxl_mbox_get_partition_info {
__le64 active_volatile_cap;
__le64 active_persistent_cap;
@@ -381,6 +455,7 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
+void cxl_mem_get_event_records(struct cxl_dev_state *cxlds);
#ifdef CONFIG_CXL_SUSPEND
void cxl_mem_active_inc(void);
void cxl_mem_active_dec(void);
diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
new file mode 100644
index 000000000000..318ba5fe046e
--- /dev/null
+++ b/include/trace/events/cxl.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM cxl
+
+#if !defined(_CXL_TRACE_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _CXL_TRACE_EVENTS_H
+
+#include <asm-generic/unaligned.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(overflow,
+
+ TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
+ struct cxl_get_event_payload *payload),
+
+ TP_ARGS(dev_name, log, payload),
+
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name)
+ __field(int, log)
+ __field(u64, first)
+ __field(u64, last)
+ __field(u16, count)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name, dev_name);
+ __entry->log = log;
+ __entry->count = le16_to_cpu(payload->overflow_err_count);
+ __entry->first = le64_to_cpu(payload->first_overflow_timestamp);
+ __entry->last = le64_to_cpu(payload->last_overflow_timestamp);
+ ),
+
+ TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
+ __get_str(dev_name), cxl_event_log_type_str(__entry->log),
+ __entry->count, __entry->first, __entry->last)
+
+);
+
+/*
+ * Common Event Record Format
+ * CXL 3.0 section 8.2.9.2.1; Table 8-42
+ */
+#define CXL_EVENT_RECORD_FLAG_PERMANENT BIT(2)
+#define CXL_EVENT_RECORD_FLAG_MAINT_NEEDED BIT(3)
+#define CXL_EVENT_RECORD_FLAG_PERF_DEGRADED BIT(4)
+#define CXL_EVENT_RECORD_FLAG_HW_REPLACE BIT(5)
+#define show_hdr_flags(flags) __print_flags(flags, " | ", \
+ { CXL_EVENT_RECORD_FLAG_PERMANENT, "Permanent Condition" }, \
+ { CXL_EVENT_RECORD_FLAG_MAINT_NEEDED, "Maintenance Needed" }, \
+ { CXL_EVENT_RECORD_FLAG_PERF_DEGRADED, "Performance Degraded" }, \
+ { CXL_EVENT_RECORD_FLAG_HW_REPLACE, "Hardware Replacement Needed" } \
+)
+
+/*
+ * Define macros for the common header of each CXL event.
+ *
+ * Tracepoints using these macros must do 3 things:
+ *
+ * 1) Add CXL_EVT_TP_entry to TP_STRUCT__entry
+ * 2) Use CXL_EVT_TP_fast_assign within TP_fast_assign;
+ * pass the dev_name, log, and CXL event header
+ * 3) Use CXL_EVT_TP_printk() instead of TP_printk()
+ *
+ * See the generic_event tracepoint as an example.
+ */
+#define CXL_EVT_TP_entry \
+ __string(dev_name, dev_name) \
+ __field(int, log) \
+ __array(u8, hdr_uuid, UUID_SIZE) \
+ __field(u32, hdr_flags) \
+ __field(u16, hdr_handle) \
+ __field(u16, hdr_related_handle) \
+ __field(u64, hdr_timestamp) \
+ __array(u8, hdr_res, CXL_EVENT_REC_HDR_RES_LEN) \
+ __field(u8, hdr_length) \
+ __field(u8, hdr_maint_op_class)
+
+#define CXL_EVT_TP_fast_assign(dname, l, hdr) \
+ __assign_str(dev_name, (dname)); \
+ __entry->log = (l); \
+ memcpy(__entry->hdr_uuid, &(hdr).id, UUID_SIZE); \
+ __entry->hdr_length = (hdr).length; \
+ __entry->hdr_flags = get_unaligned_le24((hdr).flags); \
+ __entry->hdr_handle = le16_to_cpu((hdr).handle); \
+ __entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
+ __entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
+ __entry->hdr_maint_op_class = (hdr).maint_op_class; \
+ memcpy(__entry->hdr_res, &(hdr).reserved, \
+ CXL_EVENT_REC_HDR_RES_LEN)
+
+
+#define CXL_EVT_TP_printk(fmt, ...) \
+ TP_printk("%s log=%s : time=%llu uuid=%pUl len=%d flags='%s' " \
+ "handle=%x related_handle=%x maint_op_class=%u res='%s' " \
+ " : " fmt, \
+ __get_str(dev_name), cxl_event_log_type_str(__entry->log), \
+ __entry->hdr_timestamp, __entry->hdr_uuid, __entry->hdr_length, \
+ show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, \
+ __entry->hdr_related_handle, __entry->hdr_maint_op_class, \
+ __print_hex(__entry->hdr_res, CXL_EVENT_REC_HDR_RES_LEN), \
+ ##__VA_ARGS__)
+
+TRACE_EVENT(generic_event,
+
+ TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
+ struct cxl_event_record_raw *rec),
+
+ TP_ARGS(dev_name, log, rec),
+
+ TP_STRUCT__entry(
+ CXL_EVT_TP_entry
+ __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
+ ),
+
+ TP_fast_assign(
+ CXL_EVT_TP_fast_assign(dev_name, log, rec->hdr);
+ memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
+ ),
+
+ CXL_EVT_TP_printk("%s",
+ __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
+);
+
+#endif /* _CXL_TRACE_EVENTS_H */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE cxl
+#include <trace/define_trace.h>
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c71021a2a9ed..70459be5bdd4 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -24,6 +24,7 @@
___C(IDENTIFY, "Identify Command"), \
___C(RAW, "Raw device command"), \
___C(GET_SUPPORTED_LOGS, "Get Supported Logs"), \
+ ___C(GET_EVENT_RECORD, "Get Event Record"), \
___C(GET_FW_INFO, "Get FW Info"), \
___C(GET_PARTITION_INFO, "Get Partition Information"), \
___C(GET_LSA, "Get Label Storage Area"), \
--
2.37.2
On Mon, 10 Oct 2022 15:41:22 -0700
[email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> Event records are defined for CXL devices. Each record is reported in
> one event log. Devices are required to support the storage of at least
> one event record in each event log type.
>
> Devices track event log overflow by incrementing a counter and tracking
> the time of the first and last overflow event seen.
>
> Software queries events via the Get Event Record mailbox command; CXL
> rev 3.0 section 8.2.9.2.2.
>
> Issue the Get Event Record mailbox command on driver load. Trace each
> record found, as well as any overflow conditions. Only 1 event is
> requested for each query. Optimization of multiple record queries is
> deferred.
>
> This patch traces a raw event record only and leaves the specific event
> record types to subsequent patches.
>
> Macros are created to use for the common CXL Event header fields.
>
> Cc: Steven Rostedt <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
Hi Ira,
This looks basically fine, but I'm not convinced that it is a good
or sustainable idea to report reserved fields from the underlying
events in the trace events.
If they are defined to have meaning later, we can't remove them from
the 'reserved' field with out breaking backwards compatibility.
So we end up with a weird mixture of some fields in both reserved and
new entries in the TP and some not.
I've cc'd Mauro as he's probably more experienced in how to handle this
sort of stuff than anyone else I know.
Jonathan
>
> ---
> Change from RFC:
> Remove redundant error message in get event records loop
> s/EVENT_RECORD_DATA_LENGTH/CXL_EVENT_RECORD_DATA_LENGTH
> Use hdr_uuid for the header UUID field
> Use cxl_event_log_type_str() for the trace events
> Create macros for the header fields and common entries of each event
> Add reserved buffer output dump
> Report error if event query fails
> Remove unused record_cnt variable
> Steven - reorder overflow record
> Remove NOTE about checkpatch
> Jonathan
> check for exactly 1 record
> s/v3.0/rev 3.0
> Use 3 byte fields for 24bit fields
> Add 3.0 Maintenance Operation Class
> Add Dynamic Capacity log type
> Fix spelling
> Dave Jiang/Dan/Alison
> s/cxl-event/cxl
> trace/events/cxl-events => trace/events/cxl.h
> s/cxl_event_overflow/overflow
> s/cxl_event/generic_event
> ---
...
> +/**
> + * cxl_mem_get_event_records - Get Event Records from the device
> + * @cxlds: The device data for the operation
> + *
> + * Retrieve all event records available on the device and report them as trace
> + * events.
> + *
> + * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
> + */
> +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
> +{
> + enum cxl_event_log_type log_type;
> +
> + dev_dbg(cxlds->dev, "Reading event logs\n");
> +
> + for (log_type = CXL_EVENT_TYPE_INFO;
I'd start at 0. To my mind that's clearer than this which
basically says there is a contiguous range that may or may
not be 0 based.
> + log_type < CXL_EVENT_TYPE_MAX; log_type++)
> + cxl_mem_get_records_log(cxlds, log_type);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
> +
> /**
> * cxl_mem_get_partition_info - Get partition info
> * @cxlds: The device data for the operation
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..fa8d248fb299 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -4,6 +4,7 @@
> #define __CXL_MEM_H__
> #include <uapi/linux/cxl_mem.h>
> #include <linux/cdev.h>
> +#include <linux/uuid.h>
> #include "cxl.h"
>
> /* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */
> @@ -253,6 +254,7 @@ struct cxl_dev_state {
> enum cxl_opcode {
> CXL_MBOX_OP_INVALID = 0x0000,
> CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID,
> + CXL_MBOX_OP_GET_EVENT_RECORD = 0x0100,
> CXL_MBOX_OP_GET_FW_INFO = 0x0200,
> CXL_MBOX_OP_ACTIVATE_FW = 0x0202,
> CXL_MBOX_OP_GET_SUPPORTED_LOGS = 0x0400,
> @@ -322,6 +324,78 @@ struct cxl_mbox_identify {
> u8 qos_telemetry_caps;
> } __packed;
>
> +/*
> + * Common Event Record Format
> + * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
> + */
> +#define CXL_EVENT_REC_HDR_RES_LEN 0xf
> +struct cxl_event_record_hdr {
> + uuid_t id;
> + u8 length;
> + u8 flags[3];
> + __le16 handle;
> + __le16 related_handle;
> + __le64 timestamp;
> + u8 maint_op_class;
> + u8 reserved[CXL_EVENT_REC_HDR_RES_LEN];
> +} __packed;
> +
> +#define CXL_EVENT_RECORD_DATA_LENGTH 0x50
> +struct cxl_event_record_raw {
I'd like some comments here on 'why' this makes sense.
Is expectation that it's here for future CXL spec definitions
or is it intended to allow some reporting of vendor defined
records? Actually maybe a comment down at the TP would make more
sense than here. Either way comment somewhere :)
> + struct cxl_event_record_hdr hdr;
> + u8 data[CXL_EVENT_RECORD_DATA_LENGTH];
> +} __packed;
> +
> +static inline const char *cxl_event_log_type_str(enum cxl_event_log_type type)
> +{
> + switch (type) {
> + case CXL_EVENT_TYPE_INFO:
> + return "Informational";
> + case CXL_EVENT_TYPE_WARN:
> + return "Warning";
> + case CXL_EVENT_TYPE_FAIL:
> + return "Failure";
> + case CXL_EVENT_TYPE_FATAL:
> + return "Fatal";
> + case CXL_EVENT_TYPE_DYNAMIC_CAP:
> + return "Dynamic Capacity";
Array of const char * that you pick from (with limit check) maybe rather than a switch?
Doesn't matter that much though.
> + default:
> + break;
> + }
> + return "<unknown>";
> +}
> +
> struct cxl_mbox_get_partition_info {
> __le64 active_volatile_cap;
> __le64 active_persistent_cap;
> @@ -381,6 +455,7 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
> struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
> void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds);
> #ifdef CONFIG_CXL_SUSPEND
> void cxl_mem_active_inc(void);
> void cxl_mem_active_dec(void);
> diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> new file mode 100644
> index 000000000000..318ba5fe046e
> --- /dev/null
> +++ b/include/trace/events/cxl.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM cxl
> +
> +#if !defined(_CXL_TRACE_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _CXL_TRACE_EVENTS_H
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(overflow,
> +
> + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> + struct cxl_get_event_payload *payload),
> +
> + TP_ARGS(dev_name, log, payload),
> +
> + TP_STRUCT__entry(
> + __string(dev_name, dev_name)
> + __field(int, log)
> + __field(u64, first)
first_ts maybe? first alone is a bit vague.
> + __field(u64, last)
> + __field(u16, count)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dev_name, dev_name);
> + __entry->log = log;
> + __entry->count = le16_to_cpu(payload->overflow_err_count);
> + __entry->first = le64_to_cpu(payload->first_overflow_timestamp);
> + __entry->last = le64_to_cpu(payload->last_overflow_timestamp);
> + ),
> +
> + TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
> + __get_str(dev_name), cxl_event_log_type_str(__entry->log),
So we have a potential header include ordering issue given this header doesn't
include the definition of cxl_event_log_type_str()?
> + __entry->count, __entry->first, __entry->last)
> +
> +);
> +
> +/*
> + * Common Event Record Format
> + * CXL 3.0 section 8.2.9.2.1; Table 8-42
> + */
> +#define CXL_EVENT_RECORD_FLAG_PERMANENT BIT(2)
> +#define CXL_EVENT_RECORD_FLAG_MAINT_NEEDED BIT(3)
> +#define CXL_EVENT_RECORD_FLAG_PERF_DEGRADED BIT(4)
> +#define CXL_EVENT_RECORD_FLAG_HW_REPLACE BIT(5)
> +#define show_hdr_flags(flags) __print_flags(flags, " | ", \
> + { CXL_EVENT_RECORD_FLAG_PERMANENT, "Permanent Condition" }, \
> + { CXL_EVENT_RECORD_FLAG_MAINT_NEEDED, "Maintenance Needed" }, \
> + { CXL_EVENT_RECORD_FLAG_PERF_DEGRADED, "Performance Degraded" }, \
> + { CXL_EVENT_RECORD_FLAG_HW_REPLACE, "Hardware Replacement Needed" } \
> +)
> +
> +/*
> + * Define macros for the common header of each CXL event.
> + *
> + * Tracepoints using these macros must do 3 things:
> + *
> + * 1) Add CXL_EVT_TP_entry to TP_STRUCT__entry
> + * 2) Use CXL_EVT_TP_fast_assign within TP_fast_assign;
> + * pass the dev_name, log, and CXL event header
> + * 3) Use CXL_EVT_TP_printk() instead of TP_printk()
> + *
> + * See the generic_event tracepoint as an example.
> + */
> +#define CXL_EVT_TP_entry \
> + __string(dev_name, dev_name) \
> + __field(int, log) \
> + __array(u8, hdr_uuid, UUID_SIZE) \
> + __field(u32, hdr_flags) \
> + __field(u16, hdr_handle) \
> + __field(u16, hdr_related_handle) \
> + __field(u64, hdr_timestamp) \
> + __array(u8, hdr_res, CXL_EVENT_REC_HDR_RES_LEN) \
> + __field(u8, hdr_length) \
> + __field(u8, hdr_maint_op_class)
> +
> +#define CXL_EVT_TP_fast_assign(dname, l, hdr) \
> + __assign_str(dev_name, (dname)); \
> + __entry->log = (l); \
> + memcpy(__entry->hdr_uuid, &(hdr).id, UUID_SIZE); \
> + __entry->hdr_length = (hdr).length; \
> + __entry->hdr_flags = get_unaligned_le24((hdr).flags); \
> + __entry->hdr_handle = le16_to_cpu((hdr).handle); \
> + __entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
> + __entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
> + __entry->hdr_maint_op_class = (hdr).maint_op_class; \
> + memcpy(__entry->hdr_res, &(hdr).reserved, \
> + CXL_EVENT_REC_HDR_RES_LEN)
What's the logic behind having the reserved fields here?
How does that change when a future spec defines them as used? Do we have
to keep whatever was in the previously reserved fields for ever in order to
maintain backwards compatibility even though we've added the same data to the end
of the trace point?
I don't think they should be here at all. Given a userspace stack has to cope with
out knowing the contents of those fields as userspace might not be up to date,
I see no problem with requiring a newer kernel to support stuff added in future
specs.
Steven, Mauro, you probably have a better idea of history of similar cases. How
have other people handled reserved fields in underlying hardware reports that
may become used in later revisions?
Also, probably makes sense to cc linux-edac on v3 of this series as the experts
in these flows tend to read that list. Obviously intent so far is not to pass
these into the edac subsystem, but I'd still let them know this work is going on.
> +
> +
> +#define CXL_EVT_TP_printk(fmt, ...) \
> + TP_printk("%s log=%s : time=%llu uuid=%pUl len=%d flags='%s' " \
> + "handle=%x related_handle=%x maint_op_class=%u res='%s' " \
> + " : " fmt, \
> + __get_str(dev_name), cxl_event_log_type_str(__entry->log), \
> + __entry->hdr_timestamp, __entry->hdr_uuid, __entry->hdr_length, \
> + show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle, \
> + __entry->hdr_related_handle, __entry->hdr_maint_op_class, \
> + __print_hex(__entry->hdr_res, CXL_EVENT_REC_HDR_RES_LEN), \
> + ##__VA_ARGS__)
> +
> +TRACE_EVENT(generic_event,
> +
> + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> + struct cxl_event_record_raw *rec),
> +
> + TP_ARGS(dev_name, log, rec),
> +
> + TP_STRUCT__entry(
> + CXL_EVT_TP_entry
> + __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
> + ),
> +
> + TP_fast_assign(
> + CXL_EVT_TP_fast_assign(dev_name, log, rec->hdr);
> + memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
> + ),
> +
> + CXL_EVT_TP_printk("%s",
> + __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
> +);
> +
> +#endif /* _CXL_TRACE_EVENTS_H */
On Tue, Oct 11, 2022 at 01:39:20PM +0100, Jonathan Cameron wrote:
> On Mon, 10 Oct 2022 15:41:22 -0700
> [email protected] wrote:
>
> > From: Ira Weiny <[email protected]>
> >
> > Event records are defined for CXL devices. Each record is reported in
> > one event log. Devices are required to support the storage of at least
> > one event record in each event log type.
> >
> > Devices track event log overflow by incrementing a counter and tracking
> > the time of the first and last overflow event seen.
> >
> > Software queries events via the Get Event Record mailbox command; CXL
> > rev 3.0 section 8.2.9.2.2.
> >
> > Issue the Get Event Record mailbox command on driver load. Trace each
> > record found, as well as any overflow conditions. Only 1 event is
> > requested for each query. Optimization of multiple record queries is
> > deferred.
> >
> > This patch traces a raw event record only and leaves the specific event
> > record types to subsequent patches.
> >
> > Macros are created to use for the common CXL Event header fields.
> >
> > Cc: Steven Rostedt <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
>
> Hi Ira,
>
> This looks basically fine, but I'm not convinced that it is a good
> or sustainable idea to report reserved fields from the underlying
> events in the trace events.
> If they are defined to have meaning later, we can't remove them from
> the 'reserved' field with out breaking backwards compatibility.
> So we end up with a weird mixture of some fields in both reserved and
> new entries in the TP and some not.
I removed the reserved fields from the data but I forgot the header. I think I
had a reason they would be useful in the header but I forget now. Which means
I was probably wrong. ;-)
>
> I've cc'd Mauro as he's probably more experienced in how to handle this
> sort of stuff than anyone else I know.
>
Fair enough but I agree the reserved fields should be removed.
>
> Jonathan
>
> >
> > ---
> > Change from RFC:
> > Remove redundant error message in get event records loop
> > s/EVENT_RECORD_DATA_LENGTH/CXL_EVENT_RECORD_DATA_LENGTH
> > Use hdr_uuid for the header UUID field
> > Use cxl_event_log_type_str() for the trace events
> > Create macros for the header fields and common entries of each event
> > Add reserved buffer output dump
> > Report error if event query fails
> > Remove unused record_cnt variable
> > Steven - reorder overflow record
> > Remove NOTE about checkpatch
> > Jonathan
> > check for exactly 1 record
> > s/v3.0/rev 3.0
> > Use 3 byte fields for 24bit fields
> > Add 3.0 Maintenance Operation Class
> > Add Dynamic Capacity log type
> > Fix spelling
> > Dave Jiang/Dan/Alison
> > s/cxl-event/cxl
> > trace/events/cxl-events => trace/events/cxl.h
> > s/cxl_event_overflow/overflow
> > s/cxl_event/generic_event
> > ---
>
> ...
>
> > +/**
> > + * cxl_mem_get_event_records - Get Event Records from the device
> > + * @cxlds: The device data for the operation
> > + *
> > + * Retrieve all event records available on the device and report them as trace
> > + * events.
> > + *
> > + * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
> > + */
> > +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
> > +{
> > + enum cxl_event_log_type log_type;
> > +
> > + dev_dbg(cxlds->dev, "Reading event logs\n");
> > +
> > + for (log_type = CXL_EVENT_TYPE_INFO;
>
> I'd start at 0. To my mind that's clearer than this which
> basically says there is a contiguous range that may or may
> not be 0 based.
Ok. But weather or not it is 0 based does not matter.
I'll change it. It takes out a line of code! ;-)
>
> > + log_type < CXL_EVENT_TYPE_MAX; log_type++)
> > + cxl_mem_get_records_log(cxlds, log_type);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
> > +
> > /**
> > * cxl_mem_get_partition_info - Get partition info
> > * @cxlds: The device data for the operation
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 88e3a8e54b6a..fa8d248fb299 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -4,6 +4,7 @@
> > #define __CXL_MEM_H__
> > #include <uapi/linux/cxl_mem.h>
> > #include <linux/cdev.h>
> > +#include <linux/uuid.h>
> > #include "cxl.h"
> >
> > /* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */
> > @@ -253,6 +254,7 @@ struct cxl_dev_state {
> > enum cxl_opcode {
> > CXL_MBOX_OP_INVALID = 0x0000,
> > CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID,
> > + CXL_MBOX_OP_GET_EVENT_RECORD = 0x0100,
> > CXL_MBOX_OP_GET_FW_INFO = 0x0200,
> > CXL_MBOX_OP_ACTIVATE_FW = 0x0202,
> > CXL_MBOX_OP_GET_SUPPORTED_LOGS = 0x0400,
> > @@ -322,6 +324,78 @@ struct cxl_mbox_identify {
> > u8 qos_telemetry_caps;
> > } __packed;
> >
> > +/*
> > + * Common Event Record Format
> > + * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
> > + */
> > +#define CXL_EVENT_REC_HDR_RES_LEN 0xf
> > +struct cxl_event_record_hdr {
> > + uuid_t id;
> > + u8 length;
> > + u8 flags[3];
> > + __le16 handle;
> > + __le16 related_handle;
> > + __le64 timestamp;
> > + u8 maint_op_class;
> > + u8 reserved[CXL_EVENT_REC_HDR_RES_LEN];
> > +} __packed;
> > +
> > +#define CXL_EVENT_RECORD_DATA_LENGTH 0x50
> > +struct cxl_event_record_raw {
>
> I'd like some comments here on 'why' this makes sense.
> Is expectation that it's here for future CXL spec definitions
> or is it intended to allow some reporting of vendor defined
> records? Actually maybe a comment down at the TP would make more
> sense than here. Either way comment somewhere :)
This comes from a discussion I had with Dan a while back where we decided that
any unknown record would just be dumped to user space as data. The idea was
based on the kernel ignoring vendor specified events but does allow for some
future CXL spec definitios to get through.
This may cause similar confusion to the reserved fields but I don't think it is
quite the same.
I'll add a comment.
>
> > + struct cxl_event_record_hdr hdr;
> > + u8 data[CXL_EVENT_RECORD_DATA_LENGTH];
> > +} __packed;
> > +
>
>
> > +static inline const char *cxl_event_log_type_str(enum cxl_event_log_type type)
> > +{
> > + switch (type) {
> > + case CXL_EVENT_TYPE_INFO:
> > + return "Informational";
> > + case CXL_EVENT_TYPE_WARN:
> > + return "Warning";
> > + case CXL_EVENT_TYPE_FAIL:
> > + return "Failure";
> > + case CXL_EVENT_TYPE_FATAL:
> > + return "Fatal";
> > + case CXL_EVENT_TYPE_DYNAMIC_CAP:
> > + return "Dynamic Capacity";
> Array of const char * that you pick from (with limit check) maybe rather than a switch?
> Doesn't matter that much though.
I've seen it both ways. This forces users to use the helper. And I don't
think there is much concern about the speed of the lookup.
>
> > + default:
> > + break;
> > + }
> > + return "<unknown>";
> > +}
> > +
> > struct cxl_mbox_get_partition_info {
> > __le64 active_volatile_cap;
> > __le64 active_persistent_cap;
> > @@ -381,6 +455,7 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
> > struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
> > void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> > void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds);
> > +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds);
> > #ifdef CONFIG_CXL_SUSPEND
> > void cxl_mem_active_inc(void);
> > void cxl_mem_active_dec(void);
> > diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> > new file mode 100644
> > index 000000000000..318ba5fe046e
> > --- /dev/null
> > +++ b/include/trace/events/cxl.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM cxl
> > +
> > +#if !defined(_CXL_TRACE_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _CXL_TRACE_EVENTS_H
> > +
> > +#include <asm-generic/unaligned.h>
> > +#include <linux/tracepoint.h>
> > +
> > +TRACE_EVENT(overflow,
> > +
> > + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> > + struct cxl_get_event_payload *payload),
> > +
> > + TP_ARGS(dev_name, log, payload),
> > +
> > + TP_STRUCT__entry(
> > + __string(dev_name, dev_name)
> > + __field(int, log)
> > + __field(u64, first)
>
> first_ts maybe? first alone is a bit vague.
Agreed. Changed to first_ts and last_ts.
>
> > + __field(u64, last)
> > + __field(u16, count)
> > + ),
> > +
> > + TP_fast_assign(
> > + __assign_str(dev_name, dev_name);
> > + __entry->log = log;
> > + __entry->count = le16_to_cpu(payload->overflow_err_count);
> > + __entry->first = le64_to_cpu(payload->first_overflow_timestamp);
> > + __entry->last = le64_to_cpu(payload->last_overflow_timestamp);
> > + ),
> > +
> > + TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
> > + __get_str(dev_name), cxl_event_log_type_str(__entry->log),
>
> So we have a potential header include ordering issue given this header doesn't
> include the definition of cxl_event_log_type_str()?
The RFC had a __print_symbolic() conversion which was redundant. I really
don't like having 2 conversion routines for this string. I think it will
result in maintenance issues down the line.
I'll try and resolve it. Nothing I have tried this morning seems to work. I
may just go back to what I had. I need to double check the user space code
too.
>
> > + __entry->count, __entry->first, __entry->last)
> > +
> > +);
> > +
> > +/*
> > + * Common Event Record Format
> > + * CXL 3.0 section 8.2.9.2.1; Table 8-42
> > + */
> > +#define CXL_EVENT_RECORD_FLAG_PERMANENT BIT(2)
> > +#define CXL_EVENT_RECORD_FLAG_MAINT_NEEDED BIT(3)
> > +#define CXL_EVENT_RECORD_FLAG_PERF_DEGRADED BIT(4)
> > +#define CXL_EVENT_RECORD_FLAG_HW_REPLACE BIT(5)
> > +#define show_hdr_flags(flags) __print_flags(flags, " | ", \
> > + { CXL_EVENT_RECORD_FLAG_PERMANENT, "Permanent Condition" }, \
> > + { CXL_EVENT_RECORD_FLAG_MAINT_NEEDED, "Maintenance Needed" }, \
> > + { CXL_EVENT_RECORD_FLAG_PERF_DEGRADED, "Performance Degraded" }, \
> > + { CXL_EVENT_RECORD_FLAG_HW_REPLACE, "Hardware Replacement Needed" } \
> > +)
> > +
> > +/*
> > + * Define macros for the common header of each CXL event.
> > + *
> > + * Tracepoints using these macros must do 3 things:
> > + *
> > + * 1) Add CXL_EVT_TP_entry to TP_STRUCT__entry
> > + * 2) Use CXL_EVT_TP_fast_assign within TP_fast_assign;
> > + * pass the dev_name, log, and CXL event header
> > + * 3) Use CXL_EVT_TP_printk() instead of TP_printk()
> > + *
> > + * See the generic_event tracepoint as an example.
> > + */
> > +#define CXL_EVT_TP_entry \
> > + __string(dev_name, dev_name) \
> > + __field(int, log) \
> > + __array(u8, hdr_uuid, UUID_SIZE) \
> > + __field(u32, hdr_flags) \
> > + __field(u16, hdr_handle) \
> > + __field(u16, hdr_related_handle) \
> > + __field(u64, hdr_timestamp) \
> > + __array(u8, hdr_res, CXL_EVENT_REC_HDR_RES_LEN) \
> > + __field(u8, hdr_length) \
> > + __field(u8, hdr_maint_op_class)
> > +
> > +#define CXL_EVT_TP_fast_assign(dname, l, hdr) \
> > + __assign_str(dev_name, (dname)); \
> > + __entry->log = (l); \
> > + memcpy(__entry->hdr_uuid, &(hdr).id, UUID_SIZE); \
> > + __entry->hdr_length = (hdr).length; \
> > + __entry->hdr_flags = get_unaligned_le24((hdr).flags); \
> > + __entry->hdr_handle = le16_to_cpu((hdr).handle); \
> > + __entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
> > + __entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
> > + __entry->hdr_maint_op_class = (hdr).maint_op_class; \
> > + memcpy(__entry->hdr_res, &(hdr).reserved, \
> > + CXL_EVENT_REC_HDR_RES_LEN)
>
> What's the logic behind having the reserved fields here?
I forget.
> How does that change when a future spec defines them as used? Do we have
> to keep whatever was in the previously reserved fields for ever in order to
> maintain backwards compatibility even though we've added the same data to the end
> of the trace point?
I don't expect the header to change as much as the records themselves. So I'm
ok removing the reserved fields.
>
> I don't think they should be here at all. Given a userspace stack has to cope with
> out knowing the contents of those fields as userspace might not be up to date,
> I see no problem with requiring a newer kernel to support stuff added in future
> specs.
>
> Steven, Mauro, you probably have a better idea of history of similar cases. How
> have other people handled reserved fields in underlying hardware reports that
> may become used in later revisions?
>
> Also, probably makes sense to cc linux-edac on v3 of this series as the experts
> in these flows tend to read that list. Obviously intent so far is not to pass
> these into the edac subsystem, but I'd still let them know this work is going on.
>
Sounds good.
The struggle I'm having is in how flexible these records are compared to the
other RAS trace events.
I'll drop the reserved fields.
Ira
On Mon, 10 Oct 2022 15:41:22 -0700
[email protected] wrote:
> new file mode 100644
> index 000000000000..318ba5fe046e
> --- /dev/null
> +++ b/include/trace/events/cxl.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM cxl
> +
> +#if !defined(_CXL_TRACE_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _CXL_TRACE_EVENTS_H
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(overflow,
Please do not use generic names for grouped events. As most tooling can
use the name and not associate it with the group, it makes it ambiguous
for what event it wants to enable.
That is, call it "cxl_overflow" and not "overflow".
> +
> + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> + struct cxl_get_event_payload *payload),
> +
> + TP_ARGS(dev_name, log, payload),
> +
> + TP_STRUCT__entry(
> + __string(dev_name, dev_name)
> + __field(int, log)
> + __field(u64, first)
> + __field(u64, last)
> + __field(u16, count)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dev_name, dev_name);
> + __entry->log = log;
> + __entry->count = le16_to_cpu(payload->overflow_err_count);
> + __entry->first = le64_to_cpu(payload->first_overflow_timestamp);
> + __entry->last = le64_to_cpu(payload->last_overflow_timestamp);
> + ),
> +
> + TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
> + __get_str(dev_name), cxl_event_log_type_str(__entry->log),
> + __entry->count, __entry->first, __entry->last)
> +
> +);
> +
> +TRACE_EVENT(generic_event,
Same here. It's a "cxl_generic_event" not a "generic_event" that will
clash with any other subsystem that would (but shouldn't) use the same
name.
-- Steve
> +
> + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> + struct cxl_event_record_raw *rec),
> +
> + TP_ARGS(dev_name, log, rec),
> +
> + TP_STRUCT__entry(
> + CXL_EVT_TP_entry
> + __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
> + ),
> +
> + TP_fast_assign(
> + CXL_EVT_TP_fast_assign(dev_name, log, rec->hdr);
> + memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
> + ),
> +
> + CXL_EVT_TP_printk("%s",
> + __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
> +);
> +
> +#endif /* _CXL_TRACE_EVENTS_H */
>
On Sat, Oct 15, 2022 at 07:28:40AM -0400, Steven Rostedt wrote:
> On Mon, 10 Oct 2022 15:41:22 -0700
> [email protected] wrote:
>
> > new file mode 100644
> > index 000000000000..318ba5fe046e
> > --- /dev/null
> > +++ b/include/trace/events/cxl.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM cxl
> > +
> > +#if !defined(_CXL_TRACE_EVENTS_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _CXL_TRACE_EVENTS_H
> > +
> > +#include <asm-generic/unaligned.h>
> > +#include <linux/tracepoint.h>
> > +
> > +TRACE_EVENT(overflow,
>
> Please do not use generic names for grouped events. As most tooling can
> use the name and not associate it with the group, it makes it ambiguous
> for what event it wants to enable.
>
> That is, call it "cxl_overflow" and not "overflow".
I did not realize that.[1]
[1] https://lore.kernel.org/linux-cxl/[email protected]/
>
> > +
> > + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> > + struct cxl_get_event_payload *payload),
> > +
> > + TP_ARGS(dev_name, log, payload),
> > +
> > + TP_STRUCT__entry(
> > + __string(dev_name, dev_name)
> > + __field(int, log)
> > + __field(u64, first)
> > + __field(u64, last)
> > + __field(u16, count)
> > + ),
> > +
> > + TP_fast_assign(
> > + __assign_str(dev_name, dev_name);
> > + __entry->log = log;
> > + __entry->count = le16_to_cpu(payload->overflow_err_count);
> > + __entry->first = le64_to_cpu(payload->first_overflow_timestamp);
> > + __entry->last = le64_to_cpu(payload->last_overflow_timestamp);
> > + ),
> > +
> > + TP_printk("%s: EVENT LOG OVERFLOW log=%s : %u records from %llu to %llu",
> > + __get_str(dev_name), cxl_event_log_type_str(__entry->log),
> > + __entry->count, __entry->first, __entry->last)
> > +
> > +);
> > +
>
> > +TRACE_EVENT(generic_event,
>
> Same here. It's a "cxl_generic_event" not a "generic_event" that will
> clash with any other subsystem that would (but shouldn't) use the same
> name.
Sure I'll change it back.
Thanks for setting us straight,
Ira
>
> -- Steve
>
> > +
> > + TP_PROTO(const char *dev_name, enum cxl_event_log_type log,
> > + struct cxl_event_record_raw *rec),
> > +
> > + TP_ARGS(dev_name, log, rec),
> > +
> > + TP_STRUCT__entry(
> > + CXL_EVT_TP_entry
> > + __array(u8, data, CXL_EVENT_RECORD_DATA_LENGTH)
> > + ),
> > +
> > + TP_fast_assign(
> > + CXL_EVT_TP_fast_assign(dev_name, log, rec->hdr);
> > + memcpy(__entry->data, &rec->data, CXL_EVENT_RECORD_DATA_LENGTH);
> > + ),
> > +
> > + CXL_EVT_TP_printk("%s",
> > + __print_hex(__entry->data, CXL_EVENT_RECORD_DATA_LENGTH))
> > +);
> > +
> > +#endif /* _CXL_TRACE_EVENTS_H */
> >
Hi Ira,
On 10/10/22 5:41 PM, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
>
>
> +static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> + enum cxl_event_log_type type)
> +{
> + struct cxl_get_event_payload payload;
> +
> + do {
> + u8 log_type = type;
> + int rc;
> +
> + rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_EVENT_RECORD,
> + &log_type, sizeof(log_type),
> + &payload, sizeof(payload));
> + if (rc) {
> + dev_err(cxlds->dev, "Event log '%s': Failed to query event records : %d",
> + cxl_event_log_type_str(type), rc);
> + return;
> + }
> +
> + if (le16_to_cpu(payload.record_count) == 1)
> + trace_generic_event(dev_name(cxlds->dev), type,
> + &payload.record);
> +
> + if (payload.flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> + trace_overflow(dev_name(cxlds->dev), type, &payload);
> +
> + } while (payload.flags & CXL_GET_EVENT_FLAG_MORE_RECORDS);
> +}
> +
> +/**
> + * cxl_mem_get_event_records - Get Event Records from the device
> + * @cxlds: The device data for the operation
> + *
> + * Retrieve all event records available on the device and report them as trace
> + * events.
> + *
> + * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
> + */
> +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
> +{
> + enum cxl_event_log_type log_type;
> +
> + dev_dbg(cxlds->dev, "Reading event logs\n");
> +
> + for (log_type = CXL_EVENT_TYPE_INFO;
> + log_type < CXL_EVENT_TYPE_MAX; log_type++)
Why should we loop through each event log here? What if the event
record doesn't exist in the event log?
I got some Mailbox error messages like this while bootup..
[ 346.387010] cxl_pci 0000:7f:00.0: Sending command
[ 346.387181] cxl_pci 0000:7f:00.0: Doorbell wait took 0ms
[ 346.387197] cxl_pci 0000:7f:00.0: Mailbox operation had an error: cmd
input was invalid
[ 346.387205] cxl_pci 0000:7f:00.0: Event log 'Warning': Failed to
query event records : -6
..
Can we just read the "Event Status" field from Event Status Register
(Device Status Registers Capability Offset + 00h) 8.2.8.3.1 in CXL Spec,
determine if the records exist and just query those event logs?
Thanks,
Smita
> + cxl_mem_get_records_log(cxlds, log_type);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
> +
> /**
> * cxl_mem_get_partition_info - Get partition info
> * @cxlds: The device data for the operation
On Thu, Oct 20, 2022 at 04:50:30PM -0500, Smita Koralahalli wrote:
> Hi Ira,
>
> On 10/10/22 5:41 PM, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> >
[snip]
> > +
> > +/**
> > + * cxl_mem_get_event_records - Get Event Records from the device
> > + * @cxlds: The device data for the operation
> > + *
> > + * Retrieve all event records available on the device and report them as trace
> > + * events.
> > + *
> > + * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
> > + */
> > +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
> > +{
> > + enum cxl_event_log_type log_type;
> > +
> > + dev_dbg(cxlds->dev, "Reading event logs\n");
> > +
> > + for (log_type = CXL_EVENT_TYPE_INFO;
> > + log_type < CXL_EVENT_TYPE_MAX; log_type++)
>
> Why should we loop through each event log here?
The idea was to clear all the event logs. I think this made more sense before
the addition of the optional dynamic capacity log.
>
> What if the event
> record doesn't exist in the event log?
An empty log does not cause any issue. The query will simply return 0 records
which is valid.
>
> I got some Mailbox error messages like this while bootup..
> [? 346.387010] cxl_pci 0000:7f:00.0: Sending command
> [? 346.387181] cxl_pci 0000:7f:00.0: Doorbell wait took 0ms
> [? 346.387197] cxl_pci 0000:7f:00.0: Mailbox operation had an error: cmd
> input was invalid
> [? 346.387205] cxl_pci 0000:7f:00.0: Event log 'Warning': Failed to query
> event records : -6
> ..
>
> Can we just read the "Event Status" field from Event Status Register
> (Device Status Registers Capability Offset + 00h) 8.2.8.3.1 in CXL Spec,
> determine if the records exist and just query those event logs?
Likely the hardware does not have the dynamic capacity log and so the code is
asking for something invalid. I did not think of that when I added that new
log. Checking status register looks to be the proper solution.
I'll throw in some testing in QEMU for this. I'll also have to implement the
status register in QEMU to fully test.
Thanks for the testing! :-D
Ira
>
> Thanks,
> Smita
>
> > + cxl_mem_get_records_log(cxlds, log_type);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
> > +
> > /**
> > * cxl_mem_get_partition_info - Get partition info
> > * @cxlds: The device data for the operation