| Message ID | 20250123084421.127697-5-Smita.KoralahalliChannabasappa@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi/ghes, cper, cxl: Process CXL CPER Protocol errors | expand |
On Thu, Jan 23, 2025 at 08:44:19AM +0000, Smita Koralahalli wrote: > Add support in GHES to detect and process CXL CPER Protocol errors, as > defined in UEFI v2.10, section N.2.13. > > Define struct cxl_cper_prot_err_work_data to cache CXL protocol error > information, including RAS capabilities and severity, for further > handling. > > These cached CXL CPER records will later be processed by workqueues > within the CXL subsystem. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/acpi/apei/ghes.c | 54 ++++++++++++++++++++++++++++++++++++++++ > include/cxl/event.h | 6 +++++ > 2 files changed, 60 insertions(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index b72772494655..4d725d988c43 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -674,6 +674,56 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata, > schedule_work(&entry->work); > } > > +static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err, > + int severity) > +{ > +#ifdef CONFIG_ACPI_APEI_PCIEAER #ifdef in ".c" code is ugly. But I don't see a less ugly way to deal with this. Moving this elsewhere and adding an empty stub function for when CONFIG_ACPI_APEI_PCIEAER isn't configured doesn't make things any better. > + struct cxl_cper_prot_err_work_data wd; > + u8 *dvsec_start, *cap_start; > + > + if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) { > + pr_err_ratelimited("CXL CPER invalid agent type\n"); > + return; > + } > + > + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) { > + pr_err_ratelimited("CXL CPER invalid protocol error log\n"); > + return; > + } > + > + if (prot_err->err_len != sizeof(struct cxl_ras_capability_regs)) { > + pr_err_ratelimited("CXL CPER invalid RAS Cap size (%u)\n", > + prot_err->err_len); > + return; > + } > + > + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER)) > + pr_warn(FW_WARN "CXL CPER no device serial number\n"); Should this be rate limited too? It's not like this device will somehow acquire a serial number if there is another error. > + > + switch (prot_err->agent_type) { > + case RCD: > + case DEVICE: > + case LD: > + case FMLD: > + case RP: > + case DSP: > + case USP: > + memcpy(&wd.prot_err, prot_err, sizeof(wd.prot_err)); > + > + dvsec_start = (u8 *)(prot_err + 1); > + cap_start = dvsec_start + prot_err->dvsec_len; > + > + memcpy(&wd.ras_cap, cap_start, sizeof(wd.ras_cap)); > + wd.severity = cper_severity_to_aer(severity); > + break; > + default: > + pr_err_ratelimited("CXL CPER invalid agent type: %d\n", > + prot_err->agent_type); > + return; > + } > +#endif > +} > + > /* Room for 8 entries for each of the 4 event log queues */ > #define CXL_CPER_FIFO_DEPTH 32 > DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, CXL_CPER_FIFO_DEPTH); > @@ -777,6 +827,10 @@ static bool ghes_do_proc(struct ghes *ghes, > } > else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { > queued = ghes_handle_arm_hw_error(gdata, sev, sync); > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) { > + struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata); > + > + cxl_cper_post_prot_err(prot_err, gdata->error_severity); > } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata); > > diff --git a/include/cxl/event.h b/include/cxl/event.h > index 66d85fc87701..ee1c3dec62fa 100644 > --- a/include/cxl/event.h > +++ b/include/cxl/event.h > @@ -232,6 +232,12 @@ struct cxl_ras_capability_regs { > u32 header_log[16]; > }; > > +struct cxl_cper_prot_err_work_data { > + struct cxl_cper_sec_prot_err prot_err; > + struct cxl_ras_capability_regs ras_cap; > + int severity; > +}; > + > #ifdef CONFIG_ACPI_APEI_GHES > int cxl_cper_register_work(struct work_struct *work); > int cxl_cper_unregister_work(struct work_struct *work); > -- > 2.17.1 > With added ratelimit mentioned above: Reviewed-by: Tony Luck <tony.luck@intel.com> -Tony
On Mon, Feb 03, 2025 at 10:59:14AM -0800, Luck, Tony wrote: > On Thu, Jan 23, 2025 at 08:44:19AM +0000, Smita Koralahalli wrote: > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > > index b72772494655..4d725d988c43 100644 > > --- a/drivers/acpi/apei/ghes.c > > +++ b/drivers/acpi/apei/ghes.c > > @@ -674,6 +674,56 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata, > > schedule_work(&entry->work); > > } > > > > +static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err, > > + int severity) > > +{ > > +#ifdef CONFIG_ACPI_APEI_PCIEAER > > #ifdef in ".c" code is ugly. But I don't see a less ugly way to deal > with this. Moving this elsewhere and adding an empty stub function > for when CONFIG_ACPI_APEI_PCIEAER isn't configured doesn't make things > any better. > The generally accepted ways I've seen for static funcs in .c is: #ifdef CONFIG_ACPI_APEI_PCIEAER static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) { ... } #else static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) { } #endif This at least makes the function that does stuff easier to read and gives you a spot to throw out a "Config not enabled" error if its beneficial. More of a style nit than anything else. But either way, Reviewed-by: Gregory Price <gourry@gourry.net>
Smita Koralahalli wrote: > Add support in GHES to detect and process CXL CPER Protocol errors, as > defined in UEFI v2.10, section N.2.13. > > Define struct cxl_cper_prot_err_work_data to cache CXL protocol error > information, including RAS capabilities and severity, for further > handling. > > These cached CXL CPER records will later be processed by workqueues > within the CXL subsystem. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/acpi/apei/ghes.c | 54 ++++++++++++++++++++++++++++++++++++++++ > include/cxl/event.h | 6 +++++ > 2 files changed, 60 insertions(+) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index b72772494655..4d725d988c43 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -674,6 +674,56 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata, > schedule_work(&entry->work); > } > > +static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err, > + int severity) > +{ > +#ifdef CONFIG_ACPI_APEI_PCIEAER This is ok for now, but now that there are two of these functions I would not say "no" to a follow on patch that moves ghes_handle_aer() and cxl_cper_post_prot_err() to a new: drivers/acpi/apei/aer.c ...that gets conditionally compiled instead of this minor coding-style nit. Certainly if a third instance like this arrives, a new aer.c becomes warranted. > + struct cxl_cper_prot_err_work_data wd; > + u8 *dvsec_start, *cap_start; > + > + if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) { > + pr_err_ratelimited("CXL CPER invalid agent type\n"); > + return; > + } I notice that the component event prints are not ratelimited, so a follow-on patch to add rate limiting to cxl_cper_post_event() would be welcome to match what you have here.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index b72772494655..4d725d988c43 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -674,6 +674,56 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata, schedule_work(&entry->work); } +static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err, + int severity) +{ +#ifdef CONFIG_ACPI_APEI_PCIEAER + struct cxl_cper_prot_err_work_data wd; + u8 *dvsec_start, *cap_start; + + if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) { + pr_err_ratelimited("CXL CPER invalid agent type\n"); + return; + } + + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) { + pr_err_ratelimited("CXL CPER invalid protocol error log\n"); + return; + } + + if (prot_err->err_len != sizeof(struct cxl_ras_capability_regs)) { + pr_err_ratelimited("CXL CPER invalid RAS Cap size (%u)\n", + prot_err->err_len); + return; + } + + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER)) + pr_warn(FW_WARN "CXL CPER no device serial number\n"); + + switch (prot_err->agent_type) { + case RCD: + case DEVICE: + case LD: + case FMLD: + case RP: + case DSP: + case USP: + memcpy(&wd.prot_err, prot_err, sizeof(wd.prot_err)); + + dvsec_start = (u8 *)(prot_err + 1); + cap_start = dvsec_start + prot_err->dvsec_len; + + memcpy(&wd.ras_cap, cap_start, sizeof(wd.ras_cap)); + wd.severity = cper_severity_to_aer(severity); + break; + default: + pr_err_ratelimited("CXL CPER invalid agent type: %d\n", + prot_err->agent_type); + return; + } +#endif +} + /* Room for 8 entries for each of the 4 event log queues */ #define CXL_CPER_FIFO_DEPTH 32 DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, CXL_CPER_FIFO_DEPTH); @@ -777,6 +827,10 @@ static bool ghes_do_proc(struct ghes *ghes, } else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { queued = ghes_handle_arm_hw_error(gdata, sev, sync); + } else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) { + struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata); + + cxl_cper_post_prot_err(prot_err, gdata->error_severity); } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata); diff --git a/include/cxl/event.h b/include/cxl/event.h index 66d85fc87701..ee1c3dec62fa 100644 --- a/include/cxl/event.h +++ b/include/cxl/event.h @@ -232,6 +232,12 @@ struct cxl_ras_capability_regs { u32 header_log[16]; }; +struct cxl_cper_prot_err_work_data { + struct cxl_cper_sec_prot_err prot_err; + struct cxl_ras_capability_regs ras_cap; + int severity; +}; + #ifdef CONFIG_ACPI_APEI_GHES int cxl_cper_register_work(struct work_struct *work); int cxl_cper_unregister_work(struct work_struct *work);