Change in coreboot[master]: drivers/intel/fsp2_0: Allow including FSPT at specified offset

[Michał Żygowski (Code Review)]

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 <[e-mail]>
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)

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newchange

[Angel Pons (Code Review)]

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



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jul 2020 09:45:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Michał Żygowski (Code Review)]

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 (Code Review)]

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)?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jul 2020 12:09:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Michał Żygowski (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jul 2020 13:43:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <[e-mail]>
Comment-In-Reply-To: Michał Żygowski <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

[Michał Żygowski (Code Review)]

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 (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jul 2020 14:02:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <[e-mail]>
Comment-In-Reply-To: Michał Żygowski <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

[Michał Żygowski (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jul 2020 15:00:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <[e-mail]>
Comment-In-Reply-To: Michał Żygowski <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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.

[Paul Menzel (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Michał Żygowski <[e-mail]>
Gerrit-Comment-Date: Mon, 06 Sep 2021 21:15:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[S] Change in coreboot[master]: drivers/intel/fsp2_0: Allow including FSPT at specified offset

[Martin L Roth (Code Review)]

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.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43398?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I997c7465fd7ac56633c3e7e3fa5b95384dcf5ad2
Gerrit-Change-Number: 43398
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Martin L Roth <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: abandon