| Message ID | 20240808151328.707869-2-ruansy.fnst@fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: add device reporting poison handler | expand |
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@fujitsu.com> > --- > 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 >
在 2024/8/9 2:28, Fan Ni 写道: > 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@fujitsu.com> >> --- >> 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? Sorry for late reply. I'm not really good at naming functions. Since the trace functions already have the framework to deal with each kind of uuids and event types, I don't think we should make another one for the same logics. Thus, I reused it and renamed the functions. Maybe "handle" isn't a good word to describe "tracing records and doing memory_failure if necessary". Could you help me to name it better? > >> + } >> } >> } >> -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. Ok. Will update it. -- Thanks, Ruan. > > 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 >>
On Thu, 8 Aug 2024 23:13:27 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> 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. These are async reports, so I'm not sure what 'in time' really means here. If we get synchronous poison from a processor access it will be handled via traditional means (MCE, ARM SEA etc) Whether to handle async error reports (typically from scrub or because the memory device received poison from someone else) the same way should perhaps be a policy decision. It should match what we do for firmware first async reports though (any policy controls make sense for both). An example of this would be that an host OS might attempt a polite close of an application might attempt a polite if we know there is poison somewhere in a dataset it has access to. If that poison is never seen synchronously (because that data is not read) then it my close successfully rather than being killed. If it's injected poison and we didn't see it synchronously we might well not want to kill anything. > 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. Typo in patch title. handling I've also dropped qemu-devel as this doesn't have anything to do with qemu. > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> Experienced RAS folk in the CC, how do you want this to work for asynchoronous memory errors on CXL devices?
在 2024/8/27 23:46, Jonathan Cameron 写道: > On Thu, 8 Aug 2024 23:13:27 +0800 > Shiyang Ruan <ruansy.fnst@fujitsu.com> 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. > > These are async reports, so I'm not sure what 'in time' really means here. 'in time' may not be appropriate. I think 'ASAP' is better. I just want to say: comparing with users finding out the errors from trace logs and notifying apps manually, kernel handler can do that automatically and ASAP. > If we get synchronous poison from a processor access it will be handled > via traditional means (MCE, ARM SEA etc) Yes. For FW-First path, MCE mechanism can cover this. But for OS-First path, errors can only be traced, then logged by userspace tool like rasdaemon. We hope in OS-First path, kernel can handle it like MCE does too. > > Whether to handle async error reports (typically from scrub or because > the memory device received poison from someone else) the same way > should perhaps be a policy decision. It should match what we do > for firmware first async reports though (any policy controls make sense > for both). Yes. In OS-First path, I think it should always be turned on. > > An example of this would be that an host OS might attempt a polite close > of an application might attempt a polite if we know there is poison > somewhere in a dataset it has access to. If that poison is never seen > synchronously (because that data is not read) then it my close > successfully rather than being killed. According to kernel docs for 'early kill' of memory-failure, I think it's suitable for this case. > > If it's injected poison and we didn't see it synchronously we might > well not want to kill anything. Agree. Injection APIs are used for debugging, not a really HW poison. > >> 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. > > Typo in patch title. handling Thanks. > I've also dropped qemu-devel as this doesn't have anything to do with qemu. > OK. >> >> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > Experienced RAS folk in the CC, how do you want this to work for > asynchoronous memory errors on CXL devices? > >
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); + } } } -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 + */ +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
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@fujitsu.com> --- 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(-)