Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37016 )
Change subject: security/intel/txt: Add Intel TXT support ......................................................................
Patch Set 8:
(16 comments)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... File src/security/intel/txt/logging.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 18: static void log_txt_error(const char *phase)
Are these prints meant for preproduction only? I see some of these strings in `intel_txt_log_acm_err […]
SINIT ACM errors are handled differently from BIOS ACM errors
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 146: (acm_header->size & 0xffffff) << 2)
acm_size
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 150: printk(BIOS_INFO, " Flags: PW signed (Production Worthy)\n");
One tab too much?
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 161: Cooperation
Corporation?
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 227:
nit: since SINIT is one character longer than HEAP and MSEG, add one space so that the numbers are a […]
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 228: read64((void *)TXT_HEAP_SIZE));
I think these all fit in 96 characters. […]
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... File src/security/intel/txt/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 12: #include <soc/intel/common/block/include/intelblocks/systemagent.h>
Seems to be unused?
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 29: /* MSEG SIZE and MSEG BASE might contain random values. : * Assume below 4GiB and 8byte aligned. : */
Comment should start like this: […]
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 101: sobber
"sober"?
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 152: Failed to prepare TXT environment
Maybe update this message to distinguish it from the case above?
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 161: Tested
Tests showed
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 165: .
add: […]
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 293: "CBFS.\n");
Fits in 96 characters
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... File src/security/intel/txt/txt_getsec.h:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 6:
Double empty line
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... File src/security/intel/txt/txt_register.h:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 73: /* Only present in Document Number: 315168-010 and 315168-016 */
TXT_VER_QPIIF is also present on 322372-002 and 315168-011
Done
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 131: #define TXT_FILE_LCP_PD_ACM "txt_bios_policy.bin"
hrm, is there a better place for CBFS filenames? That way, one doesn't have to keep them in sync wit […]
Done