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 8:
(7 comments)
Patch Set 8:
Just wondering if it wouldn't be easier to use some linker script to generate the correct SMRAM layout and then put everything into the SMM rmodule.
Benefits:
- You would get a build error if you try to use more space than available
- No magic calculations that need to be done, as everything is known at compile time
- Nothin to set up as everything is already in place
Downside: Bigger SMM rmodule (hopefully compression catches that)
For long term a dynamic is method is better. A linker script would solve the problem temporarily but it will be more work. A lot of effort has already been placed into fixing the existing SMM loader. There are certain places where it is necessary to take action based on the CPUID or different flags in MSRs (such as state save flags) and this cannot be done at compile time. Also, the handlers are installed based on the number of CPU threads detected in the system.
In addition, the SMM handler will need to go through more changes later on. Currently the entry into SMM is flat 32 real mode. Intel now supports protected mode entries and 64 bit support in SMM. This is beneficial for server platforms mostly.
Another approach is that we could call this SMM module loader version 2. Only the server platforms will use version 2 and client/desktop/etc can continue to use the old version. It will make this patch integrate much better into the existing model.
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).
Done
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. […]
It's the smm relocation record that needs the delay. The delay is a workaround for now. .relocation_handler = smm_relocation_handler
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
Done
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
Done
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.
It's part of enabling SMM support for Xeon-SP. Without relocation SMM wouldn't be enabled as per the commit message titie.
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 '*'
good catch.
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.
Done