ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 29:
(10 comments)
I had a few nits, nothing big.
https://review.coreboot.org/c/coreboot/+/33234/29/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/cpu/x86/mp_init.c@819 PS29, Line 819: uint32_t mseg; I bet some compiler at some point is going to complain that this is unused when CONFIG(STM) is not true.
Could you just move the use of it to the if (CONFIG(STM)) block at 837.
https://review.coreboot.org/c/coreboot/+/33234/29/src/cpu/x86/mp_init.c@1055 PS29, Line 1055: state->smm_save_state_size += Does line 1055 recreate line 1045 without the guard? I'm a bit confused here. Also, did you want the smm_save_state_size aligned up to the next 0x1000 granularity or just add the TXT_PROCESSOR_SMM_DESCRIPTOR rounded up to 0x1000?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 122: uint64_t resource_lo; I guess this is intel code we should change as little as possible, but I'm puzzled they didn't just initialize these variables to 0 in the declaration instead of these additional lines of assignment. Oh well.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 144: != record->mem.rwx_attributes) { This code may provide better justification for very long lines than I could ever make myself :-)
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 540: // ASSERT (false); I assume the ASSERT comes with Intel's code? Thanks for changing that. Intels' theory on BIOSes is that they should brick your machine if they get unhappy, which I"ve never understood.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 623: * Create 4G page table for STM. I just got lost. I see them setting PG_PS below. It looks like an array of 4M or 2M PTEs below.
So maybe this comment is Create a 4G page table for STM with 2M PTEs (x86_64) or 4M PTEs (x86_32)?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 637: pagetable_base += SIZE_4KB; I have to say, the SIZE_4KB constant here is not that helpful to me ... it obscures meaning. I'd almost rather just see 4096 or PTPSize, i.e. page table page size. I guess there's not much to do for it.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... File src/security/intel/stm/StmApi.h:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 44: uint32_t intel_64mode_supported : 1; // bitfield I think you can drop the bitfield comments; if people reading this don't know that already they don't know C ... Also, you could name the variable reserved to mbz, which is a pretty well known convention? Your call on that one.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 226: uint32_t gdt_base_hi_dword; // fdd0h : NO what does NO mean here?
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 338: #define END_OF_RESOURCES 0 so sometimes we have enum and sometimes define, I was wondering if there is a reason.