Name of user not set #1002358 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)
Patch Set 6:
Eugene, if you're willing, I'd like to help you get this into Coreboot Normal Form. It's not that there's anything terribly wrong with this code, it just needs to be reshaped a tad. Can I help?
Thanks for the offer. The next patch set will non-CamelCase and it should be in line with the coreboot coding standard. So, wait until I put out the next patchset as it is 99% done.
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.
Done
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.
Done
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 195: Setup
Set up
Done
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?
The MSEG (MSR) and SMM Descriptor has to be initialized on a per-thread (logical processor) basis. The duplication is a result of an earlier review that suggested that console_init be called only once. My earlier implementation resulted in console_init being called twice.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_handler.c... PS6, Line 211: Setup
Set up
Done
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?
Done
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?
I'm confused, this is not the added code.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 195: e num
Set up
This seems to be a bug not related to the contribution.
https://review.coreboot.org/#/c/33235/6/src/cpu/x86/smm/smm_module_loader.c@... PS6, Line 211:
Set up
Same here.
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?
Same here, it seems to be a bug.
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.
Done
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 //.
Done
https://review.coreboot.org/#/c/33235/1/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/#/c/33235/1/src/cpu/x86/smm/smm_stub.S@51 PS1, Line 51: /* allows the STM to bring up SMM in 32-bit mode*/
This variable is consumed in smm_handler_start as part of the STM/MSEG initialization
Done
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 */.
Done