On Tue, 05 Aug 2025 11:35:38 -0700
Daniel Ferguson [off-list ref] wrote:

From: Jason Tian <redacted>

The ARM processor CPER record was added in UEFI v2.6 and remained
unchanged up to v2.10.

Yet, the original arm_event trace code added by

  e9279e83ad1f ("trace, ras: add ARM processor error trace event")

is incomplete, as it only traces some fields of UAPI 2.6 table N.16, not
exporting any information from tables N.17 to N.29 of the record.

This is not enough for the user to be able to figure out what has
exactly happened or to take appropriate action.

According to the UEFI v2.9 specification chapter N2.4.4, the ARM
processor error section includes:

- several (ERR_INFO_NUM) ARM processor error information structures
  (Tables N.17 to N.20);
- several (CONTEXT_INFO_NUM) ARM processor context information
  structures (Tables N.21 to N.29);
- several vendor specific error information structures. The
  size is given by Section Length minus the size of the other
  fields.

In addition, it also exports two fields that are parsed by the GHES
driver when firmware reports it, e.g.:

- error severity
- CPU logical index

Report all of these information to userspace via a the ARM tracepoint so
that userspace can properly record the error and take decisions related
to CPU core isolation according to error severity and other info.

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
======================================  =============================

It should be noted that decoding of tables N.17 to N.29, if needed, will
be handled in userspace. That gives more flexibility, as there won't be
any need to flood the kernel with micro-architecture specific error
decoding.

Also, decoding the other fields require a complex logic, and should be
done for each of the several values inside the record field.  So, let
userspace daemons like rasdaemon decode them, parsing such tables and
having vendor-specific micro-architecture-specific decoders.

  [mchehab: modified description, solved merge conflicts and fixed coding style]

Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event")

Fixes tag is part of the main tag block so no blank line here.
There are at least some scripts running on the kernel tree that trip
up on this (and one that moans at the submitter ;)

I'd also add something to explain the SoB sequence for the curious.

Co-developed-by: Jason Tian <redacted>

Jason's the Author, so shouldn't have a Co-dev tag.
There is some info on this in
https://docs.kernel.org/process/submitting-patches.html

Signed-off-by: Jason Tian <redacted>
Co-developed-by: Shengwei Luo <redacted>
Signed-off-by: Shengwei Luo <redacted>
Co-developed-by: Daniel Ferguson <redacted>
Signed-off-by: Daniel Ferguson <redacted>

As person submitting I'd normally expect your SoB last.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

I guess this is because Mauro posted an earlier version in which
case this is arguably correct, but likely to confuse.
For cases like this I add comments.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Shiju Jose <redacted>
Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section

A couple of trivial things inline.

Maybe combine the two u8 *

+
+	pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num;
+	pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm);

	pei_err = (u8 *)(err + 1);

Which is the same as err_info. Setting one to the other will make that
relationship more obvious if we do need to keep them both around.
(Similar to the ctx_err = (u8 *)ctx_info; below)

+
+	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);
+	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);
 }

Trivial but this is an odd bit of wrapping to have two items
on first line, but then only one on later lines.  Also additional
indent seems unusual and isn't matched by the first few things I looked
at in this file.  So should be under the c of const (one space I think
rather than a tab).

+			const u8 *ctx_err,
+			const u32 ctx_len,
+			const u8 *oem,
+			const u32 oem_len,
+			u8 sev,
+			int cpu),