Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 29: Code-Review-1
(12 comments)
- Previous comments made on the unquashed patche series still apply - please add a config so Jenkins actually builds the STM code - additional comments would be great to understand why it's done
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@9 PS29, Line 9: combination It can be done without breaking anything.
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: resource Why does it create a resource list? Where is it stored? What spec was used to write the code?
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: tables Where reside the page tables? What mapping is used? Identity mapping? Why does it use paging?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... PS29, Line 5: default Some Intel platforms are missing, why?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... PS29, Line 6: (PLATFORM_USES_FSP2_0||PLATFORM_USES_FSP1_1||PLATFORM_USES_FSP1_0) FSP1_0 support was dropped. Shouldn't this depend on the presence of TSEG?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 483: retval Never returned
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformResource.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 113: ACPI_BASE_ADDRESS Platform specific code doesn't belong here
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 116: attribute__((weak)) No need for weak
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 126: 255 Depends on the mmconf size
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 157: Fix-up What's fixed?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformSmm.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 32: retval No true, returns only a single retval
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 81: mem_region_device_ro_init(&stm_region No commented code