Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37016 )
Change subject: security/intel/txt: Add Intel TXT support ......................................................................
Patch Set 1:
(9 comments)
Any public docs to be able to do a more in depth review?
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/comm... PS1, Line 41: if (acm_error & ACMERROR_TXT_VALID) { : const uint8_t type = (acm_error & ACMERROR_TXT_TYPE_CODE) : >> ACMERROR_TXT_TYPE_SHIFT; : : switch (type) { : case ACMERROR_TXT_AC_MODULE_TYPE_BIOS: : printk(BIOS_ERR, "BIOSACM"); : break; : case ACMERROR_TXT_AC_MODULE_TYPE_SINIT: : printk(BIOS_ERR, "SINIT"); : break; : default: : printk(BIOS_ERR, "ACM"); : break; : } : printk(BIOS_ERR, ": Error code valid\n"); : : if (acm_error & ACMERROR_TXT_EXTERNAL) : printk(BIOS_ERR, " Caused by: External\n"); : else : printk(BIOS_ERR, " Caused by: Processor\n"); : : const uint32_t class = (acm_error & ACMERROR_TXT_CLASS_CODE) : >> ACMERROR_TXT_CLASS_SHIFT; : const uint32_t major = (acm_error & ACMERROR_TXT_MAJOR_CODE) : >> ACMERROR_TXT_MAJOR_SHIFT; : const uint32_t minor = (acm_error & ACMERROR_TXT_MINOR_CODE) : >> ACMERROR_TXT_MINOR_SHIFT; : const uint32_t progress = (acm_error & ACMERROR_TXT_PROGRESS_CODE) : >> ACMERROR_TXT_PROGRESS_SHIFT; : : if (!minor) { : if (class == 0 && major == 0 && progress > 0) { : printk(BIOS_ERR, " Execution successful\n"); : printk(BIOS_ERR, " Progress code 0x%x\n", : progress); : } else { : printk(BIOS_ERR, " Error Class: %x\n", class); : printk(BIOS_ERR, " Error: %x.%x\n", major, progress); : } : } else { : printk(BIOS_ERR, " ACM didn't start\n"); : printk(BIOS_ERR, " Error Type: 0x%x\n", acm_error & 0xffffff); : return -1; : } : : return (acm_error & ACMERROR_TXT_EXTERNAL) && : class == 0 && : major == 0 && : progress > 0; : } : : return -1; reduce indentation level by reversing the if condition and returning -1
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/comm... PS1, Line 241: ((uintptr_t)acm_data) % 4096 != 0 use the IS_ALIGNED macro.
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/comm... PS1, Line 456: !(msr.lo & LAPIC_BASE_MSR_BOOTSTRAP_PROCESSOR) !boot_cpu()
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/comm... PS1, Line 502: PARALLEL_MP_AP_WORK This depends on PARALLEL_MP?
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... PS1, Line 21: CONFIG_CPU_ADDR_BITS You can get this at runtime via cpuid 0x80000008.
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... PS1, Line 45: (MTRR_CAP_MSR) parentheses not required?
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... PS1, Line 61: (0x200 + 2 * (\x) + 1) MTRR_PHYS_MASK(0) + 2 * (\x).
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... PS1, Line 62: (0x200 + 2 * (\x)) MTRR_PHYS_BASE(0) + 2 * (\x).
https://review.coreboot.org/c/coreboot/+/37016/1/src/security/intel/txt/gets... PS1, Line 319: %ebp,%esp space after ','