On Thu, Aug 08, 2024 at 11:13:27PM +0800, Shiyang Ruan wrote:
> CXL device can find&report memory problems, even before MCE is detected
> by CPU. AFAIK, the current kernel only traces POISON error event
> from FW-First/OS-First path, but it doesn't handle them, neither
> notify processes who are using the POISON page like MCE does.
>
> Thus, user have to read logs from trace and find out which device
> reported the error and which applications are affected. That is not
> an easy work and cannot be handled in time. Thus, it is needed to add
> the feature to make the work done automatically and quickly. Once CXL
> device reports the POISON error (via FW-First/OS-First), kernel
> handles it immediately, similar to the flow when a MCE is triggered.
>
> The current call trace of error reporting&handling looks like this:
> ```
> 1. MCE (interrupt #18, while CPU consuming POISON)
> -> do_machine_check()
> -> mce_log()
> -> notify chain (x86_mce_decoder_chain)
> -> memory_failure()
>
> 2.a FW-First (optional, CXL device proactively find&report)
> -> CXL device -> Firmware
> -> OS: ACPI->APEI->GHES->CPER -> CXL driver -> trace
> \-> memory_failure()
> ^----- ADD
> 2.b OS-First (optional, CXL device proactively find&report)
> -> CXL device -> MSI
> -> OS: CXL driver -> trace
> \-> memory_failure()
> ^------------------------------- ADD
> ```
> This patch adds calling memory_failure() while CXL device reporting
> error is received, marked as "ADD" in figure above.
>
> Signed-off-by: Shiyang Ruan <ruansy.fnst@xxxxxxxxxxx>
> ---
> drivers/cxl/core/mbox.c | 75 ++++++++++++++++++++++++++++++++-------
> drivers/cxl/cxlmem.h | 8 ++---
> drivers/cxl/pci.c | 4 +--
> include/linux/cxl-event.h | 16 ++++++++-
> 4 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index e5cdeafdf76e..0cb6ef2e6600 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -849,10 +849,55 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
> }
> EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
>
> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> - enum cxl_event_log_type type,
> - enum cxl_event_type event_type,
> - const uuid_t *uuid, union cxl_event *evt)
> +static void cxl_report_poison(struct cxl_memdev *cxlmd, u64 hpa)
> +{
> + unsigned long pfn = PHYS_PFN(hpa);
> +
> + memory_failure_queue(pfn, 0);
> +}
> +
> +static void cxl_event_handle_general_media(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + u64 hpa,
> + struct cxl_event_gen_media *rec)
> +{
> + if (type == CXL_EVENT_TYPE_FAIL) {
> + switch (rec->media_hdr.transaction_type) {
> + case CXL_EVENT_TRANSACTION_READ:
> + case CXL_EVENT_TRANSACTION_WRITE:
> + case CXL_EVENT_TRANSACTION_SCAN_MEDIA:
> + case CXL_EVENT_TRANSACTION_INJECT_POISON:
> + cxl_report_poison(cxlmd, hpa);
> + break;
> + default:
> + break;
> + }
> + }
> +}
> +
> +static void cxl_event_handle_dram(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + u64 hpa,
> + struct cxl_event_dram *rec)
> +{
> + if (type == CXL_EVENT_TYPE_FAIL) {
> + switch (rec->media_hdr.transaction_type) {
> + case CXL_EVENT_TRANSACTION_READ:
> + case CXL_EVENT_TRANSACTION_WRITE:
> + case CXL_EVENT_TRANSACTION_SCAN_MEDIA:
> + case CXL_EVENT_TRANSACTION_INJECT_POISON:
> + cxl_report_poison(cxlmd, hpa);
> + break;
> + default:
> + break;
> + }
> + }
> +}
> +
> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + enum cxl_event_type event_type,
> + const uuid_t *uuid, union cxl_event *evt)
> {
> if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
> trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
> @@ -880,18 +925,22 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> if (cxlr)
> hpa = cxl_dpa_to_hpa(cxlr, cxlmd, dpa);
>
> - if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
> + if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
> trace_cxl_general_media(cxlmd, type, cxlr, hpa,
> &evt->gen_media);
> - else if (event_type == CXL_CPER_EVENT_DRAM)
> + cxl_event_handle_general_media(cxlmd, type, hpa,
> + &evt->gen_media);
> + } else if (event_type == CXL_CPER_EVENT_DRAM) {
> trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
> + cxl_event_handle_dram(cxlmd, type, hpa, &evt->dram);
Does it make sense to call the trace function in
cxl_event_handle_dram/general_media and replace the trace function with
the handle_* here?
> + }
> }
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_event_handle_record, CXL);
>
> -static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> - enum cxl_event_log_type type,
> - struct cxl_event_record_raw *record)
> +static void __cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + struct cxl_event_record_raw *record)
> {
> enum cxl_event_type ev_type = CXL_CPER_EVENT_GENERIC;
> const uuid_t *uuid = &record->id;
> @@ -903,7 +952,7 @@ static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> else if (uuid_equal(uuid, &CXL_EVENT_MEM_MODULE_UUID))
> ev_type = CXL_CPER_EVENT_MEM_MODULE;
>
> - cxl_event_trace_record(cxlmd, type, ev_type, uuid, &record->event);
> + cxl_event_handle_record(cxlmd, type, ev_type, uuid, &record->event);
> }
>
> static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> @@ -1012,8 +1061,8 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> break;
>
> for (i = 0; i < nr_rec; i++)
> - __cxl_event_trace_record(cxlmd, type,
> - &payload->records[i]);
> + __cxl_event_handle_record(cxlmd, type,
> + &payload->records[i]);
>
> if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> trace_cxl_overflow(cxlmd, type, payload);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index afb53d058d62..5c4810dcbdeb 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -826,10 +826,10 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
> void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
> unsigned long *cmds);
> void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> - enum cxl_event_log_type type,
> - enum cxl_event_type event_type,
> - const uuid_t *uuid, union cxl_event *evt);
> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + enum cxl_event_type event_type,
> + const uuid_t *uuid, union cxl_event *evt);
> int cxl_set_timestamp(struct cxl_memdev_state *mds);
> int cxl_poison_state_init(struct cxl_memdev_state *mds);
> int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4be35dc22202..6e65ca89f666 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1029,8 +1029,8 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
> hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
> log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
>
> - cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type,
> - &uuid_null, &rec->event);
> + cxl_event_handle_record(cxlds->cxlmd, log_type, ev_type,
> + &uuid_null, &rec->event);
> }
>
> static void cxl_cper_work_fn(struct work_struct *work)
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 0bea1afbd747..be4342a2b597 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -7,6 +7,20 @@
> #include <linux/uuid.h>
> #include <linux/workqueue_types.h>
>
> +/*
> + * Event transaction type
> + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
Here and below, update the specification reference to reflect cxl 3.1.
Fan
> + */
> +enum cxl_event_transaction_type {
> + CXL_EVENT_TRANSACTION_UNKNOWN = 0X00,
> + CXL_EVENT_TRANSACTION_READ,
> + CXL_EVENT_TRANSACTION_WRITE,
> + CXL_EVENT_TRANSACTION_SCAN_MEDIA,
> + CXL_EVENT_TRANSACTION_INJECT_POISON,
> + CXL_EVENT_TRANSACTION_MEDIA_SCRUB,
> + CXL_EVENT_TRANSACTION_MEDIA_MANAGEMENT,
> +};
> +
> /*
> * Common Event Record Format
> * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
> @@ -26,7 +40,7 @@ struct cxl_event_media_hdr {
> __le64 phys_addr;
> u8 descriptor;
> u8 type;
> - u8 transaction_type;
> + u8 transaction_type; /* enum cxl_event_transaction_type */
> /*
> * The meaning of Validity Flags from bit 2 is
> * different across DRAM and General Media records
> --
> 2.34.1
>
