Jonathan Zhang 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:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41829/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41829/7//COMMIT_MSG@9 PS7, Line 9: Xeon-SP a skylake based system has 36 CPU threads (18 cores). Current coreboot Skylake Scalable Process can have 36 cpu threads (18 cores).
https://review.coreboot.org/c/coreboot/+/41829/7/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/41829/7/src/cpu/x86/mp_init.c@541 PS7, Line 541: mdelay(1); This will add to boot time. Let's find out which MP record needs such added delay, when SMM is enabled.
https://review.coreboot.org/c/coreboot/+/41829/7/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/41829/7/src/cpu/x86/smm/smm_module_... PS7, Line 354: printk(BIOS_INFO, "%s : not enough stacks\n", __func__); The error conditions should be printk'ed as BIOS_ERR
https://review.coreboot.org/c/coreboot/+/41829/7/src/cpu/x86/smm/smm_module_... PS7, Line 612: "%s : increase SMM_CODE_SEGMENT_SIZE\n", __func__); print out handler_size will help to know to increase SMM_CODE_SEGMENT_SIZE
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/Makef... File src/soc/intel/xeon_sp/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/Makef... PS7, Line 11: ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smmrelocate.c The xeon_sp related changes should be made into a separate patch.
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/smmre... File src/soc/intel/xeon_sp/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/smmre... PS7, Line 106: * BSP to do * the final move. For APs, a relocation handler always There is an extra '*'
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/uncor... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/41829/7/src/soc/intel/xeon_sp/uncor... PS7, Line 290: void smm_region(uintptr_t *start, size_t *size) Add an empty line above.
-
Jonathan Zhang (Code Review)