Patrick Rudolph 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 6:
(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?
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
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
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@734 PS6, Line 734: smbase SMBASE
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/mp_init.c@772 PS6, Line 772: num_cpus unused argument
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
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
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?
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 510: +-----------------+ <- that ascii art is now useless
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 513: * +-----------------+ why is there an empty section?
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 519: cpu0 what's the offset within SMRAM?
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 554: unsigned uintptr_t
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?
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 650: unsigned uintptr_t
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?
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 should be a separate commit.
-
Patrick Rudolph (Code Review)