Hi Ard,
Ard Biesheuvel <ardb@xxxxxxxxxx> writes:
> 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
[...]
>> 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?
Good catch. I will re-work this to avoid skipping revision 0 record.
>> + 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.
That's a better interpretation of the spec. I will follow your
suggestion and send a new version incorporating this and other
suggestions.
Thanks for taking a look.
Punit
[...]
