Change in coreboot[master]: drivers/intel/fsp2_0: Allow including FSPT at specified offset
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
drivers/intel/fsp2_0: Allow including FSPT at specified offset
FSPT is executed by assembly code and is not being automatically relocated, thus it must be at specified offset. Add options to specify FSPT location in CBFS and user option to include FSPT.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2 --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/Makefile.inc 2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/43398/1
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 3caa04a..5d27f32 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -89,6 +89,14 @@ help The path and filename of the Intel FSP-T binary for this platform.
+config FSP_T_CBFS_LOCATION + hex "Intel FSP-T Binary location in CBFS" + default 0xfffd1000 + help + Specify the location of FSP-T binary. FSP-T is executed early by + assembly code and is not automatically relocated. The location must + match the binary base address in FSP-T header. + config FSP_M_FILE string "Intel FSP-M (memory init) binary path and filename" if !FSP_USE_REPO depends on ADD_FSP_BINARIES @@ -104,7 +112,7 @@ The path and filename of the Intel FSP-S binary for this platform.
config FSP_CAR - bool + bool "Use FSP to setup temporary memory" default n help Use FSP APIs to initialize & Tear Down the Cache-As-Ram diff --git a/src/drivers/intel/fsp2_0/Makefile.inc b/src/drivers/intel/fsp2_0/Makefile.inc index 278036a..a1758ea 100644 --- a/src/drivers/intel/fsp2_0/Makefile.inc +++ b/src/drivers/intel/fsp2_0/Makefile.inc @@ -42,8 +42,9 @@ cbfs-files-$(CONFIG_FSP_CAR) += $(FSP_T_CBFS) $(FSP_T_CBFS)-file := $(call strip_quotes,$(CONFIG_FSP_T_FILE)) $(FSP_T_CBFS)-type := fsp +$(FSP_T_CBFS)-options := -b $(CONFIG_FSP_T_CBFS_LOCATION) ifeq ($(CONFIG_FSP_T_XIP),y) -$(FSP_T_CBFS)-options := --xip $(TXTIBB) $(BTGIBB) +$(FSP_T_CBFS)-options := --xip $(TXTIBB) $(BTGIBB) -b $(CONFIG_FSP_T_CBFS_LOCATION) endif
cbfs-files-$(CONFIG_ADD_FSP_BINARIES) += $(FSP_M_CBFS)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@7 PS1, Line 7: FSPT FSP-T
https://review.coreboot.org/c/coreboot/+/43398/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/43398/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 114: FSP_CAR I think this symbol isn't meant to have a prompt. See `USE_APOLLOLAKE_FSP_CAR`, `USE_CANNONLAKE_FSP_CAR` symbols
https://review.coreboot.org/c/coreboot/+/43398/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 115: setup set up
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/43398/1/src/drivers/intel/fsp2_0/Kc... PS1, Line 114: FSP_CAR
I think this symbol isn't meant to have a prompt. […]
Okay, I missed these ones
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT. I don't follow... is this a `cbfstool` issue? i.e. `cbfstool` doesn't relocate it even if it should?
Or is there code in the queue (or even upstream) that allows to load FSP-T into SRAM or CAR (without relocation)?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
I don't follow... is this a `cbfstool` issue? i.e. `cbfstool` doesn't […]
The assmebly code that is supossed to locate and jump to FSP-T does correctly its job, but it assumes that FSP-T is located at correct offset. cbfstool seems to put FSP-T sa an normal file at the first available free space that is enough for the binary, which is obviously wrong. Does cbfstool have a logic to place FSP binary at correct offset?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
The assmebly code that is supossed to locate and jump to FSP-T does correctly its job, but it assume […]
What is wrong with the first free location? Is some alignment not fulfilled? `cbfstool` should have an option for that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
What is wrong with the first free location? Is some alignment not fulfilled? […]
The FSP-T binary is not relocatable, it seems. So, it needs to be placed at a specific address in memory. Placing it in CAR is probably not going to work, if FSP-T reinitializes CAR and erases its own code while doing so.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
The FSP-T binary is not relocatable, it seems. So, it needs to be placed at a specific address in memory.
Documented where? If it's not documented, we should. It seems like another violation of the spec.
Placing it in CAR is probably not going to work, if FSP-T reinitializes CAR and erases its own code while doing so.
We already know that FSP-T is not always about setting up CAR. So it's actually possible that Intel would implement such folly.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
The FSP-T binary is not relocatable, it seems. […]
Is is written in integration guide that the binary must be placed at the address specified in FSP binary header, it is valid for all FSP binaries, i.e. FSP-T, FSP-M and FSP-S. IIRC all three have the relocation tables which are meant to runtime relocate the binary. But it isn't implemented in assembly that loads and executes FSP-T in coreboot. That is why first free location is bad.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
Is is written in integration guide that the binary must be placed at the address specified in FSP bi […]
To avoid misunderstandings: When I said "relocate" I meant the rebase process as described in the FSP spec.
`cbfstool` does this rebasing. And should do so if `xip` is specified or an address is given (cf. cbfstool_convert_fsp(), in `cbfstool.c`). If that doesn't work, I think there is a bug to fix. Or maybe we talk about the non-XIP case, then my reasoning is meaningless.
So, as we are reviewing backwards here (my question on CB:43396 is unanswered), which path in the Makefile are we talking about? XIP or non-XIP case? Were both paths tested?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
To avoid misunderstandings: When I said "relocate" I meant the rebase […]
Sorry, answered right now. So we are dealing with the XIP case.
Yes, I also meant FSP rebase process by "relocate".
Didn't dig deep into cbfstool and didn't know cbfstool_convert_fsp exist. So it definitely didn't work for me until I have specified the base address in the Makefile. I will look into the cbfstool to search for the root cause.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43398/1//COMMIT_MSG@11 PS1, Line 11: specify FSPT location in CBFS and user option to include FSPT.
Sorry, answered right now. So we are dealing with the XIP case. […]
I got confused with the meaning of "relocatable", and thought it was not about the "rebase" stuff. Sorry if this caused confusion.
Attention is currently required from: Michał Żygowski. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43398 )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1: Is this still needed?
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43398?usp=email )
Change subject: drivers/intel/fsp2_0: Allow including FSPT at specified offset ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Martin L Roth (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)