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.

View Change

To view, visit change 35165. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I943eff1225b976dc4440a6ca6d02ceea378319f8
Gerrit-Change-Number: 35165
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subrata.banik@intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan@google.com>
Gerrit-Reviewer: Kane Chen <kane.chen@intel.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com>
Gerrit-Reviewer: Meera Ravindranath <meera.ravindranath@intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak@chromium.org>
Gerrit-Reviewer: Usha P <usha.p@intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya@intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-Comment-Date: Fri, 30 Aug 2019 18:35:32 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment