Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43684 )
Change subject: cpu/x86/smm: SMM module loader version 2 ......................................................................
Patch Set 2: Code-Review+1
(12 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 nit: Please wrap these lines at 72 characters
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@125 PS2, Line 125: bool Could we please add a prompt here so that one can easily enable it?
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 15: uint8_t #include <stdint.h>
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 51: unsigned int Are these memory addresses? If so, I'd use uintptr_t
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__); Can we continue after this error?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF What does this magic number mean?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 176: int Shouldn't this be an uintptr_t as well?
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 213: unsigned int uintptr_t
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 226: params->smm_main_entry_offset < stack_top) { I'd indent this part differently so that it is clear it's a continuation (e.g. add a tab)
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 231: __func__, cpus[num_cpus].smbase, stack_top); one more tab
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 604: "%s: need more SMRAM\n", __func__); add another tab
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/43684/2/src/include/cpu/x86/smm.h@1... PS2, Line 152: #if CONFIG(X86_SMM_LOADER_VERSION2) nit: I would not indent preprocessor