Name of user not set #1002358 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Kconfig File src/security/intel/stm/Kconfig:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Kconfig@5 PS6, Line 5: bool "Enable STM"
Please give more details here, what STM is, and why it should be enabled.
Done
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc File src/security/intel/stm/Makefile.inc:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc@... PS6, Line 2: # put the stm where is can be found
where *it*
Done
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc@... PS6, Line 4: cbfs-files-y += stm.bin
the convention here would be to replace -y with $(CONFIG_STM) so I'm wondering why you did not do th […]
no, just still figuring this out.
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.h File src/security/intel/stm/SmmStm.h:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.h@20 PS6, Line 20: #define IA32_VMX_BASIC_MSR_INDEX 0x480
is there any reason not to just put these in src/include/cpu/x86/msr. […]
Done
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c@27 PS6, Line 27: #define RDWR_ACCS 3
It would be much better if we could get most of these constants in a generic place. […]
Agreed. Some of these could be merged with the TXT support as the STM does utilize the TXT registers when available. Others, like the IA32_PG_*, I am not so sure about since they are related to page tables and a quick scan of the source didn't turn up anything related.
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/StmPlatformSm... File src/security/intel/stm/StmPlatformSmm.c:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/StmPlatformSm... PS6, Line 51: StmImageSize = cbfs_boot_load_file("stm.bin", MsegBase, StmBufferSize,
Maybe do measurements here
Done