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 4: Code-Review+1
(15 comments)
Patch Set 4:
(1 comment)
Angel, I incorporated your changes from the other patch. I've added one more change in mp_init.c. There are 2 SMM regions:
- Default Region at 30000h. During this relocation phase only so many CPUs can relocate at the same time otherwise the state save will corrupt the entry code. Therefore, you will notice I added code to handle this case which means CPUs will relocate one at a time. This will add a slight delay to the boot time (hopefully its negligible). Unfortunately there is nothing we can do because of the SMRAM HW limitations.
IMHO, looks good. I've bumped all comments from older patchsets, most of which are not done yet.
Another thing: to make sure this code gets build-tested, I would add `CONFIG_X86_SMM_LOADER_VERSION2=y` to `configs/config.asrock_b85m_pro4.debug_smmstore_hotplug_gcov_ubsan_em100`. This is a config file I've added to build-test several options that were previously not build-tested.
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
*poke*
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?
No need
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/Kconfig@129 PS2, Line 129: server
nit: maybe deemphasize the "server" mentions a bit? There are few non-server designs out there with […]
Agreed, this works even on non-server stuff
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/43684/4/src/cpu/x86/mp_init.c@734 PS4, Line 734: __func__, cpu); Fits on the previous line (line length limit is 96 characters)
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. […]
*poke*
Or, if you use `bool` and `size_t`, just #include <types.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
*poke*
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?
*poke*
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 112: 0xFFFF
What does this magic number mean?
*poke*
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
So how would a firmware developer start to solve this, or is SMM just not available on designs with […]
*poke*
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?
Done
https://review.coreboot.org/c/coreboot/+/43684/2/src/cpu/x86/smm/smm_module_... PS2, Line 213: unsigned int
uintptr_t
*poke*
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. […]
*poke*
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
*poke*
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
*poke*
Or reflow, it fits in a single line
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
*poke*