Re: [RFC PATCH 3/4] cxl/events: Updates for CXL DRAM Event Record

Jonathan Cameron
2024-10-17T12:38:43Z

On Wed, 16 Oct 2024 17:33:48 +0100
<shiju.jose@huawei.com> wrote:

> From: Shiju Jose <shiju.jose@huawei.com>
>
> CXL spec 3.1 section 8.2.9.2.1.2 Table 8-46, DRAM Event Record has updated
> with following new fields and new types for Memory Event Type, Transaction
> Type and Validity Flags fields.
> 1. Component Identifier
> 2. Sub-channel
> 3. Advanced Programmable Corrected Memory Error Threshold Event Flags
> 4. Corrected Memory Error Count at Event
> 5. Memory Event Sub-Type
>
> Add updates for the above spec changes in the CXL events record and CXL
> DRAM trace event implementations.
>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
Passing comments on two things inline.
1) There are a couple of whitespace consistency changes in here.
Spaces to tabs for alignment. That's fine but maybe needs a brief
mention in the patch description.
2) Really odd that the spec didn't have a component ID field for DRAM
errors. They weren't all that useful before the PLDM format was added
but still a curiosity that made me open up the 3.0 spec. Indeed, no
such field.

With that one line added to the patch description this looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> drivers/cxl/core/trace.h | 44 ++++++++++++++++++++++++++++++++--------
> include/cxl/event.h | 7 ++++++-
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index e638e82429bc..20790dffa2b4 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -468,7 +468,7 @@ TRACE_EVENT(cxl_general_media,
> /*
> * DRAM Event Record - DER
> *
> - * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
> + * CXL rev 3.1 section 8.2.9.2.1.2; Table 8-46
> */
> /*
> * DRAM Event Record defines many fields the same as the General Media Event
> @@ -478,11 +478,17 @@ TRACE_EVENT(cxl_general_media,
> #define CXL_DER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR 0x01
> #define CXL_DER_MEM_EVT_TYPE_INV_ADDR 0x02
> #define CXL_DER_MEM_EVT_TYPE_DATA_PATH_ERROR 0x03
> -#define show_dram_mem_event_type(type) __print_symbolic(type, \
> +#define CXL_DER_MEM_EVT_TYPE_TE_STATE_VIOLATION 0x04
> +#define CXL_DER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE 0x05
> +#define CXL_DER_MEM_EVT_TYPE_CKID_VIOLATION 0x06
> +#define show_dram_mem_event_type(type) __print_symbolic(type, \

This change looks odd here but does print the line above into the
same formatting style as the other similar cases in the file.
Maybe worth a line in the patch description to say "Includes trivial consistency of white
space improvements" just to flag up that it was intentional.

> { CXL_DER_MEM_EVT_TYPE_ECC_ERROR, "ECC Error" }, \
> { CXL_DER_MEM_EVT_TYPE_SCRUB_MEDIA_ECC_ERROR, "Scrub Media ECC Error" }, \
> { CXL_DER_MEM_EVT_TYPE_INV_ADDR, "Invalid Address" }, \
> - { CXL_DER_MEM_EVT_TYPE_DATA_PATH_ERROR, "Data Path Error" } \
> + { CXL_DER_MEM_EVT_TYPE_DATA_PATH_ERROR, "Data Path Error" }, \
> + { CXL_DER_MEM_EVT_TYPE_TE_STATE_VIOLATION, "TE State Violation" }, \
> + { CXL_DER_MEM_EVT_TYPE_AP_CME_COUNTER_EXPIRE, "Adv Prog CME Counter Expiration" }, \
> + { CXL_DER_MEM_EVT_TYPE_CKID_VIOLATION, "CKID Violation" } \
> )
>
> #define CXL_DER_VALID_CHANNEL BIT(0)
> @@ -493,7 +499,10 @@ TRACE_EVENT(cxl_general_media,
> #define CXL_DER_VALID_ROW BIT(5)
> #define CXL_DER_VALID_COLUMN BIT(6)
> #define CXL_DER_VALID_CORRECTION_MASK BIT(7)
> -#define show_dram_valid_flags(flags) __print_flags(flags, "|", \
> +#define CXL_DER_VALID_COMPONENT BIT(8)
> +#define CXL_DER_VALID_COMPONENT_ID_FORMAT BIT(9)
> +#define CXL_DER_VALID_SUB_CHANNEL BIT(10)
> +#define show_dram_valid_flags(flags) __print_flags(flags, "|", \

As above this is a minor white space consistency change.

> { CXL_DER_VALID_CHANNEL, "CHANNEL" }, \
> { CXL_DER_VALID_RANK, "RANK" }, \
> { CXL_DER_VALID_NIBBLE, "NIBBLE" }, \
> @@ -501,7 +510,9 @@ TRACE_EVENT(cxl_general_media,
> { CXL_DER_VALID_BANK, "BANK" }, \
> { CXL_DER_VALID_ROW, "ROW" }, \
> { CXL_DER_VALID_COLUMN, "COLUMN" }, \
> - { CXL_DER_VALID_CORRECTION_MASK, "CORRECTION MASK" } \
> + { CXL_DER_VALID_CORRECTION_MASK, "CORRECTION MASK" }, \
> + { CXL_DER_VALID_COMPONENT, "COMPONENT" }, \
> + { CXL_DER_VALID_SUB_CHANNEL, "SUB CHANNEL" } \
> )

> diff --git a/include/cxl/event.h b/include/cxl/event.h
> index ea8cd44a52e9..7e98492c85df 100644
> --- a/include/cxl/event.h
> +++ b/include/cxl/event.h
> @@ -71,7 +71,12 @@ struct cxl_event_dram {
> u8 row[3];
> u8 column[2];
> u8 correction_mask[CXL_EVENT_DER_CORRECTION_MASK_SIZE];
> - u8 reserved[0x17];
> + u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
Odd that the general media had this field in 3.0 but DRAM didn't.
I checked though and indeed the case!

> + u8 sub_channel;
> + u8 cme_threshold_ev_flags;
> + u8 cvme_count[3];
> + u8 sub_type;
> + u8 reserved;
> } __packed;
>
> /*