cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 33:
(22 comments)
To do: Still need to know how to create a config to get Jenkins to build STM Code
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@9 PS29, Line 9: combination
It can be done without breaking anything.
Paragraph changed to:
This update is a combination of all four of the patches so that the commit can be done without breaking parts of coreboot. This possible breakage is because of the cross-dependencies between the original separate patches would cause failure because of data structure changes.
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: resource
Why does it create a resource list? […]
The BIOS resource list defines the resources that the SMI Handler is allowed to access. This includes the SMM memory area where the SMI handler resides and other resources such as I/O devices. The STM uses the BIOS resource list to restrict the SMI handler's accesses.
The BIOS resource list is currently located in the same area as the SMI handler. This location is shown in the comment section before smm_load_module in smm_module_loader.c
For more information see: SMI Transfer Monitor (STM) User Guide, Intel Corp., August 2015, Rev 1.0, can be found at firmware.intel.com
https://review.coreboot.org/c/coreboot/+/33234/29//COMMIT_MSG@16 PS29, Line 16: tables
Where reside the page tables? […]
The STM page tables is a six page region located in the MSEG and are pointed to by the CR3 Offset field in the MSEG header. The initial page tables will identity map all memory between 0-4G. The STM starts in IA32e mode, which requires page table to exist at startup.
For more information see: SMI Transfer Monitor (STM) User Guide, Intel Corp., August 2015, Rev 1.0, can be found at firmware.intel.com
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 t […]
done
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. […]
fixed the code to what it should be, though what I have done is a conservative fudge. x86 architecture "requires" that, after the size of the elements of the smm save state are summed, the final size needs to be rounded up to the next power of 2. So, 0x1000 is a conservative fudge.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... PS29, Line 5: default
Some Intel platforms are missing, why?
Not intentional, if there is a way to ensure that all the relevant x86 platforms are included please let me know.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Kco... PS29, Line 6: (PLATFORM_USES_FSP2_0||PLATFORM_USES_FSP1_1||PLATFORM_USES_FSP1_0)
FSP1_0 support was dropped. […]
dropped FSP1_0 support and added CONFIG_SMM_TSEG requirement
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 […]
Changed the code to initialize the variables in the declaration.
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 :-)
Ack
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Smm... PS29, Line 483: retval
Never returned
Comment added in line 517 to indicate the out_of_resources return
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. […]
Thanks - it was a headache during debugging and it made no sense to brick the machine for an invalid input that really had no impact.
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. […]
Comment modified
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. […]
Fixed
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' […]
Fixed
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?
Writable - comment added to indicate this
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.
Not that I know of.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformResource.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 113: ACPI_BASE_ADDRESS
Platform specific code doesn't belong here
Modified this to get the value set by the kconfig menu
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 116: attribute__((weak))
No need for weak
Have to disagree. Some platform setup code (baytrail) provides this call get_pmbase while others (Kabylake) do not provide the equivelent. Having this function as weak allows the STM setup to use the call when available and, failing that, get it from the configuration
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 126: 255
Depends on the mmconf size
Found that the variable was not used (other than being set), so I deleted it.
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 157: Fix-up
What's fixed?
deleted comment
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... File src/security/intel/stm/StmPlatformSmm.c:
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 32: retval
No true, returns only a single retval
Comments fixed
https://review.coreboot.org/c/coreboot/+/33234/29/src/security/intel/stm/Stm... PS29, Line 81: mem_region_device_ro_init(&stm_region
No commented code
Fixed