Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37016 )
Change subject: security/intel/txt: Add Intel TXT support ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 135: 4
What's this magic number?
sizeof(uint32_t)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 316: locked
If it's not locked, isn't that a serious error? I'd expect that TXT is enabled after the MSR is lock […]
maybe
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 364: return true;
Maybe add some message? […]
In that case it already prints "TEE-TXT: CPU supports SMX: false"
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 425: policy
Which policy?
TXT Policy Data Record (Firmware Inteface Table Type 0x0A)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... File src/security/intel/txt/getsec.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 65: 0x1f
What does this 0x1f mean?
That's taken from some TXT Spec. The SDM uses 4GiB-1 here, so we should use the same
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 191: 1:
Where is this used?
looks unused
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 192: GPE
GPE or GP? Or GPF? GPE reminds me of General Purpose Event and I get confused
GPF
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 245: 1:
I think this one is used right above?
yes
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 161: Tested Tests showed