| Message ID | 3853853f820a666253ca8ed6c7c724dc3d50044a.1720679234.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix issues with ARM Processor CPER records | expand |
On Thu, Jul 11, 2024 at 08:28:52AM +0200, Mauro Carvalho Chehab wrote: > In addition to those data, it also exports two fields that are > parsed by the GHES driver when firmware reports it, e. g.: > > - error severity > - cpu logical index s/cpu/CPU/g check your whole set pls. > Report all of these information to userspace via trace uAPI, So that > userspace can properly record the error and take decisions related > to cpu core isolation according to error severity and other info. > > After this patch, all the data from ARM Processor record from table Avoid having "This patch" or "This commit" in the commit message. It is tautologically useless. Also, do $ git grep 'This patch' Documentation/process for more details. ... > [mchehab: modified patch description, solve merge conflicts and fix coding style] > Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event") > Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> > Signed-off-by: Jason Tian <jason@os.amperecomputing.com> > Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com> What is this SOB chain trying to tell me? All those folks handled the patch? > Tested-by: Shiju Jose <shiju.jose@huawei.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section > --- > drivers/acpi/apei/ghes.c | 11 ++++----- > drivers/ras/ras.c | 45 +++++++++++++++++++++++++++++++++++-- > include/linux/ras.h | 16 +++++++++++--- > include/ras/ras_event.h | 48 +++++++++++++++++++++++++++++++++++----- > 4 files changed, 103 insertions(+), 17 deletions(-) ... > -void log_arm_hw_error(struct cper_sec_proc_arm *err) > +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) > { > - trace_arm_event(err); > + struct cper_arm_err_info *err_info; > + struct cper_arm_ctx_info *ctx_info; > + u8 *ven_err_data; > + u32 ctx_len = 0; > + int n, sz, cpu; > + s32 vsei_len; > + u32 pei_len; > + u8 *pei_err; > + u8 *ctx_err; > + > + pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num; > + pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm); > + > + err_info = (struct cper_arm_err_info *)(err + 1); > + ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num); > + ctx_err = (u8 *)ctx_info; > + for (n = 0; n < err->context_info_num; n++) { > + sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size; > + ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz); > + ctx_len += sz; > + } > + > + vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) + > + pei_len + ctx_len); > + if (vsei_len < 0) { > + pr_warn(FW_BUG > + "section length: %d\n", err->section_length); > + pr_warn(FW_BUG > + "section length is too small\n"); > + pr_warn(FW_BUG > + "firmware-generated error record is incorrect\n"); No need to break those lines. > + vsei_len = 0; > + } > + ven_err_data = (u8 *)ctx_info; > + > + cpu = GET_LOGICAL_INDEX(err->mpidr); > + /* when return value is invalid, set cpu index to -1 */ Obvious comment - no need for it.
Hi Boris, Em Thu, 29 Aug 2024 16:38:11 +0200 Borislav Petkov <bp@alien8.de> escreveu: > On Thu, Jul 11, 2024 at 08:28:52AM +0200, Mauro Carvalho Chehab wrote: > > In addition to those data, it also exports two fields that are > > parsed by the GHES driver when firmware reports it, e. g.: > > > > - error severity > > - cpu logical index > > s/cpu/CPU/g > > check your whole set pls. Ok. Will address those at the hole series, sending you later today a new version. Except for those, are patches 2-5 ok? Regards, Mauro > > Report all of these information to userspace via trace uAPI, So that > > userspace can properly record the error and take decisions related > > to cpu core isolation according to error severity and other info. > > > > After this patch, all the data from ARM Processor record from table > > Avoid having "This patch" or "This commit" in the commit message. It is > tautologically useless. > > Also, do > > $ git grep 'This patch' Documentation/process > > for more details. Usually, I don't use "this patch". In this specific case, I wanted to bold the new fields that were added to the ARM trace event, making clear that before the changeset, none of such fields exist; they were added on such change. On other words, the keyword here is not patch, but instead "After". Maybe I can replace it with "now", e. g.: The updated ARM trace event now contains the following fields: ====================================== ============================= UEFI field on table N.16 ARM Processor trace fields ====================================== ============================= Validation handled when filling data for affinity MPIDR and running state. ERR_INFO_NUM pei_len CONTEXT_INFO_NUM ctx_len Section Length indirectly reported by pei_len, ctx_len and oem_len Error affinity level affinity MPIDR_EL1 mpidr MIDR_EL1 midr Running State running_state PSCI State psci_state Processor Error Information Structure pei_err - count at pei_len Processor Context ctx_err- count at ctx_len Vendor Specific Error Info oem - count at oem_len ====================================== ============================= > > ... > > > [mchehab: modified patch description, solve merge conflicts and fix coding style] > > Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event") > > Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> > > Signed-off-by: Jason Tian <jason@os.amperecomputing.com> > > Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com> > > What is this SOB chain trying to tell me? > > All those folks handled the patch? They reflect what happened with past attempts of upstreaming this change at the EDAC mailing list. See, originally this seems to come from Jason Tian in 2021: https://lore.kernel.org/linux-edac/20210205022229.313030-1-jason@os.amperecomputing.com/ https://lore.kernel.org/linux-edac/20210422084944.3718-1-jason@os.amperecomputing.com/ https://lore.kernel.org/linux-edac/20210802135929.5283-1-shijie@os.amperecomputing.com/ In 2022, it came a new version from Shengwei Luo: https://lore.kernel.org/lkml/20220126030906.56765-1-lostway@zju.edu.cn/ https://lore.kernel.org/linux-edac/20220214030813.135766-1-lostway@zju.edu.cn/ A new version from Daniel Ferguson arrived in 2023: https://lore.kernel.org/linux-edac/20231214232330.306526-2-danielf@os.amperecomputing.com/ Hard to reconstruct the entire history of this changeset, as there were several attempts to fix it, and patches got renamed on some of such attempts. Anyway, it sounds that the custody chan can better be written as: Co-authored-by: Jason Tian <jason@os.amperecomputing.com> Co-authored-by: Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> Co-authored-by: Daniel Ferguson <danielf@os.amperecomputing.com> Signed-off-by: Jason Tian <jason@os.amperecomputing.com> Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com> It probably makes sense to also indicate the original author of it by change the "From" metadata to: From: Jason Tian <jason@os.amperecomputing.com> > > > Tested-by: Shiju Jose <shiju.jose@huawei.com> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section > > --- > > drivers/acpi/apei/ghes.c | 11 ++++----- > > drivers/ras/ras.c | 45 +++++++++++++++++++++++++++++++++++-- > > include/linux/ras.h | 16 +++++++++++--- > > include/ras/ras_event.h | 48 +++++++++++++++++++++++++++++++++++----- > > 4 files changed, 103 insertions(+), 17 deletions(-) > > ... > > > -void log_arm_hw_error(struct cper_sec_proc_arm *err) > > +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) > > { > > - trace_arm_event(err); > > + struct cper_arm_err_info *err_info; > > + struct cper_arm_ctx_info *ctx_info; > > + u8 *ven_err_data; > > + u32 ctx_len = 0; > > + int n, sz, cpu; > > + s32 vsei_len; > > + u32 pei_len; > > + u8 *pei_err; > > + u8 *ctx_err; > > + > > + pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num; > > + pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm); > > + > > + err_info = (struct cper_arm_err_info *)(err + 1); > > + ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num); > > + ctx_err = (u8 *)ctx_info; > > + for (n = 0; n < err->context_info_num; n++) { > > + sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size; > > + ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz); > > + ctx_len += sz; > > + } > > + > > + vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) + > > + pei_len + ctx_len); > > + if (vsei_len < 0) { > > + pr_warn(FW_BUG > > + "section length: %d\n", err->section_length); > > + pr_warn(FW_BUG > > + "section length is too small\n"); > > + pr_warn(FW_BUG > > + "firmware-generated error record is incorrect\n"); > > No need to break those lines. > > > + vsei_len = 0; > > + } > > + ven_err_data = (u8 *)ctx_info; > > + > > + cpu = GET_LOGICAL_INDEX(err->mpidr); > > + /* when return value is invalid, set cpu index to -1 */ > > Obvious comment - no need for it. > Will address at the next review. Thanks, Mauro
On Mon, Sep 02, 2024 at 06:12:36AM +0200, Mauro Carvalho Chehab wrote: > Ok. Will address those at the hole series, sending you later today > a new version. Except for those, are patches 2-5 ok? Haven't looked at them yet. > Usually, I don't use "this patch". In this specific case, I wanted > to bold the new fields that were added to the ARM trace event, making > clear that before the changeset, none of such fields exist; they were > added on such change. On other words, the keyword here is not patch, > but instead "After". Maybe I can replace it with "now", e. g.: Yes, and you can see what you're doing in the patch itself. > Anyway, it sounds that the custody chan can better be written as: > > Co-authored-by: Jason Tian <jason@os.amperecomputing.com> > Co-authored-by: Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> > Co-authored-by: Daniel Ferguson <danielf@os.amperecomputing.com> > Signed-off-by: Jason Tian <jason@os.amperecomputing.com> > Signed-off-by: Shengwei Luo <luoshengwei@huawei.com> > Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com> The tag's name is Co-developed-by: and yes, I think it makes more sense here. Note: "Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch." It is all documented: Documentation/process/submitting-patches.rst Thx.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 623cc0cb4a65..06d9351a9abc 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -529,7 +529,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, } static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, - int sev, bool sync) + int sev, bool sync) { struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata); int flags = sync ? MF_ACTION_REQUIRED : 0; @@ -537,9 +537,8 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sec_sev, i; char *p; - log_arm_hw_error(err); - sec_sev = ghes_severity(gdata->error_severity); + log_arm_hw_error(err, sec_sev); if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE) return false; @@ -773,11 +772,9 @@ static bool ghes_do_proc(struct ghes *ghes, arch_apei_report_mem_error(sev, mem_err); queued = ghes_handle_memory_failure(gdata, sev, sync); - } - else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { + } else if (guid_equal(sec_type, &CPER_SEC_PCIE)) { ghes_handle_aer(gdata); - } - else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) { + } 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_GEN_MEDIA_GUID)) { struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata); diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c index a6e4792a1b2e..359bb163aee0 100644 --- a/drivers/ras/ras.c +++ b/drivers/ras/ras.c @@ -52,9 +52,50 @@ void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id, trace_non_standard_event(sec_type, fru_id, fru_text, sev, err, len); } -void log_arm_hw_error(struct cper_sec_proc_arm *err) +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) { - trace_arm_event(err); + struct cper_arm_err_info *err_info; + struct cper_arm_ctx_info *ctx_info; + u8 *ven_err_data; + u32 ctx_len = 0; + int n, sz, cpu; + s32 vsei_len; + u32 pei_len; + u8 *pei_err; + u8 *ctx_err; + + pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num; + pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm); + + err_info = (struct cper_arm_err_info *)(err + 1); + ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num); + ctx_err = (u8 *)ctx_info; + for (n = 0; n < err->context_info_num; n++) { + sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size; + ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz); + ctx_len += sz; + } + + vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) + + pei_len + ctx_len); + if (vsei_len < 0) { + pr_warn(FW_BUG + "section length: %d\n", err->section_length); + pr_warn(FW_BUG + "section length is too small\n"); + pr_warn(FW_BUG + "firmware-generated error record is incorrect\n"); + vsei_len = 0; + } + ven_err_data = (u8 *)ctx_info; + + cpu = GET_LOGICAL_INDEX(err->mpidr); + /* when return value is invalid, set cpu index to -1 */ + if (cpu < 0) + cpu = -1; + + trace_arm_event(err, pei_err, pei_len, ctx_err, ctx_len, + ven_err_data, (u32)vsei_len, sev, cpu); } static int __init ras_init(void) diff --git a/include/linux/ras.h b/include/linux/ras.h index a64182bc72ad..df444492b434 100644 --- a/include/linux/ras.h +++ b/include/linux/ras.h @@ -24,8 +24,7 @@ int __init parse_cec_param(char *str); void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id, const char *fru_text, const u8 sev, const u8 *err, const u32 len); -void log_arm_hw_error(struct cper_sec_proc_arm *err); - +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev); #else static inline void log_non_standard_event(const guid_t *sec_type, @@ -33,7 +32,7 @@ log_non_standard_event(const guid_t *sec_type, const u8 sev, const u8 *err, const u32 len) { return; } static inline void -log_arm_hw_error(struct cper_sec_proc_arm *err) { return; } +log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) { return; } #endif struct atl_err { @@ -53,4 +52,15 @@ static inline unsigned long amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err) { return -EINVAL; } #endif /* CONFIG_AMD_ATL */ +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64) +#include <asm/smp_plat.h> +/* + * Include ARM specific SMP header which provides a function mapping mpidr to + * cpu logical index. + */ +#define GET_LOGICAL_INDEX(mpidr) get_logical_index(mpidr & MPIDR_HWID_BITMASK) +#else +#define GET_LOGICAL_INDEX(mpidr) -EINVAL +#endif /* CONFIG_ARM || CONFIG_ARM64 */ + #endif /* __RAS_H__ */ diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h index 7c47151d5c72..7d818763934f 100644 --- a/include/ras/ras_event.h +++ b/include/ras/ras_event.h @@ -168,11 +168,24 @@ TRACE_EVENT(mc_event, * This event is generated when hardware detects an ARM processor error * has occurred. UEFI 2.6 spec section N.2.4.4. */ +#define APEIL "ARM Processor Err Info data len" +#define APEID "ARM Processor Err Info raw data" +#define APECIL "ARM Processor Err Context Info data len" +#define APECID "ARM Processor Err Context Info raw data" +#define VSEIL "Vendor Specific Err Info data len" +#define VSEID "Vendor Specific Err Info raw data" TRACE_EVENT(arm_event, - TP_PROTO(const struct cper_sec_proc_arm *proc), + TP_PROTO(const struct cper_sec_proc_arm *proc, const u8 *pei_err, + const u32 pei_len, + const u8 *ctx_err, + const u32 ctx_len, + const u8 *oem, + const u32 oem_len, + u8 sev, + int cpu), - TP_ARGS(proc), + TP_ARGS(proc, pei_err, pei_len, ctx_err, ctx_len, oem, oem_len, sev, cpu), TP_STRUCT__entry( __field(u64, mpidr) @@ -180,6 +193,14 @@ TRACE_EVENT(arm_event, __field(u32, running_state) __field(u32, psci_state) __field(u8, affinity) + __field(u32, pei_len) + __dynamic_array(u8, pei_buf, pei_len) + __field(u32, ctx_len) + __dynamic_array(u8, ctx_buf, ctx_len) + __field(u32, oem_len) + __dynamic_array(u8, oem_buf, oem_len) + __field(u8, sev) + __field(int, cpu) ), TP_fast_assign( @@ -199,12 +220,29 @@ TRACE_EVENT(arm_event, __entry->running_state = ~0; __entry->psci_state = ~0; } + __entry->pei_len = pei_len; + memcpy(__get_dynamic_array(pei_buf), pei_err, pei_len); + __entry->ctx_len = ctx_len; + memcpy(__get_dynamic_array(ctx_buf), ctx_err, ctx_len); + __entry->oem_len = oem_len; + memcpy(__get_dynamic_array(oem_buf), oem, oem_len); + __entry->sev = sev; + __entry->cpu = cpu; ), - TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; " - "running state: %d; PSCI state: %d", + TP_printk("cpu: %d; error: %d; affinity level: %d; MPIDR: %016llx; MIDR: %016llx; " + "running state: %d; PSCI state: %d; " + "%s: %d; %s: %s; %s: %d; %s: %s; %s: %d; %s: %s", + __entry->cpu, + __entry->sev, __entry->affinity, __entry->mpidr, __entry->midr, - __entry->running_state, __entry->psci_state) + __entry->running_state, __entry->psci_state, + APEIL, __entry->pei_len, APEID, + __print_hex(__get_dynamic_array(pei_buf), __entry->pei_len), + APECIL, __entry->ctx_len, APECID, + __print_hex(__get_dynamic_array(ctx_buf), __entry->ctx_len), + VSEIL, __entry->oem_len, VSEID, + __print_hex(__get_dynamic_array(oem_buf), __entry->oem_len)) ); /*