Hello Punit,
On Mon, 27 Apr 2020 at 11:03, Punit Agrawal
<punit1.agrawal@xxxxxxxxxxxxx> wrote:
>
> While debugging a boot failure, the following unknown error record was
> seen in the boot logs.
>
> <...>
> BERT: Error records from previous boot:
> [Hardware Error]: event severity: fatal
> [Hardware Error]: Error 0, type: fatal
> [Hardware Error]: section type: unknown, 81212a96-09ed-4996-9471-8d729c8e69ed
> [Hardware Error]: section length: 0x290
> [Hardware Error]: 00000000: 00000001 00000000 00000000 00020002 ................
> [Hardware Error]: 00000010: 00020002 0000001f 00000320 00000000 ........ .......
> [Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................
> [Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................
> <...>
>
> On further investigation, it was found that the error record with
> UUID (81212a96-09ed-4996-9471-8d729c8e69ed) has been defined in the
> UEFI Specification at least since v2.4 and has recently had additional
> fields defined in v2.7 Section N.2.10 Firmware Error Record Reference.
>
> Add support for parsing and printing the defined fields to give users
> a chance to figure out what's went wrong.
>
> Signed-off-by: Punit Agrawal <punit1.agrawal@xxxxxxxxxxxxx>
> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: James Morse <james.morse@xxxxxxx>
> Cc: linux-acpi@xxxxxxxxxxxxxxx
> Cc: linux-efi@xxxxxxxxxxxxxxx
> ---
>
> Hi,
>
> I ran into this while debugging a boot failure on an end product
> device. Parsing known record types can help the user narrow down the
> investigation quickly to the failing components - firmware error in
> this case.
>
> It would be good to support the record types specified in the
> standard.
>
> Feedback welcome.
>
> Thanks,
> Punit
> ---
> drivers/firmware/efi/cper.c | 49 +++++++++++++++++++++++++++++++++++++
> include/linux/cper.h | 11 +++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 9d2512913d25..153b95257e23 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -407,6 +407,46 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
> }
> }
>
> +static const char * const fw_err_rec_type_strs[] = {
> + "IPF SAL Error Record",
> + "SOC Firmware Error Record Type1 (Legacy CrashLog Support)",
> + "SOC Firmware Error Record Type2",
> +};
> +
> +static void cper_print_fw_err(const char *pfx,
> + struct acpi_hest_generic_data *gdata,
> + const struct cper_sec_fw_err_rec_ref *fw_err)
> +{
> + void *buf = acpi_hest_get_payload(gdata);
> + u32 offset, length = gdata->error_data_length;
> +
> + printk("%s""Firmware Error Record Type: %s\n", pfx,
> + fw_err->record_type < ARRAY_SIZE(fw_err_rec_type_strs) ?
> + fw_err_rec_type_strs[fw_err->record_type] : "unknown");
> +
> + /* Record Type based on UEFI 2.7 */
> + if (fw_err->revision == 0)
> + printk("%s""Record Identifier: %08llx\n", pfx,
> + fw_err->record_identifier);
> + else if (fw_err->revision == 2)
> + printk("%s""Record Identifier: %pUl\n", pfx,
> + &fw_err->record_identifier_guid);
> +
Please use {} for multi-line statements between the ifs
> + if (fw_err->revision == 0)
> + offset = offsetof(struct cper_sec_fw_err_rec_ref,
> + record_identifier_guid);
> + else if (fw_err->revision == 1)
> + offset = offsetof(struct cper_sec_fw_err_rec_ref,
> + record_identifier);
> + else
> + offset = sizeof(*fw_err);
> +
This logic is slightly confusing, so it could do with a comment
regarding which part of the structure is being dumped and why.
> + buf += offset;
> + length -= offset;
> +
> + print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, buf, length, true);
> +}
> +
> static void cper_print_tstamp(const char *pfx,
> struct acpi_hest_generic_data_v300 *gdata)
> {
> @@ -494,6 +534,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
> else
> goto err_section_too_small;
> #endif
> + } else if (guid_equal(sec_type, &CPER_SEC_FW_ERR_REC_REF)) {
> + struct cper_sec_fw_err_rec_ref *fw_err = acpi_hest_get_payload(gdata);
> +
> + printk("%ssection_type: Firmware Error Record Reference\n",
> + newpfx);
> + if (gdata->error_data_length >= sizeof(*fw_err))
> + cper_print_fw_err(newpfx, gdata, fw_err);
This doesn't work for revision 0 structures unless they happen to have
some trailing data, which is not necessarily the case, right?
> + else
> + goto err_section_too_small;
> } else {
> const void *err = acpi_hest_get_payload(gdata);
>
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 4f005d95ce88..5cb57e69df70 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -521,6 +521,17 @@ struct cper_sec_pcie {
> u8 aer_info[96];
> };
>
> +/* Firmware Error Record Reference, UEFI v2.7 sec N.2.10 */
> +struct cper_sec_fw_err_rec_ref {
> + u8 record_type;
> + union {
> + u8 revision;
> + u8 reserved[7];
> + };
Even though the spec is slightly silly here, I think we can avoid the
union, and just have
u8 record_type;
u8 revision;
u8 reserved[6];
as the leading fields.
> + u64 record_identifier;
> + guid_t record_identifier_guid;
> +};
> +
> /* Reset to default packing */
> #pragma pack()
>
> --
> 2.26.2
>
