Aaron Durbin 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 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
How does FSP use the region near _car_stack_start? For something else than a stack? Since active stack is already within _car_stack_start to _car_stack_end, FSP should not and cannot zero-fill that entire region either, so what does FSP do with it?
Why do we even pass _car_stack_start and DCACHE_BSP_STACK_SIZE to FSP, like done in setup_fsp_stack_frame? On entry to FSP, %esp is what FSP will have to cope with and it's the configurations fault if we have not reserved enough space for the stack.
This is further convoluted as configurations with FSP_USES_CB_STACK=n will place their FSP stack below _car_region_end. While allocation is dynamically checked, it really should have been accounted for in the linker scripts instead.
Since the logged errors are now just annoyance and not boot regression, I suggest we revisit CB:34882 with this new information and see what can be achieved here.
All great questions. I was searching around for FSP 2.1 spec, but it doesn't seem to be published. 2.1 has support for utilizing the stack that's already being used. Empirically it sure seems that FSP is using the whole thing and for uses other than a 'stack'. The interpretation of StackBase and StackSize in FSP is dependent on the spec version. They have different meanings.
It was way harder than it should have been to find: https://cdrdv2.intel.com/v1/dl/getContent/611786
Quoting the explanation which isn't clear to me how to bound the size properly:
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.
StackSize: For FSP implementations compliant to v2.0 of this specification, the temporary RAM size used to establish a stack and HOB heap. Consumed by the FspMemoryInit() API. For FSP implementations compliant to v2.1 of this specification, the temporary RAM size used to establish a HOB heap inside the FspMemoryInit() API. 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 StackSize name. Refer to the Integration Guide for the minimum required temporary RAM size. In the case of FSP v2.1, the Integration Guide shall also specify the minimum free stack space required at the point where the FSP API entrypoints are called.
I believe we should completely carve out a region for FSP in the 2.1 case that matches in size the explicit requirements in the integration guide is how I interpret this. What I haven't looked into is if actual stack recommendations are provided -- which is a completely separate thing that region for HOB heap we're providing here.
The CML integration guide refers to FSP 2.0 and a minimum stack size of 160KiB. However, we know CML is using 2.1 API so there's quite the conflict and no separation of heap vs stack size requirements.
So it appears CML is using 2.0 *but* with the addition of the stack behavior above. However, the hob size and FSP stack size requirements are not explicitly called out.