Change in coreboot[master]: cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable ......................................................................
cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable
With FSP2.1 FSP_USES_CB_STACK likely to be enable. Impacted platforms are CML and ICL.
Don't need to run code logic to check the integrity of stack if FSP and coreboot both likely to make use of common stack. Notified by CONFIG_FSP_USES_CB_STACK, its expected to print "Smashed stack detected in romstage!" msg multiple time in that case.
This patch fixes CML/ICL platform printing "Smashed stack detected in romstage!" multiple times. This prints might be misleading.
TEST=Build and boot CML-Hatch
With this CL
No "Smashed stack detected in romstage!" msg in serial log.
Without this CL
"Smashed stack detected in romstage!" 64 times in serial log after FSP-M returns into coreboot.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/intel/car/romstage.c 1 file changed, 13 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/1
diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 547b121..78b2b8d 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -54,10 +54,19 @@ mainboard_romstage_entry();
/* Check the stack. */ - for (i = 0; i < num_guards; i++) { - if (stack_base[i] == stack_guard) - continue; - printk(BIOS_DEBUG, "Smashed stack detected in romstage!\n"); + /* + * Don't need to run below logic to check the integrity of stack + * if FSP and coreboot both likely to make use of common stack. + * Notified by CONFIG_FSP_USES_CB_STACK, its expected to print + * "Smashed stack detected in romstage!" msg many times in that case. + */ + if (!CONFIG(FSP_USES_CB_STACK)) { + for (i = 0; i < num_guards; i++) { + if (stack_base[i] == stack_guard) + continue; + printk(BIOS_DEBUG, "Smashed stack detected in" + "romstage!\n"); + } }
if (CONFIG(SMM_TSEG))
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG@12 PS1, Line 12: Don't need to run code logic to check the integrity of stack if FSP and : coreboot both likely to make use of common stack. Why? This is an assertion without an explanation. If FSP is utilizing the low part of the stack then 1. it should be documented as such and 2. we should then explain the motivation for this change in the commit description.
Furquan Shaikh 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG@12 PS1, Line 12: Don't need to run code logic to check the integrity of stack if FSP and : coreboot both likely to make use of common stack.
Why? This is an assertion without an explanation. […]
I agree with Aaron. We should properly document what the FSP utilization of the stack is. Also, do we really want to skip the stack integrity check? Can we not tell FSP about the stack - guard band so that it does not end up using the guard band?
Tim Wawrzynczak 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG@12 PS1, Line 12: Don't need to run code logic to check the integrity of stack if FSP and : coreboot both likely to make use of common stack.
I agree with Aaron. We should properly document what the FSP utilization of the stack is. […]
There still shouldn't be anything wrong with checking that the stack is a) used as an actual stack, and b) no unallocated memory is being touched.
Tim Wawrzynczak 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG@12 PS1, Line 12: Don't need to run code logic to check the integrity of stack if FSP and : coreboot both likely to make use of common stack.
There still shouldn't be anything wrong with checking that the stack is a) used as an actual stack, […]
If FSP needs a memory area to use as a "heap", can it require one from the bootloader instead of treating the stack that way?
Kyösti Mälkki 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:
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.
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:
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.
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/1/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/1/src/cpu/intel/car/romstage.... PS1, Line 63: if (!CONFIG(FSP_USES_CB_STACK)) { If we do need to go this route, I think we can make this a little cleaner with some helper functions:
static void setup_stack_guards(uintptr_t base, size_t num_guards); static void check_stack_guards(uintptr_t base, size_t num_guards);
We can conditionally bail early in those based on the Kconfig once the behavior is properly documented out FSP.
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:
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.
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:
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.
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.
And also, the stack size used on CML is 0x20000 == 128KiB. Which is also not even in alignment with the integration guide (both which version and size requirements). Yay.
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.
Tim Wawrzynczak 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:
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.
Sure sounds like it. If it literally requires 160KB but we're only giving it 128KB, what's going on? That's a huge discrepancy. Also why do they need a "heap" at all? If it's just for the HOBs, why can't a chunk of memory just get carved out and handed to FSP for filling them out? My understanding is that they're static data structures that are known ahead of time.
Furquan Shaikh 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:
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.
Sure sounds like it. If it literally requires 160KB but we're only giving it 128KB, what's going on? That's a huge discrepancy. Also why do they need a "heap" at all? If it's just for the HOBs, why can't a chunk of memory just get carved out and handed to FSP for filling them out? My understanding is that they're static data structures that are known ahead of time.
We basically need to do that exactly. With shared stack between FSP and bootloader, StackBase is really not stack base. Instead it is looking for a temporary space to put its HOBs. We will have to carve it out of the CAR and provide it to FSP. The way it is implemented in coreboot currently is not how FSP ends up actually using it.
Subrata Banik 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:
single stack sharing documentation is pending and already a CB ticket raised.
@Nate: can you please check if this is ready now ?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#2).
Change subject: cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable ......................................................................
cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable
With FSP2.1 FSP_USES_CB_STACK likely to be enable. Impacted platforms are CML (FSP2.0 + FSP_USES_CB_STACK feature enable) and ICL.
Don't need to run code logic to check the integrity of stack if FSP and coreboot both likely to make use of common stack. Notified by CONFIG_FSP_USES_CB_STACK, its expected to print "Smashed stack detected in romstage!" msg multiple time in that case.
This patch fixes CML/ICL platform printing "Smashed stack detected in romstage!" multiple times. This prints might be misleading.
TEST=Build and boot CML-Hatch
With this CL
No "Smashed stack detected in romstage!" msg in serial log.
Without this CL
"Smashed stack detected in romstage!" 64 times in serial log after FSP-M returns into coreboot.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/intel/car/romstage.c 1 file changed, 13 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/2
Tim Wawrzynczak 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 63: if (!CONFIG(FSP_USES_CB_STACK)) { So by this logic, the stack_guard should not be created when FSP_USES_CB_STACK is enabled either. But I would think we still want some kind of assurance that the stack hasn't been trashed. How can we make this work, Subrata, when FSP & coreboot "share" the stack? Can we get HOB size requirements so we can carve out a separate chunk of memory there exclusively for their use? How do we know that FSP-S hasn't "smashed" the stack for real and overwritten all the HOBs as well?
Angel Pons 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 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 67: printk(BIOS_DEBUG, "Smashed stack detected in" : "romstage!\n"); Line length limit is 96 characters, no need to split this printk message
Subrata Banik 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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 63: if (!CONFIG(FSP_USES_CB_STACK)) {
So by this logic, the stack_guard should not be created when FSP_USES_CB_STACK is enabled either. But I would think we still want some kind of assurance that the stack hasn't been trashed. How can we make this work, Subrata, when FSP & coreboot "share" the stack? Can we get HOB size requirements so we can carve out a separate chunk of memory there exclusively for their use?
Yes, on it to know the exact requirement and documenting the same.
How do we know that FSP-S hasn't "smashed" the stack for real and overwritten all the HOBs as well?
btw, its due to FSP-M call.
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 67: printk(BIOS_DEBUG, "Smashed stack detected in" : "romstage!\n");
Line length limit is 96 characters, no need to split this printk message
thanks noted. i thought its 80 :)
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#3).
Change subject: cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable ......................................................................
cpu/intel/car: Skip stack integrity check if FSP_USES_CB_STACK is enable
With FSP2.1 FSP_USES_CB_STACK likely to be enable. Impacted platforms are CML (FSP2.0 + FSP_USES_CB_STACK feature enable) and ICL.
Don't need to run code logic to check the integrity of stack if FSP and coreboot both likely to make use of common stack. Notified by CONFIG_FSP_USES_CB_STACK, its expected to print "Smashed stack detected in romstage!" msg multiple time in that case.
This patch fixes CML/ICL platform printing "Smashed stack detected in romstage!" multiple times. This prints might be misleading.
TEST=Build and boot CML-Hatch
With this CL
No "Smashed stack detected in romstage!" msg in serial log.
Without this CL
"Smashed stack detected in romstage!" 64 times in serial log after FSP-M returns into coreboot.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/intel/car/romstage.c 1 file changed, 12 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/3
Philipp Deppenwiese 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: Code-Review-2
Please submit documentation about the feature first. Mixing stack and heap isn't the way to go
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: 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.
Kyösti Mälkki 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:
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:
Whoever submitted the lines below did not see or understand the text of that paragraph in FSP specification. There is little clue of anything related to heap or temporary RAM.
config FSP_USES_CB_STACK bool default n help Enable support for fsp to use same stack as coreboot. This option allows fsp to continue using coreboot stack without reinitializing stack pointer. This feature is supported Icelake onwards.
config DCACHE_BSP_STACK_SIZE hex default 0x20000 if FSP_USES_CB_STACK default 0x4000 help The amount of anticipated stack usage in CAR by bootblock and other stages.
I think the previous comments were detailed enough to explain the situation; stack and heap must be carved out from CAR as two separate allocations. You are currently embedding this "temporary RAM" into the stack allocation, messing up the attempts to have usable stack guard checks.
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:
I think the previous comments were detailed enough to explain the situation; stack and heap must be carved out from CAR as two separate allocations. You are currently embedding this "temporary RAM" into the stack allocation, messing up the attempts to have usable stack guard checks.
That was not the intention from a FSP specification standpoint anyway. Purely for backwards compatibility reasons, the FSP's heap is confusingly called "StackBase" and "StackSize" even though it is totally not used for a stack starting with v2.1.
The intention is that during Pre-Memory phase StackBase, and StackSize would be a separate memory space only used for storing HOBs produced by the FSP, and the FSP MemoryInit() function call would run on top of the coreboot stack. The reason we made this change is because memory is at a premium before DRAM is available, and we wanted to share that memory with coreboot better than the previous design of just taking the memory and reserving it for FSP.
I must confess that I don't know a whole lot about coreboot's stack allocation mechanisms, but at a basic level it makes sense to me that if the FSP MemoryInit() runs on the same stack as coreboot then the coreboot stack will need to be larger. For a full memory training FSP uses approximately 128KB, which matches up with the new value for the DCACHE_BSP_STACK_SIZE kcfg option. That said, I have no idea if this is the best way to implement this and defer to others for that feedback.
Kyösti Mälkki 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:
I think the previous comments were detailed enough to explain the situation; stack and heap must be carved out from CAR as two separate allocations. You are currently embedding this "temporary RAM" into the stack allocation, messing up the attempts to have usable stack guard checks.
That was not the intention from a FSP specification standpoint anyway. Purely for backwards compatibility reasons, the FSP's heap is confusingly called "StackBase" and "StackSize" even though it is totally not used for a stack starting with v2.1.
CB:35233 untested
Furquan Shaikh 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:
I must confess that I don't know a whole lot about coreboot's stack allocation mechanisms, but at a basic level it makes sense to me that if the FSP MemoryInit() runs on the same stack as coreboot then the coreboot stack will need to be larger. For a full memory training FSP uses approximately 128KB, which matches up with the new value for the DCACHE_BSP_STACK_SIZE kcfg option. That said, I have no idea if this is the best way to implement this and defer to others for that feedback.
But the FSP integration guide for CML mentions 160KiB without shared stack i.e. with FSP2.0. Nothing is captured for FSP 2.1. What are the size requirements for a) stack b) hob temporary space for CML with the shared stack for FSP and bootloader?
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:
I must confess that I don't know a whole lot about coreboot's stack allocation mechanisms, but at a basic level it makes sense to me that if the FSP MemoryInit() runs on the same stack as coreboot then the coreboot stack will need to be larger. For a full memory training FSP uses approximately 128KB, which matches up with the new value for the DCACHE_BSP_STACK_SIZE kcfg option. That said, I have no idea if this is the best way to implement this and defer to others for that feedback.
But the FSP integration guide for CML mentions 160KiB without shared stack i.e. with FSP2.0. Nothing is captured for FSP 2.1. What are the size requirements for a) stack b) hob temporary space for CML with the shared stack for FSP and bootloader?
Sounds like the Comet Lake Integration Guide has gotten a little stale then. I'm out of office this week but I'll follow up next week.
Subrata Banik 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: 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.
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.
Furquan Shaikh 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:
@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.
I don't think FSP needs to be passed in stack information since it is sharing the stack with coreboot. The only thing that is required is that the stack be large enough to avoid overflow. This should be part of integration guide and implemented accordingly in coreboot.
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?
Like mentioned in one of the above comments, the name "StackBase" and "StackSize" which were retained for backward compatibility is the cause of confusion here. Though they have Stack in the name, the region being passed in is not really used for stack by FSP. Instead, FSP is expecting a temporary space for holding HOBs.
Subrata Banik 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:
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.
i have a meeting to discuss this. i will let you know
Subrata Banik 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:
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.
i have a meeting to discuss this. i will let you know
Updates from meeting and next steps:
Wrong assumptions due to naming conventions 1. m_arch->stack_base is NOT meant for stack base rather its more for HEAP BASE. 2. No UPD required to stack base FSP2.1 implementation (ESP to know stack base) 3. Stack_size is something required for HEAP SIZE and not stack size. 4. Right now on CML, heap size requirement is 0x11000, will call out in integration guide and subjected to change over platform if any then integration guide should have latest data.
ARs:
1. Update FSP integration guide to explicitly call out those naming conventions. Due to backward compatibility FSP can’t able to change those UPD names 2. Coreboot team to fix the existing code to remove those dependencies and provide a HEAP start and size using arch UPD
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#4).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it usage StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given DACHE_BSP_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP stack end +--------------+ _car_heap_end |heap | FSP heap base +--------------+ _car_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start FSP stack end +--------------+ _car_stack_end | | +--------------+ |stack_guard | FSP stack base +--------------+ _car_stack_start |vboot | +--------------+ _car_region_start
BUG=b:140268415
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 46 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
Patch Set 3: Code-Review-2
Please submit documentation about the feature first. Mixing stack and heap isn't the way to go
Can you please consider your vote as i have submitted CL to update FSP2.1 doc link CB:35238
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/1//COMMIT_MSG@12 PS1, Line 12: Don't need to run code logic to check the integrity of stack if FSP and : coreboot both likely to make use of common stack.
If FSP needs a memory area to use as a "heap", can it require one from the bootloader instead of tre […]
update the latest patchset which should resolve such question, i hope
https://review.coreboot.org/c/coreboot/+/35165/1/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/1/src/cpu/intel/car/romstage.... PS1, Line 63: if (!CONFIG(FSP_USES_CB_STACK)) {
If we do need to go this route, I think we can make this a little cleaner with some helper functions […]
we don't need this now and FSP won't touch stack area for heap allocation hence we won't see "Smash stack" issue now
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 63: if (!CONFIG(FSP_USES_CB_STACK)) {
So by this logic, the stack_guard should not be created when FSP_USES_CB_STACK is enabled either. […]
Ack
https://review.coreboot.org/c/coreboot/+/35165/2/src/cpu/intel/car/romstage.... PS2, Line 67: printk(BIOS_DEBUG, "Smashed stack detected in" : "romstage!\n");
thanks noted. […]
Ack
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 188: FSP stack base Do we need to call this out now? Since the FSP would just call get esp to get its stack frame, so the stack base for FSP would be esp as FSP-M is called.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@96 PS4, Line 96: CONFIG_C_ENVIRONMENT_BOOTBLOCK Are we dependent on this? I don't think we are.
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@102 PS4, Line 102: CONFIG_DCACHE_BSP_HEAP_SIZE Put 'FSP' in the name since it has only to do w/ FSP. Same for the symbols -- add 'fsp' in there.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG@27 PS4, Line 27: stack heap
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@96 PS4, Line 96: CONFIG_C_ENVIRONMENT_BOOTBLOCK
Are we dependent on this? I don't think we are.
Yeah, having this space should be independent.
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... File src/arch/x86/include/arch/symbols.h:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... PS4, Line 64: _car_heap_start - _car_heap_end Isn't this inverted?
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK It would be good to have a help text indicating what this is used for. Also, shouldn't this live in a FSP-related Kconfig file since it is very specific to FSP?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 164: setup_fsp_stack_frame Doing this in setup_fsp_stack_frame() makes it confusing. Can we call setup_fsp_stack_frame() only if !CONFIG(FSP_USES_CB_STACK) and setup the params for heap HOB if CONFIG(FSP_USES_CB_STACK) is true outside of this function?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 178: stack heap?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 189: vboot I understand the intent here to add vboot to provide a complete picture of the memory layout. But, car.ld actually contains more than just vboot. It would be good to either put in a comment or ... or some indication that there are more components within this space.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 196: arch_upd->StackSize = CONFIG_DCACHE_BSP_HEAP_SIZE; Please use symbol macros for size.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK
It would be good to have a help text indicating what this is used for. […]
maybe FSP_DCACHE_HEAP_SIZE ?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 188: FSP stack base
Do we need to call this out now? Since the FSP would just call get esp to get its stack frame, so th […]
Agreed, it shouldn't be called out specifically, as it's no longer separate.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#5).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it usage StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given DACHE_BSP_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_heap_end |heap | FSP heap base +--------------+ _car_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
BUG=b:140268415
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 56 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/5
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#6).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it usage StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given DACHE_BSP_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_fsp_heap_end |heap | FSP heap base +--------------+ _car_fsp_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
BUG=b:140268415
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 56 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG@27 PS4, Line 27: stack
heap
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@96 PS4, Line 96: CONFIG_C_ENVIRONMENT_BOOTBLOCK
Yeah, having this space should be independent.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@102 PS4, Line 102: CONFIG_DCACHE_BSP_HEAP_SIZE
Put 'FSP' in the name since it has only to do w/ FSP. Same for the symbols -- add 'fsp' in there.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... File src/arch/x86/include/arch/symbols.h:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... PS4, Line 64: _car_heap_start - _car_heap_end
Isn't this inverted?
my bad
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK
maybe FSP_DCACHE_HEAP_SIZE ?
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 164: setup_fsp_stack_frame
Doing this in setup_fsp_stack_frame() makes it confusing. […]
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 178: stack
heap?
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 188: FSP stack base
Agreed, it shouldn't be called out specifically, as it's no longer separate.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 189: vboot
I understand the intent here to add vboot to provide a complete picture of the memory layout. […]
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 196: arch_upd->StackSize = CONFIG_DCACHE_BSP_HEAP_SIZE;
Please use symbol macros for size.
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/6//COMMIT_MSG@20 PS6, Line 20: usage uses
https://review.coreboot.org/c/coreboot/+/35165/6/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/6/src/cpu/Kconfig@26 PS6, Line 26: FSP_DCACHE_HEAP_SIZE DCACHE_FSP_HEAP_SIZE(for coherency?)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/6//COMMIT_MSG@20 PS6, Line 20: usage
uses
Done
https://review.coreboot.org/c/coreboot/+/35165/6/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/6/src/cpu/Kconfig@26 PS6, Line 26: FSP_DCACHE_HEAP_SIZE
DCACHE_FSP_HEAP_SIZE(for coherency?)
check this https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig#28
Tim has request to make as FSP_DACHE_HEAP_SIZE
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK
Done
Added help text. i didn;t move into FSP kconfig file because we are using the Kconfig in car.ld which is in common x86. I'm not very familer with kconfig scoping
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#7).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it uses StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given DACHE_BSP_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_fsp_heap_end |heap | FSP heap base +--------------+ _car_fsp_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
BUG=b:140268415
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 60 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/7
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Aamir Bohra, Kane Chen, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#8).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it uses StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given FSP_DCACHE_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_fsp_heap_end |heap | FSP heap base +--------------+ _car_fsp_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
Set default value for FSP_DCACHE_HEAP_SIZE is 64KB for now. Platform should override the default value from soc/intel/<platform>/Kconfig if required.
(Right now CML & ICL, FSP requires at least heap size is 64KB and stack is 128KB)
BUG=b:140268415 TEST=Build and boot CML-Hatch and ICL.
With this CL No "Smashed stack detected in romstage" msg in serial log.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 62 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/8
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/8//COMMIT_MSG@16 PS8, Line 16: will same stack will share same stack?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Kane Chen, Aamir Bohra, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#9).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will share same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it uses StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given FSP_DCACHE_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_fsp_heap_end |heap | FSP heap base +--------------+ _car_fsp_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
Set default value for FSP_DCACHE_HEAP_SIZE is 64KB for now. Platform should override the default value from soc/intel/<platform>/Kconfig if required.
(Right now CML & ICL, FSP requires at least heap size is 64KB and stack is 128KB)
BUG=b:140268415 TEST=Build and boot CML-Hatch and ICL.
With this CL No "Smashed stack detected in romstage" msg in serial log.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 62 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/8//COMMIT_MSG@16 PS8, Line 16: will same stack
will share same stack?
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(2 comments)
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
Usually you would choose the minimum scope for the change you need, and avoid exposing stuff globally or architecturally without good arguments. A this time this all seems very much tied to FSP2.1 and the allocation you make can be made even within the function fsp_fill_common_arch_params() inside fsp2_0/memory_init.c.
This FSP_USES_CB_STACK now starts to sound like a poor design choice when it has 57% (64/112) increase in the CAR it reserves for romstage. Additionally, with your changes in car.ld, bootblock and verstage will see reserve from CAR for FSP to go from 0 KiB to 176 KiB (112 KiB in car_stack, 64 KiB in car_heap), none of which you need?
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK) Why define this for bootblock and verstage, when you only need it for romstage and it can change location between the stages?
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... PS9, Line 209: default 0x10000 Your previous comment Sep 03 09:05
4. Right now on CML, heap size requirement is 0x11000, will call out in integration guide and subjected to change over platform if any then integration guide should have latest data.
Another thing CB:35237, Sep 03 05:26
No Aaron/Tim. Stack requirement remain same ~128KB and HOB heap requirement is ~64KB. earlier we were actually passing ~128KB to stack and heap both using same arch->upd = stack_size. Now with next integration guide things will be call out correctly.
If we continue to allow 0x4000 for coreboot stack, like we did without FSP_USES_CB_STACK, FSP is currently left with 112 KiB for stack? So DCACHE_BSP_STACK_SIZE is still not synced with documentation?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK)
Why define this for bootblock and verstage, when you only need it for romstage and it can change loc […]
if reviewers say we need to guard with ENV_ROMSTAGE then we could do that.
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... PS9, Line 209: default 0x10000
Your previous comment Sep 03 09:05 […]
we had a sync with FSP team and finalize heap = 0x10000 and stack = 0x20000 for now on all FSP2.1 design and will get back on future soc if this requiremtn doesn't meet there.
The same will be documented. Not sure where you see the gap. commit msg has been updated with the expectation.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
I think Kyosti was suggesting one could use an array of uint8_t of a specified size (and appropriate alignment) in a C compilation unit to achieve the same allocation.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... PS9, Line 209: default 0x10000
we had a sync with FSP team and finalize heap = 0x10000 and stack = 0x20000 for now on all FSP2.1 design and will get back on future soc if this requiremtn doesn't meet there.
The same will be documented. Not sure where you see the gap. commit msg has been updated with the expectation.
128 KiB for stack and 64KiB for heap? That is massive. Where is the bloat coming from?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK)
if reviewers say we need to guard with ENV_ROMSTAGE then we could do that.
Yes, it should be guarded to be more correct. However, what matters here is the ordering. It's ok sitting here, but it can't be before _car_global_start. I believe this could also be achieved with an array:
static uint8_t fsp_heap[CONFIG_FSP_DCACHE_HEAP_SIZE] __aligned(sizeof(uint64_t);
The compiler will cull the allocation based on FSP_USES_CB_STACK Kconfig w/ garbage collection of the unused sections.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
I think Kyosti was suggesting one could use an array of uint8_t of a specified size (and appropriate alignment) in a C compilation unit to achieve the same allocation.
yes, this will work eventually but i'm afraid that someone might not able judge why we are declaring static variable with such big size :) where car.ld reserve might be worthy to explain with diagram.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK)
Yes, it should be guarded to be more correct. However, what matters here is the ordering. […]
yes, agree the ordering is important hence i have added the layout diagram to call out where FSP heap should exist. i hope that helps. Now as you said, will guard with ENV_ROMSTAGE to make it explicit. but i need to check in case of FSP-T usecase do FSP need to have heap region reversed as well (i hope that would be the case as well), right not chrome doesn't use FSP_t but there are some user for FSp-T as well.
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... PS9, Line 209: default 0x10000
we had a sync with FSP team and finalize heap = 0x10000 and stack = 0x20000 for now on all FSP2. […]
seems like this always the case with single stack assumption where FSP need to have bigger stack size and heap requirement is something that personally i have noticed with this issue but it was there and got resolved by our arch->StacKBase UPD wrongly assumed as stack base for FSP to provide additional 128KB for heap as well.
Now hopefully with CML integration guide documentation, FSP stack requirement ~128KB (mostly getting used in FSP-M training considering both debug and release mode) and heap requirement ~64KB (for all hobs)
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Julius Werner, Angel Pons, Kane Chen, Aamir Bohra, build bot (Jenkins), Furquan Shaikh, Meera Ravindranath, Usha P, Tim Wawrzynczak, Philipp Deppenwiese, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35165
to look at the new patch set (#10).
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
arch/x86: Reserve some CAR memory for FSP single stack requirement
From FSP2.1 onwards stack and heap requirement has changed (For documentation on this feature, please see page 25 of the FSP v2.1 specification (https://cdrdv2.intel.com/v1/dl/getContent/611786))
arch_upd->StackBase will refer as Heap Start for HOB and arch_upd->StackSize will be heap size.
As FSP and coreboot will share same stack hence FSP doesn't need dedicated UPD to pass stack start information.
But FSP might need to know where to keep HOB heap and for the same purpose it uses StackBase and StackSize UPDs (names are confusing!)
Proposed to reserve a small region above _car_global_end with platform given FSP_DCACHE_HEAP_SIZE for FSP to keep HOB heap.
+--------------+ _car_region_end | | FSP heap end +--------------+ _car_fsp_heap_end |heap | FSP heap base +--------------+ _car_fsp_heap_start/_car_global_end |car global | +--------------+ _car_global_start +--------------+ _car_relocatable_data_start +--------------+ _car_stack_end | | +--------------+ |stack_guard | +--------------+ _car_stack_start |vboot | +--------------+ +--------------+ _car_region_start
Set default value for FSP_DCACHE_HEAP_SIZE is 64KB for now. Platform should override the default value from soc/intel/<platform>/Kconfig if required.
(Right now CML & ICL, FSP requires at least heap size is 64KB and stack is 128KB)
BUG=b:140268415 TEST=Build and boot CML-Hatch and ICL.
With this CL No "Smashed stack detected in romstage" msg in serial log.
Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 4 files changed, 62 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/35165/10
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK)
yes, agree the ordering is important hence i have added the layout diagram to call out where FSP heap should exist. i hope that helps. Now as you said, will guard with ENV_ROMSTAGE to make it explicit. but i need to check in case of FSP-T usecase do FSP need to have heap region reversed as well (i hope that would be the case as well), right not chrome doesn't use FSP_t but there are some user for FSp-T as well.
If this reservation is needed for FSP-T then I think the FSP spec should be fixed. It has no mention of this in the spec, and I don't see why it would. If that is the case this allocation would have to be allocated and fixed in location (higher up this file). With my current understanding there's no reason to put this directly in the linker when we can put it in the .bss. If you are concerned about quality then just add a comment on top of variable.
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK)
If this reservation is needed for FSP-T then I think the FSP spec should be fixed. It has no mention of this in the spec, and I don't see why it would.
The FSP heap is not needed until FSP-M. You can see the list of input parameters for FSP-T at the top of https://github.com/tianocore/edk2/blob/master/IntelFsp2Pkg/FspSecCore/Ia32/F...
The FSP StackBase/StackSize (used to be stack + heap, now its only heap) parameters are part of the FSPM_UPD_COMMON structure in https://github.com/tianocore/edk2/blob/master/IntelFsp2Pkg/FspSecCore/Ia32/F...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 10:
Aaron, Furquan: Please make a decision between this and CB:35233. We have CB:35266 from amd/picasso on final approach too, landing on the same lines.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Abandoned
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 10:
Patch Set 10:
Aaron, Furquan: Please make a decision between this and CB:35233. We have CB:35266 from amd/picasso on final approach too, landing on the same lines.
Looks like Subrata abandoned this one.
-
Aamir Bohra (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Kane Chen (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nathaniel L Desimone (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)