Attention is currently required from: Marc Jones, Furquan Shaikh, Duncan Laurie, Angel Pons. Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49286 )
Change subject: src/acpi: Add APEI EINJ support ......................................................................
Patch Set 4:
(21 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49286/comment/2baa6e4f_27ab5fbe PS4, Line 9: This adds full EINJ support with trigger action tables. The actual error injection functionality is HW specific. Therefore, HW specific code should call acpi_create_einj with an address where action table resides. The default params of the action table are filled out by the common code. Control is then returned back to the caller to modify or override default parameters. If no changes are needed, caller can simply add the acpi table. At runtime, FW is responsible for filling out the action table with the proper entries. The action table memory is shared between FW and OS. This memory should be marked as reserved in E820 table.
Please break commit message lines at 72 characters
Done
File src/acpi/Kconfig:
https://review.coreboot.org/c/coreboot/+/49286/comment/8d0d7315_237cbf7a PS4, Line 42: This variable provides control for ACPI error injection table (EINJ)
This does absolutely nothing.
it was added for completeness. For anyone that will add the SOC specific code can use this instead of creating some new config option. It's used currently by me for the code that is not "upstreamable" yet. Since it doesn't hurt anything and it will be used later, we can leave it here for now.
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/49286/comment/f85573f8_9fdffe41 PS4, Line 925:
Seems unrelated?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/3c6662eb_22126023 PS4, Line 778: u64
Why not use `uintptr_t` instead?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/cb134426_0b5c30ba PS4, Line 785: intptr_t
*u*intptr_t ? […]
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/804eb1d2_72c8c2b8 PS4, Line 787: sizeof(acpi_einj_smi_t)
sizeof(*einj_smi)
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/4247a0eb_0b69397b PS4, Line 787: (void *)
Are all these (void *) casts really necessary?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/6d4f24cf_cc92990d PS4, Line 796: actions
The way the struct type is defined, `actions` can only be less than 1.
That's correct, but this piece of the table belongs to the caller defined memory and this code initializes to the defaults. The caller has the option to override these settings once control is returned.
https://review.coreboot.org/c/coreboot/+/49286/comment/05dd06cb_417d2e6c PS4, Line 809: []
[ACTION_COUNT]
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/94b91177_a22dc734 PS4, Line 810: /* Action 0 */
Instead of having these comments, you could initialise the array indices explicitly: […]
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/89a1d07d_53fbaeb0 PS4, Line 825: 0x0
Why not simply 0?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/44b56cf4_2bb34218 PS4, Line 894:
Casts are unary operators and do not use spaces
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/5682faf1_71afe093 PS4, Line 902: default_actions[8].reg.addr = (u64) (uintptr_t)&einj_smi->setaddrtable;
Why not add the addresses in the initializer?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/d2e409b7_b09557b3 PS4, Line 906: (unsigned int long)
Uh, drop the cast and use %p instead?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/3c0c9d8d_9d944602 PS4, Line 908: acpi_einj_t
*einj
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/288a74d5_bbc4638f PS4, Line 910: return;
Maybe exit early?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/fa23bd92_4c8140e9 PS4, Line 925: unsigned int long
Uh, drop the cast and use %p instead?
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/86916a2d_1b81ec0d PS4, Line 927: sizeof(acpi_einj_action_table_t) * ACTION_COUNT
`sizeof(einj->action_table)` or `sizeof(default_actions)` please
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/dac4dc0e_5ef5da38 PS4, Line 928: acpi_einj_t
*einj
Done
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/49286/comment/15a910ad_3bd79c4a PS4, Line 963: +
Um, these are bitmasks. […]
Done
https://review.coreboot.org/c/coreboot/+/49286/comment/e59e6c9e_8e9fe410 PS4, Line 1055: 0
Pass the address through a macro parameter?
that is an option but at this point I will leave it as is to reduce complexity and increase clarity.
-
Rocky Phagura (Code Review)