Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3: Code-Review-2
Please submit documentation about the feature first. Mixing stack and heap isn't the way to go
Hi Philipp,
The FSP does not mix stack and heap. For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786). Here is the relevant paragraph:
StackBase - Pointer to the temporary RAM base address to be consumed inside FspMemoryInit() API.
For FSP implementations compliant to v2.0 of this specification, the temporary RAM is used to establish a stack and a HOB heap. For FSP implementations compliant to v2.1 of this specification, the temporary RAM is only used for a HOB heap.
Starting with v2.1 of this specification, FSP will run on top of the stack provided by the bootloader instead of establishing a separate stack. This allows the stack memory to be reused after FspMemoryInit() returns to the bootloader. To retain backwards compatibility with earlier versions of this specification, this parameter retains the StackBase name.
@Nate, the confusion is that, we understood that FSP and Coreboot will use the same stack hence we have given stackbase and size to FSP with same as CB is using. You could see CB on CML, ICL with single stack implementation is passing ~128KB stack size which might be huge for coreboot expectation and just to satisfy the FSP-M need. Till this part it was okay.
But the question comes, what FSP is storing at very end of stack (like stack base), is that some memory FSP is allocating for Hob?
we got to know about such behavior with one recent CL from Kyosti where he has marked stackbase + (64*4 bytes) as "DEADBEEF" and after FSP-M returns into coreboot, we check for integrity of this region to see if stackbase + (64*4 bytes) still has "DEADBEEF" or not. It appears to me that with CML recent FSP change to implement single stack requirement, we are overriding this reason. Hence i have submitted this CL to skip that check. But before that we might need to understand if FSP is using this very lower stack region intentionally if yes, then what for and where we could document such requirement to skip this integrity check.
Hope you understand the overall concern. For time being i would like hold any other CB:35233 and get the understanding clear.
@Subrata, I haven't been involved in the CML FSP implementation but usage of stack beyond 256KB surprises me as well. Could you maybe follow up with someone from the CML UEFI team in BA? There could be an implementation issue here.