| Message ID | 20241001005234.61409-5-Smita.KoralahalliChannabasappa@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi/ghes, cper, cxl: Trace FW-First CXL Protocol Errors | expand |
Smita Koralahalli wrote: > When PCIe AER is in FW-First, OS should process CXL Protocol errors from > CPER records. > > Reuse the existing work queue cxl_cper_work registered with GHES to notify > the CXL subsystem on a Protocol error. > > The defined trace events cxl_aer_uncorrectable_error and > cxl_aer_correctable_error currently trace native CXL AER errors. Reuse > them to trace FW-First Protocol Errors. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > --- > v2: > Removed pr_warn for serial number. > p_err -> rec/p_rec. > --- > drivers/acpi/apei/ghes.c | 14 ++++++++++++++ > drivers/cxl/core/pci.c | 24 ++++++++++++++++++++++++ > drivers/cxl/cxlpci.h | 3 +++ > drivers/cxl/pci.c | 20 ++++++++++++++++++-- > include/cxl/event.h | 1 + > 5 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 9dcf0f78458f..5082885e1f2c 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata) > > if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec)) > return; > + > + guard(spinlock_irqsave)(&cxl_cper_work_lock); > + > + if (!cxl_cper_work) > + return; > + > + wd.event_type = CXL_CPER_EVENT_PROT_ERR; > + > + if (!kfifo_put(&cxl_cper_fifo, wd)) { > + pr_err_ratelimited("CXL CPER kfifo overflow\n"); > + return; > + } > + > + schedule_work(cxl_cper_work); > } > > int cxl_cper_register_work(struct work_struct *work) > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 5b46bc46aaa9..39ef24c8991f 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -650,6 +650,30 @@ void read_cdat_data(struct cxl_port *port) > } > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); > > +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, > + struct cxl_cper_prot_err *rec) > +{ > + u32 status, fe; > + > + if (rec->severity == CXL_AER_CORRECTABLE) { > + status = rec->cxl_ras.cor_status & ~rec->cxl_ras.cor_mask; > + > + trace_cxl_aer_correctable_error(cxlds->cxlmd, status); > + } else { > + status = rec->cxl_ras.uncor_status & ~rec->cxl_ras.uncor_mask; > + > + if (hweight32(status) > 1) > + fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK, > + rec->cxl_ras.cap_control)); > + else > + fe = status; > + > + trace_cxl_aer_uncorrectable_error(cxlds->cxlmd, status, fe, > + rec->cxl_ras.header_log); > + } > +} > +EXPORT_SYMBOL_NS_GPL(cxl_trace_prot_err, CXL); > + > static void __cxl_handle_cor_ras(struct cxl_dev_state *cxlds, > void __iomem *ras_base) > { > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > index 4da07727ab9c..8acd8f2c39c9 100644 > --- a/drivers/cxl/cxlpci.h > +++ b/drivers/cxl/cxlpci.h > @@ -129,4 +129,7 @@ void read_cdat_data(struct cxl_port *port); > void cxl_cor_error_detected(struct pci_dev *pdev); > pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, > pci_channel_state_t state); > +struct cxl_cper_prot_err; > +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, > + struct cxl_cper_prot_err *rec); > #endif /* __CXL_PCI_H__ */ > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 915102f5113f..0a29321921a0 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -1064,12 +1064,28 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type, > &uuid_null, &rec->event); > } > > +static void cxl_handle_prot_err(struct cxl_cper_prot_err *rec) > +{ > + struct cxl_dev_state *cxlds; > + > + cxlds = get_cxl_devstate(rec->segment, rec->bus, > + rec->device, rec->function); As mentioned in 2/4 I don't think this is going to work correctly. > + if (!cxlds) > + return; > + > + cxl_trace_prot_err(cxlds, rec); > +} > + > static void cxl_cper_work_fn(struct work_struct *work) > { > struct cxl_cper_work_data wd; > > - while (cxl_cper_kfifo_get(&wd)) > - cxl_handle_cper_event(wd.event_type, &wd.rec); > + while (cxl_cper_kfifo_get(&wd)) { > + if (wd.event_type == CXL_CPER_EVENT_PROT_ERR) > + cxl_handle_prot_err(&wd.p_rec); > + else > + cxl_handle_cper_event(wd.event_type, &wd.rec); Why does this need to change? Have cxl_handle_cper_event() decode the BDF as it does now and call cxl_handle_prot_err() there? I think you could drop patch 2/4 entirely and not have to duplicate the logic. Ira > + } > } > static DECLARE_WORK(cxl_cper_work, cxl_cper_work_fn); > > diff --git a/include/cxl/event.h b/include/cxl/event.h > index 5b316150556a..d854d8c435db 100644 > --- a/include/cxl/event.h > +++ b/include/cxl/event.h > @@ -115,6 +115,7 @@ struct cxl_event_record_raw { > } __packed; > > enum cxl_event_type { > + CXL_CPER_EVENT_PROT_ERR, > CXL_CPER_EVENT_GENERIC, > CXL_CPER_EVENT_GEN_MEDIA, > CXL_CPER_EVENT_DRAM, > -- > 2.17.1 >
Smita Koralahalli wrote: > When PCIe AER is in FW-First, OS should process CXL Protocol errors from > CPER records. > > Reuse the existing work queue cxl_cper_work registered with GHES to notify > the CXL subsystem on a Protocol error. > > The defined trace events cxl_aer_uncorrectable_error and > cxl_aer_correctable_error currently trace native CXL AER errors. Reuse > them to trace FW-First Protocol Errors. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > --- > v2: > Removed pr_warn for serial number. > p_err -> rec/p_rec. > --- > drivers/acpi/apei/ghes.c | 14 ++++++++++++++ > drivers/cxl/core/pci.c | 24 ++++++++++++++++++++++++ > drivers/cxl/cxlpci.h | 3 +++ > drivers/cxl/pci.c | 20 ++++++++++++++++++-- > include/cxl/event.h | 1 + > 5 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 9dcf0f78458f..5082885e1f2c 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata) > > if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec)) > return; > + > + guard(spinlock_irqsave)(&cxl_cper_work_lock); > + > + if (!cxl_cper_work) > + return; > + > + wd.event_type = CXL_CPER_EVENT_PROT_ERR; > + > + if (!kfifo_put(&cxl_cper_fifo, wd)) { > + pr_err_ratelimited("CXL CPER kfifo overflow\n"); > + return; > + } > + > + schedule_work(cxl_cper_work); The cxl_cper_work item is only for cases where the cxl_pci driver might care about annotating an error report with driver specific details like the impacted kernel object name, 'struct cxl_memdev', or address translation for DPA data. Protocol errors that are not endpoint errors should never be placed in the cxl_cper_fifo. That is exclusively for errors that cxl_pci needs to consume. My expectation is that similar to aer_recover_queue for PCIe protocol errors CXL needs to grow a cxl_recover_queue that at a minimum triggers new trace events to dump these records to RAS daemon. I am struggling to think what useful information cxl_pci could ever append to a protocol error event. What is more likely is that later when Terry adds port error handling a CPER protocol error record could trigger a new cxl_do_recovery() to react to CXL topology errors that might impact downstream CXL devices. In that case the notification will come through something like a new 'struct cxl_error_handlers *' hanging off 'struct pci_driver' since accelerator drivers are going to have distinct error handling from generic memory expanders.
On 10/1/2024 8:52 AM, Ira Weiny wrote: > Smita Koralahalli wrote: >> When PCIe AER is in FW-First, OS should process CXL Protocol errors from >> CPER records. >> >> Reuse the existing work queue cxl_cper_work registered with GHES to notify >> the CXL subsystem on a Protocol error. >> >> The defined trace events cxl_aer_uncorrectable_error and >> cxl_aer_correctable_error currently trace native CXL AER errors. Reuse >> them to trace FW-First Protocol Errors. >> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> >> --- >> v2: >> Removed pr_warn for serial number. >> p_err -> rec/p_rec. >> --- >> drivers/acpi/apei/ghes.c | 14 ++++++++++++++ >> drivers/cxl/core/pci.c | 24 ++++++++++++++++++++++++ >> drivers/cxl/cxlpci.h | 3 +++ >> drivers/cxl/pci.c | 20 ++++++++++++++++++-- >> include/cxl/event.h | 1 + >> 5 files changed, 60 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 9dcf0f78458f..5082885e1f2c 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata) >> >> if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec)) >> return; >> + >> + guard(spinlock_irqsave)(&cxl_cper_work_lock); >> + >> + if (!cxl_cper_work) >> + return; >> + >> + wd.event_type = CXL_CPER_EVENT_PROT_ERR; >> + >> + if (!kfifo_put(&cxl_cper_fifo, wd)) { >> + pr_err_ratelimited("CXL CPER kfifo overflow\n"); >> + return; >> + } >> + >> + schedule_work(cxl_cper_work); >> } >> >> int cxl_cper_register_work(struct work_struct *work) >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index 5b46bc46aaa9..39ef24c8991f 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -650,6 +650,30 @@ void read_cdat_data(struct cxl_port *port) >> } >> EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); >> >> +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, >> + struct cxl_cper_prot_err *rec) >> +{ >> + u32 status, fe; >> + >> + if (rec->severity == CXL_AER_CORRECTABLE) { >> + status = rec->cxl_ras.cor_status & ~rec->cxl_ras.cor_mask; >> + >> + trace_cxl_aer_correctable_error(cxlds->cxlmd, status); >> + } else { >> + status = rec->cxl_ras.uncor_status & ~rec->cxl_ras.uncor_mask; >> + >> + if (hweight32(status) > 1) >> + fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK, >> + rec->cxl_ras.cap_control)); >> + else >> + fe = status; >> + >> + trace_cxl_aer_uncorrectable_error(cxlds->cxlmd, status, fe, >> + rec->cxl_ras.header_log); >> + } >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_trace_prot_err, CXL); >> + >> static void __cxl_handle_cor_ras(struct cxl_dev_state *cxlds, >> void __iomem *ras_base) >> { >> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h >> index 4da07727ab9c..8acd8f2c39c9 100644 >> --- a/drivers/cxl/cxlpci.h >> +++ b/drivers/cxl/cxlpci.h >> @@ -129,4 +129,7 @@ void read_cdat_data(struct cxl_port *port); >> void cxl_cor_error_detected(struct pci_dev *pdev); >> pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, >> pci_channel_state_t state); >> +struct cxl_cper_prot_err; >> +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, >> + struct cxl_cper_prot_err *rec); >> #endif /* __CXL_PCI_H__ */ >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >> index 915102f5113f..0a29321921a0 100644 >> --- a/drivers/cxl/pci.c >> +++ b/drivers/cxl/pci.c >> @@ -1064,12 +1064,28 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type, >> &uuid_null, &rec->event); >> } >> >> +static void cxl_handle_prot_err(struct cxl_cper_prot_err *rec) >> +{ >> + struct cxl_dev_state *cxlds; >> + >> + cxlds = get_cxl_devstate(rec->segment, rec->bus, >> + rec->device, rec->function); > > As mentioned in 2/4 I don't think this is going to work correctly. > >> + if (!cxlds) >> + return; >> + >> + cxl_trace_prot_err(cxlds, rec); >> +} >> + >> static void cxl_cper_work_fn(struct work_struct *work) >> { >> struct cxl_cper_work_data wd; >> >> - while (cxl_cper_kfifo_get(&wd)) >> - cxl_handle_cper_event(wd.event_type, &wd.rec); >> + while (cxl_cper_kfifo_get(&wd)) { >> + if (wd.event_type == CXL_CPER_EVENT_PROT_ERR) >> + cxl_handle_prot_err(&wd.p_rec); >> + else >> + cxl_handle_cper_event(wd.event_type, &wd.rec); > > Why does this need to change? > > Have cxl_handle_cper_event() decode the BDF as it does now and call > cxl_handle_prot_err() there? I think you could drop patch 2/4 entirely > and not have to duplicate the logic. > > Ira Yeah works for me. My intention was to not disturb the cxl_handle_cper_event() as that was focussed on handling device errors. Thanks Smita > >> + } >> } >> static DECLARE_WORK(cxl_cper_work, cxl_cper_work_fn); >> >> diff --git a/include/cxl/event.h b/include/cxl/event.h >> index 5b316150556a..d854d8c435db 100644 >> --- a/include/cxl/event.h >> +++ b/include/cxl/event.h >> @@ -115,6 +115,7 @@ struct cxl_event_record_raw { >> } __packed; >> >> enum cxl_event_type { >> + CXL_CPER_EVENT_PROT_ERR, >> CXL_CPER_EVENT_GENERIC, >> CXL_CPER_EVENT_GEN_MEDIA, >> CXL_CPER_EVENT_DRAM, >> -- >> 2.17.1 >> > >
On 10/2/2024 5:16 PM, Dan Williams wrote: > Smita Koralahalli wrote: >> When PCIe AER is in FW-First, OS should process CXL Protocol errors from >> CPER records. >> >> Reuse the existing work queue cxl_cper_work registered with GHES to notify >> the CXL subsystem on a Protocol error. >> >> The defined trace events cxl_aer_uncorrectable_error and >> cxl_aer_correctable_error currently trace native CXL AER errors. Reuse >> them to trace FW-First Protocol Errors. >> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> >> --- >> v2: >> Removed pr_warn for serial number. >> p_err -> rec/p_rec. >> --- >> drivers/acpi/apei/ghes.c | 14 ++++++++++++++ >> drivers/cxl/core/pci.c | 24 ++++++++++++++++++++++++ >> drivers/cxl/cxlpci.h | 3 +++ >> drivers/cxl/pci.c | 20 ++++++++++++++++++-- >> include/cxl/event.h | 1 + >> 5 files changed, 60 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 9dcf0f78458f..5082885e1f2c 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata) >> >> if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec)) >> return; >> + >> + guard(spinlock_irqsave)(&cxl_cper_work_lock); >> + >> + if (!cxl_cper_work) >> + return; >> + >> + wd.event_type = CXL_CPER_EVENT_PROT_ERR; >> + >> + if (!kfifo_put(&cxl_cper_fifo, wd)) { >> + pr_err_ratelimited("CXL CPER kfifo overflow\n"); >> + return; >> + } >> + >> + schedule_work(cxl_cper_work); > > The cxl_cper_work item is only for cases where the cxl_pci driver might care > about annotating an error report with driver specific details like the > impacted kernel object name, 'struct cxl_memdev', or address translation > for DPA data. > > Protocol errors that are not endpoint errors should never be placed in > the cxl_cper_fifo. That is exclusively for errors that cxl_pci needs to > consume. > > My expectation is that similar to aer_recover_queue for PCIe protocol > errors CXL needs to grow a cxl_recover_queue that at a minimum triggers > new trace events to dump these records to RAS daemon. > > I am struggling to think what useful information cxl_pci could ever > append to a protocol error event. > > What is more likely is that later when Terry adds port error handling a > CPER protocol error record could trigger a new cxl_do_recovery() to > react to CXL topology errors that might impact downstream CXL devices. > In that case the notification will come through something like a new > 'struct cxl_error_handlers *' hanging off 'struct pci_driver' since > accelerator drivers are going to have distinct error handling from > generic memory expanders. Hmm, I agree handling device and protocol errors separately. Unless Terry has something to add here, I can start working on a minimal version of cxl_recover_queue as of now. Thanks Smita >
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 9dcf0f78458f..5082885e1f2c 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata) if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec)) return; + + guard(spinlock_irqsave)(&cxl_cper_work_lock); + + if (!cxl_cper_work) + return; + + wd.event_type = CXL_CPER_EVENT_PROT_ERR; + + if (!kfifo_put(&cxl_cper_fifo, wd)) { + pr_err_ratelimited("CXL CPER kfifo overflow\n"); + return; + } + + schedule_work(cxl_cper_work); } int cxl_cper_register_work(struct work_struct *work) diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 5b46bc46aaa9..39ef24c8991f 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -650,6 +650,30 @@ void read_cdat_data(struct cxl_port *port) } EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, + struct cxl_cper_prot_err *rec) +{ + u32 status, fe; + + if (rec->severity == CXL_AER_CORRECTABLE) { + status = rec->cxl_ras.cor_status & ~rec->cxl_ras.cor_mask; + + trace_cxl_aer_correctable_error(cxlds->cxlmd, status); + } else { + status = rec->cxl_ras.uncor_status & ~rec->cxl_ras.uncor_mask; + + if (hweight32(status) > 1) + fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK, + rec->cxl_ras.cap_control)); + else + fe = status; + + trace_cxl_aer_uncorrectable_error(cxlds->cxlmd, status, fe, + rec->cxl_ras.header_log); + } +} +EXPORT_SYMBOL_NS_GPL(cxl_trace_prot_err, CXL); + static void __cxl_handle_cor_ras(struct cxl_dev_state *cxlds, void __iomem *ras_base) { diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index 4da07727ab9c..8acd8f2c39c9 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -129,4 +129,7 @@ void read_cdat_data(struct cxl_port *port); void cxl_cor_error_detected(struct pci_dev *pdev); pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, pci_channel_state_t state); +struct cxl_cper_prot_err; +void cxl_trace_prot_err(struct cxl_dev_state *cxlds, + struct cxl_cper_prot_err *rec); #endif /* __CXL_PCI_H__ */ diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 915102f5113f..0a29321921a0 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -1064,12 +1064,28 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type, &uuid_null, &rec->event); } +static void cxl_handle_prot_err(struct cxl_cper_prot_err *rec) +{ + struct cxl_dev_state *cxlds; + + cxlds = get_cxl_devstate(rec->segment, rec->bus, + rec->device, rec->function); + if (!cxlds) + return; + + cxl_trace_prot_err(cxlds, rec); +} + static void cxl_cper_work_fn(struct work_struct *work) { struct cxl_cper_work_data wd; - while (cxl_cper_kfifo_get(&wd)) - cxl_handle_cper_event(wd.event_type, &wd.rec); + while (cxl_cper_kfifo_get(&wd)) { + if (wd.event_type == CXL_CPER_EVENT_PROT_ERR) + cxl_handle_prot_err(&wd.p_rec); + else + cxl_handle_cper_event(wd.event_type, &wd.rec); + } } static DECLARE_WORK(cxl_cper_work, cxl_cper_work_fn); diff --git a/include/cxl/event.h b/include/cxl/event.h index 5b316150556a..d854d8c435db 100644 --- a/include/cxl/event.h +++ b/include/cxl/event.h @@ -115,6 +115,7 @@ struct cxl_event_record_raw { } __packed; enum cxl_event_type { + CXL_CPER_EVENT_PROT_ERR, CXL_CPER_EVENT_GENERIC, CXL_CPER_EVENT_GEN_MEDIA, CXL_CPER_EVENT_DRAM,
When PCIe AER is in FW-First, OS should process CXL Protocol errors from CPER records. Reuse the existing work queue cxl_cper_work registered with GHES to notify the CXL subsystem on a Protocol error. The defined trace events cxl_aer_uncorrectable_error and cxl_aer_correctable_error currently trace native CXL AER errors. Reuse them to trace FW-First Protocol Errors. Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> --- v2: Removed pr_warn for serial number. p_err -> rec/p_rec. --- drivers/acpi/apei/ghes.c | 14 ++++++++++++++ drivers/cxl/core/pci.c | 24 ++++++++++++++++++++++++ drivers/cxl/cxlpci.h | 3 +++ drivers/cxl/pci.c | 20 ++++++++++++++++++-- include/cxl/event.h | 1 + 5 files changed, 60 insertions(+), 2 deletions(-)