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 33:
(13 comments)
To do: I am looking for guidance/direction about the config to get Jenkins to build the STM.
Next patch set will be out Fri/Sat and will include the addition of get_pmbase for all Intel platforms. Unless the reviewers want me to separate them into separate CL's
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: resource
Thanks, I found that document. […]
Will do.
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: tables
That's an important fact that's need to be mentioned. […]
At this point we are talking about STM internals.
(1) SMM does support IA32e mode, this support has existed before sandybridge and it is the mode that the STM functions in. See Chapter 5 of the STM User Guide. Also, the STM that we are using functions in that manner.
(2) The six pages noted above are actually located in the STM image, which by this time has been moved by the BIOS into the MSEG.
(3) The discussion in Chapter 5 talks about TXT based systems, in this case, SINIT creates the STM page tables and clears the STM heap. In non-TXT systems, the BIOS has to perform these functions.
This patch is responsible for moving the STM into the MSEG, setting up the BIOS resource list, creating the STM page tables and clearing the STM heap.
The operating system is responsible for starting the STM by doing a VMCALL from the root virtual machine.
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Kco... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Kco... PS33, Line 40: config ACPI_BASE_ADDRESS
that's defined in the platform headers. […]
Okay - I will add get_pmbase to all the intel patforms.
I will make those changes a part of this CL, unless you rather me make them as separate CL's
Note: next patch set will not be out until probably Friday or Saturday, too much going on.
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Smm... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Smm... PS33, Line 603: void stm_gen_4g_pagetable_ia32(uint32_t pagetable_base)
this is unused
Correct. This for those folks who want to run the STM as 32-bit. Since we are loading as x64 and the Intel documentation suggests that is the way to go, I am removing the code for now.
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Smm... PS33, Line 621: x86_32
it only creates 2M PTEs
Fixed
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Smm... PS33, Line 641: for (index = 0; index < 4; index++) {
this can be placed in CBFS and used with CB:36778 applied. […]
The problem with gen_pgtbl_x86_64 is that it uses malloc. The STM expects that it's page tables are being created in a six page region within the MSEG. The STM implementation has a specific memory layout where it expects everything to be and the location of the STM page tables are provided in the STM hardware header.
Not sure what you mean by "static page tables in rom"
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformSmm.c:
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 122: uint32_t
uintptr_t
Fixed
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 127: uint32_t
uintptr_t
Fixed
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 128: psd->smm_cs = 0x8;
ROM_CODE_SEG
Wouldn't it be better to use something like SMM_CODE_SEG, since it is pointing to the SMI handler, which is in SMM and not ROM?
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 129: 0x10
ROM_DATA_SEG
SMM_DATA_SEG?
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 130: 0x10
ROM_DATA_SEG
SMM_DATA_SEG?
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 131: 0x10
ROM_DATA_SEG
SMM_DATA_SEG?
https://review.coreboot.org/c/coreboot/+/33234/33/src/security/intel/stm/Stm... PS33, Line 132: psd->smm_tr = 0x18;
ROM_CODE_SEG64
SMM_CODE_SEG64?