Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: Introduce SMM module loader version 2 ......................................................................
Patch Set 5:
(14 comments)
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: a
no `a`
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: (
nit: space before `(`
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@15 PS5, Line 15: specifc
typo: specific
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@18 PS5, Line 18: smi
nit: SMI
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@23 PS5, Line 23: must is
`must` or `is`? Pick one of them, but not both 😄
Done
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@26 PS5, Line 26: smm
nit: SMM
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... File Documentation/releases/coreboot-4.13-relnotes.md:
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44: an extensive : number
I'd mention numbers here, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 50:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@775 PS5, Line 775: cpus = 1;
Instead of serializing relocation, could we use a smaller number instead? […]
This would be a performance and optimization setting. SMM is already very difficult to debug. How about we save this for a future patch?
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@782 PS5, Line 782: cpus
This changes the number of concurrent save states for the !X86_SMM_LOADER_VERSION2 case.
It shouldn't unless I missed something. On line 794: int cpus = num_cpus;
will take care of it for the older loader version.
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@804 PS5, Line 804: CPUS
nit: CPUs
Done
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@806 PS5, Line 806: cpus
nit: CPUs
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loaderv2.c:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 123: not enough space in SMM to setup all CPUs
From reading the statements on line 112, 114 and 116, the error message in line 123 is somewhat misl […]
Done. Clarification added.