Change in coreboot[master]: acpi: Add support for reporting CrashLog in BERT table

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49799 )

Change subject: acpi: Add support for reporting CrashLog in BERT table
......................................................................

Patch Set 10:

(5 comments)

Patchset:

PS10: 
This picture has to be used as reference to fully understand the changes. They are valid, as fully verified in the CrashLog flow. Explanations are provided below. I still believe updating your data offset would be a more sounds approach; however we will move the raw data tracking to the Intel SOC common code. Which is one of several possible WA to unblocking you.

+-------------------------------------------------------------------+
| Boot Error Region (ACPI 6.3 - Table 18-381)                       |
| +---------------------------------------------------------------+ |
| | Generic Error Status Block (ACPI 6.3 - Table 18-391)          | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #1                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #2                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| | | ...                                                       | | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #n                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| +---------------------------------------------------------------+ |
+-------------------------------------------------------------------+

File src/arch/x86/acpi_bert_storage.c:

PS10:
> It was also part of the 6.2 spec that was active when the code was written. […]

Correct, there is no requirement to report this, but it is used in the by the Intel SOC decoding flow.
For the "common good" we will tentatively be using another reporting work around in the Intel SOC specific code.

https://review.coreboot.org/c/coreboot/+/49799/comment/8a2f545d_b5257acf 
PS10, Line 109: status->raw_data_length += size;
> This doesn't seem right. […]

Actually this is right ant makes sense. The size applies to both because the "raw data" are part (sub-section) of the generic section. And in the case there are additional (non register) info in the generic section, they are also "raw data" since there contains IP level info.
The picture below captures this well and should remove the confusion.

+-------------------------------------------------------------------+
| Boot Error Region (ACPI 6.3 - Table 18-381)                       |
| +---------------------------------------------------------------+ |
| | Generic Error Status Block (ACPI 6.3 - Table 18-391)          | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #1                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #2                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| | | ...                                                       | | |
| | +-----------------------------------------------------------+ | |
| | | Generic Data Error Data Entry (ACPI 6.3 - Table 18-392)   | | |
| | | +-------------------------------------------------------+ | | |
| | | | CPER FW Error Record Reference (UEFI 2.8 - N.2.10)    | | | |
| | | | +---------------------------------------------------+ | | | |
| | | | | Crash Log Region #n                               | | | | |
| | | | +---------------------------------------------------+ | | | |
| | | +-------------------------------------------------------+ | | |
| | +-----------------------------------------------------------+ | |
| +---------------------------------------------------------------+ |
+-------------------------------------------------------------------+

That change is valid, an working just fine with Chrome. But to prevent disturbance/noises to your AMD flow with linux flow, it will be pulled into the intel SOC common code.

https://review.coreboot.org/c/coreboot/+/49799/comment/af3438cb_1db519d3 
PS10, Line 178: status->raw_data_length += sizeof(*entry);
> This is a generic error entry.  Raw data length doesn't come into play here.

Same explanations above applies here. Raw data are part/section of the generic error entry.

https://review.coreboot.org/c/coreboot/+/49799/comment/6fa0295a_2dd11882 
PS10, Line 531: status->raw_data_length = sizeof(*status);
> I don't understand this. […]

These 20 bytes need to be accounted for during the data analysis in the Intel decoder.
Once again this will tracked at the intel SOC common code level.
so for now you can proceed with  CB:54738