Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41829 )
Change subject: cpu/x86/smm: Enable SMM support for Xeon-SP ......................................................................
Patch Set 7:
(16 comments)
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/Makefile.inc File src/cpu/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/Makefile.inc@2 PS6, Line 2: +=
is this necessary?
yes, otherwise the code in smm directory compiles unnecessarily.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@508 PS6, Line 508: trigger_smm_relocation
not used
Done..removed.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@545 PS6, Line 545: mdelay
sounds like a race condition somewhere
Yes, there might be. Will address this in a future patch as it requires a lot more debug.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@734 PS6, Line 734: smbase
SMBASE
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@772 PS6, Line 772: num_cpus
unused argument
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@775 PS6, Line 775: relocate
that's not explained in the commit message
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@803 PS6, Line 803: * All the CPUs will relocate to permanaent handler now.
please use the full line width for comments
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 70: * | common |
where are the code segments in this picture?
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 510: +-----------------+ <-
that ascii art is now useless
yeah, not sure what else to do there. I just built on top of what was already there.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 513: * +-----------------+
why is there an empty section?
It shouldn't be empty...fixed..
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 519: cpu0
what's the offset within SMRAM?
depends on the size of the code/data sections above (STM, Fxsave, smi handler region). also the number of CPU threads in the system will impact the entry points. Basically the entry points are all staggered (tiled) in SMRAM.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 554: unsigned
uintptr_t
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 575: total_size
why is total_size overwritten here?
That's a good catch. It is a bug, I have fixed it now. It should be: total_size += CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE;
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 650: unsigned
uintptr_t
Done
https://review.coreboot.org/c/coreboot/+/41829/6/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/41829/6/src/include/cpu/x86/smm.h@1... PS6, Line 123: smm_entry
are those offsets or addresses?
clarification added.
https://review.coreboot.org/c/coreboot/+/41829/6/src/soc/intel/xeon_sp/smmre... File src/soc/intel/xeon_sp/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/41829/6/src/soc/intel/xeon_sp/smmre... PS6, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
unrelated to changes in cpu/x86/smm... […]
This is needed. The existing relocation code does not work when CPU thread count is greater ~30 (depends on other features also). The title of this patch "Enable SMM support for Xeon-SP" includes relocation. Thus to fully enable SMM, relocation has to function properly.
-
Rocky Phagura (Code Review)