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.

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: 3
Gerrit-Owner: Subrata Banik <subrata.banik@intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra@intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
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: Nathaniel L Desimone <nathaniel.l.desimone@intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com>
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: Tue, 03 Sep 2019 04:39:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment