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 21:
(7 comments)
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.
added space
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
done
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
Changed to: "returns the pointer to the STM BIOS resource list"
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 ... […]
changed to uint32_t, scanned the other modules and changed UINTN to the appropriate type
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. […]
Since I changed all the UINTN's, this typedef has been removed
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 […]
Indeterminate length array where the length is determined by the "num_of_rev_ids" field.
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 ...
In the STM spec, the image_page_base can be multiple entries. In this implementation, only a single page is supported.