Angel Pons 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: Code-Review+1
(18 comments)
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43684/2//COMMIT_MSG@9 PS2, Line 9: Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current
*poke*
Done
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: ( nit: space before `(`
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@9 PS5, Line 9: a no `a`
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@15 PS5, Line 15: specifc typo: specific
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@18 PS5, Line 18: smi nit: SMI
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 😄
https://review.coreboot.org/c/coreboot/+/43684/5//COMMIT_MSG@26 PS5, Line 26: smm nit: SMM
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
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 44: an extensive : number I'd mention numbers here, e.g.: `more than 32 CPU threads`
https://review.coreboot.org/c/coreboot/+/43684/5/Documentation/releases/core... PS5, Line 50: trailing whitespace
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?
int cpus = MIN(num_cpus, 24);
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.
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@804 PS5, Line 804: CPUS nit: CPUs
https://review.coreboot.org/c/coreboot/+/43684/5/src/cpu/x86/mp_init.c@806 PS5, Line 806: cpus nit: CPUs
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 51: unsigned int
done
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__);
No, we should not continue. Good catch. I've made the fix by adding a return 0 there.
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF
Yes, this can be confusing. It means 64K segment. […]
Ack, thank you
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) {
Done
Ack