I had a few nits, nothing big.
10 comments:
Patch Set #29, 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.
Patch Set #29, 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?
File src/security/intel/stm/SmmStm.c:
Patch Set #29, 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.
Patch Set #29, Line 144: != record->mem.rwx_attributes) {
This code may provide better justification for very long lines than I could ever make myself :-)
Patch Set #29, 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.
Patch Set #29, 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)?
Patch Set #29, 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.
File src/security/intel/stm/StmApi.h:
Patch Set #29, 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.
Patch Set #29, Line 226: uint32_t gdt_base_hi_dword; // fdd0h : NO
what does NO mean here?
Patch Set #29, Line 338: #define END_OF_RESOURCES 0
so sometimes we have enum and sometimes define, I was wondering if there is a reason.
To view, visit change 33234. To unsubscribe, or for help writing mail filters, visit settings.