Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44880 )
Change subject: security/intel/txt: Improve MTRR setup for GETSEC[ENTERACCS] ......................................................................
Patch Set 11:
(3 comments)
Patch Set 11:
(4 comments)
Looking good!
I'd think that passing a MTRR handof struct (maybe even reuse the one from postcar) and setting things up in C code could make our lives a lot easier?
It would make things harder for Haswell, though: I need to call the SCLEAN function of the BIOS ACM in early romstage if there are secrets from DRAM. If I don't do it, memory initialization will fail (MRC hangs; I think because the MPLL never starts up). The current, known-working implementation I have uses roughly the same MTRR setup code, but with a different prologue/epilogue.
I'm in process of extracting this romstage SCLEAN launch out of the big 'golden' (works, but is heavy) change I have.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 183: Use XMM to store local variables
Maybe we should document that after all MTRR's are saved, the rest should be stackless?
I decided to use XMM registers for this because I saw them being used later on (right before filling in the parameters for the getsec instruction), and decided to use them here too. Plus, I don't have any stack in romstage (I tore it down earlier), and there I use four XMM registers.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 195: /* Check if there are enough variable MTRRs to cache this size */ : popcnt %ebx, %edx : cmp %eax, %edx
I guess some of the checks could be moved to C code before calling assembly? Probably a thing to do […]
This could be done, yes. It would also avoid having the error path below, since assembly would then not be called. OTOH, I like the simplicity of using `popcnt` to check this 😎
Reminds me that the BIOS ACM needs to be aligned to its size (or the next largest power of 2) and this is checked in C code already.
https://review.coreboot.org/c/coreboot/+/44880/11/src/security/intel/txt/get... PS11, Line 246: (1 << 11)
just use MTRR_PHYS_MASK_VALID.
Good catch. I reused the original code, and missed this.