Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33235 )
Change subject: cpu/x86/smm: Add STM Support ......................................................................
Patch Set 6:
(14 comments)
https://review.coreboot.org/#/c/33235/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33235/6//COMMIT_MSG@9 PS6, Line 9: setup The verb is spelled with a space: set up.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c File src/cpu/x86/smm/smm_module_handler.c:
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 144: * If a single processor lags then a locking/counting scheme will Please check the coding style on how to format concise multi-line comments.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 195: Setup Set up
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 198: printk(BIOS_DEBUG, "MSEG Initialized (%d) 0x%08x 0x%08x\n", Same debug message?
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 211: Setup Set up
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 165: : /* : * The stub setup code assumes it is completely co Gerrit syntax highlighting bug?
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 179: void *fxsave_area) Same debug message again?
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 195: e num Set up
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 211: Set up
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 211: : /* Need a minimum stack size and alignment. */ : if (params->per_cpu_stack_size <= SMM_MINIMUM_STACK_SIZE || : (params->per_cpu_stack_size & 3) != 0) : return -1; : : smm_stub_loc = NULL; : smm_stub_size = rmodule_memory_size(&smm_stub); : stub_entry_offset = rmodule_entr Can this somehow be refactored into a separate function?
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 274: #ifdef CONFIG_STM Please use C code to check the Kconfig variable, if possible.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 277: //STM not configured - no mseg Space after //.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_stub.S@51 PS6, Line 51: /* allows the STM to bring up SMM in 32-bit mode*/ Please add a space before */.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_stub.S@277 PS6, Line 277: Please add a space.