ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 21:
(7 comments)
So if you can address these comments, and we can work out why this won't build, we can get this in soon I hope.
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... File src/security/intel/stm/SmmStm.h:
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... PS21, Line 43: * @retval OUT_OF_RESOURCES If nested procedure returned it and we cannot I think you need a space here.
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... PS21, Line 77: * This function notify STM resource change. Notify and STM resource change
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... PS21, Line 84: * This function return BIOS STM resource. Get a BIOS STM resource
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Smm... PS21, Line 295: UINTN index; it would be nice to use uintptr_t instead of UINTN. I realize Intel likes it but ... uintptr_t is a bit more standard.
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Stm... File src/security/intel/stm/StmApi.h:
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Stm... PS21, Line 26: typedef uint64_t UINTN; oh boy. We have 22 different typedefs of UINTN in the coreboot tree today. In some code Intel makes this a uintptr_t, and here it looks like it's a uint64_t. Could we just replace all uses of UINTN with uint64_t in the STM code so people don't have to dig around? Otherwise it gets confusing.
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Stm... PS21, Line 60: uint32_t stm_smm_rev_id[1]; BY this did they mean an array of one uint32_t or an indeterminate length array? If the latter this is a pre-gcc 3.0 usage. If the former, why an array?
https://review.coreboot.org/c/coreboot/+/33234/21/src/security/intel/stm/Stm... PS21, Line 83: uint64_t image_page_base[1]; //[NumberOfPages]; why 1 ...