cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 57:
(3 comments)
Responded to Nico Huber's comments
https://review.coreboot.org/c/coreboot/+/33234/57/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/33234/57/src/cpu/x86/mp_init.c@750 PS57, Line 750: if (is_smm_enabled()) {
What's this about? AFAICS, we are in SMM.
You're correct. I didn't catch that trigger_smm_relocation prevented this code path. I will issue a correction.
https://review.coreboot.org/c/coreboot/+/33234/57/src/cpu/x86/smm/smm_module... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/33234/57/src/cpu/x86/smm/smm_module... PS57, Line 365: base += size - CONFIG_MSEG_SIZE; // take out the mseg
What is MSEG, why is it not in the picture above?
MSEG, aka monitor segment, is where the STM is loaded. It is normally at the top of the TSEG, and in coreboot the MSEG is located in the region above where the SMI handler is located. It was not added in this diagram because the function is focused on the SMI handler (aka SMM module)
If you think that it will add clarity, I can add the MSEG to the diagram.
https://review.coreboot.org/c/coreboot/+/33234/57/src/cpu/x86/smm/smm_module... PS57, Line 398: // account for the bios resource list
This comment and the empty lines kind of break the relation of the comment […]
I will take out the blank line