Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80581?usp=email )
Change subject: Kconfig: Default to no romstage on XIP early stages
......................................................................
Kconfig: Default to no romstage on XIP early stages
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Change-Id: I38d1c5d2d432253836f419275e0b0d994a9c2b1f
---
M src/Kconfig
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/80581/1
diff --git a/src/Kconfig b/src/Kconfig
index 2afb9db..725af6b 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -1539,4 +1539,5 @@
default y
config SEPARATE_ROMSTAGE
+ default n if !NO_XIP_EARLY_STAGES
default y
--
To view, visit https://review.coreboot.org/c/coreboot/+/80581?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I38d1c5d2d432253836f419275e0b0d994a9c2b1f
Gerrit-Change-Number: 80581
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Andrey Petrov, Frans Hendriks, Maximilian Brune, Paul Menzel, Yu-Ping Wu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63419?usp=email )
Change subject: Don't build a separate romstage by default on most x86 targets
......................................................................
Patch Set 55:
(1 comment)
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/63419/comment/b0693966_42b92897 :
PS55, Line 248: default y
> Could you separate the patch that removes the default here? Then I can redefine that value inside an SOC Kconfig without waiting until the discussion for this patch is over.
CB:80580
--
To view, visit https://review.coreboot.org/c/coreboot/+/63419?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4c53b6fa7a3e66415c5b1c539918a6c1cd8defa1
Gerrit-Change-Number: 63419
Gerrit-PatchSet: 55
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Sun, 18 Feb 2024 13:07:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-MessageType: comment
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80580?usp=email )
Change subject: Kconfig: Allow other Kconfig files to override the default
......................................................................
Kconfig: Allow other Kconfig files to override the default
This also sets a good default in arch and vboot to have a separate
romstage when it makes sense.
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Change-Id: I09ab5f8c79917bf93c9d5c9dfd157c652478b186
---
M src/Kconfig
M src/arch/x86/Kconfig
M src/security/vboot/Kconfig
3 files changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/80580/1
diff --git a/src/Kconfig b/src/Kconfig
index 2bcc3ce..2afb9db 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -245,7 +245,6 @@
config SEPARATE_ROMSTAGE
bool "Build a separate romstage"
- default y
help
Build a separate romstage that is loaded by bootblock. With this
option disabled the romstage sources are linked inside the bootblock
@@ -1538,3 +1537,6 @@
bool
default n if RAMPAYLOAD
default y
+
+config SEPARATE_ROMSTAGE
+ default y
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index 0c11653..63f71e5 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -251,8 +251,9 @@
bool "Always load fallback"
config BOOTBLOCK_NORMAL
- select CONFIGURABLE_CBFS_PREFIX
bool "Switch to normal if CMOS says so"
+ select CONFIGURABLE_CBFS_PREFIX
+ select SEPARATE_ROMSTAGE
endchoice
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig
index 56e94d5..a739852 100644
--- a/src/security/vboot/Kconfig
+++ b/src/security/vboot/Kconfig
@@ -90,7 +90,7 @@
config VBOOT_STARTS_IN_BOOTBLOCK
bool
default n
- depends on SEPARATE_ROMSTAGE
+ select SEPARATE_ROMSTAGE
help
Firmware verification happens during the end of or right after the
bootblock. This implies that a static VBOOT2_WORK() buffer must be
--
To view, visit https://review.coreboot.org/c/coreboot/+/80580?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I09ab5f8c79917bf93c9d5c9dfd157c652478b186
Gerrit-Change-Number: 80580
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange
Attention is currently required from: Andrey Petrov, Arthur Heymans, Frans Hendriks, Paul Menzel, Yu-Ping Wu.
Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63419?usp=email )
Change subject: Don't build a separate romstage by default on most x86 targets
......................................................................
Patch Set 55:
(1 comment)
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/63419/comment/29184071_b9614350 :
PS55, Line 248: default y
Could you separate the patch that removes the default here? Then I can redefine that value inside an SOC Kconfig without waiting until the discussion for this patch is over.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63419?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4c53b6fa7a3e66415c5b1c539918a6c1cd8defa1
Gerrit-Change-Number: 63419
Gerrit-PatchSet: 55
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Sun, 18 Feb 2024 12:47:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Christian Walter, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80578?usp=email )
Change subject: soc/intel/xeon_sp: Add support for is_ioat_iio_stack_res
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
File src/soc/intel/xeon_sp/include/soc/util.h:
https://review.coreboot.org/c/coreboot/+/80578/comment/fde17c08_12a7eec6 :
PS1, Line 28: bool is_ioat_iio_stack_res(const STACK_RES *res);
If you add a Kconfig and a static inline the compiler can likely optimize out the SKX code that does not need it?
File src/soc/intel/xeon_sp/uncore_acpi.c:
https://review.coreboot.org/c/coreboot/+/80578/comment/58e70548_87ccbccc :
PS1, Line 338: #if CONFIG(SOC_INTEL_SAPPHIRERAPIDS_SP) || CONFIG(SOC_INTEL_COOPERLAKE_SP)
drop
https://review.coreboot.org/c/coreboot/+/80578/comment/45ccd06d_c12ef8e8 :
PS1, Line 492: #if CONFIG(SOC_INTEL_SAPPHIRERAPIDS_SP) || CONFIG(SOC_INTEL_COOPERLAKE_SP)
drop
--
To view, visit https://review.coreboot.org/c/coreboot/+/80578?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I376928ad89b68b294734000678dad6f070d3c97d
Gerrit-Change-Number: 80578
Gerrit-PatchSet: 1
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Sun, 18 Feb 2024 11:17:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Andrey Petrov, Arthur Heymans, Christian Walter, Johnny Lin, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu.
Hello Andrey Petrov, Christian Walter, Johnny Lin, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/80579?usp=email
to look at the new patch set (#3).
Change subject: drivers/intel/fsp: Add a workaround for multi socket xeon-sp
......................................................................
drivers/intel/fsp: Add a workaround for multi socket xeon-sp
Starting with Intel CPX there is a bug in there reference code during
the Pipe init. This code synchronises the CAR between sockets in FSP-M.
This code implicitly assumes that the top of FSP heap is FspStackBase +
FspStackSize and the bottom is the bottom of CAR.
Work around this issue by making that implicit assumption done in FSP
explicit in the coreboot linker script and allocation.
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
M src/soc/intel/xeon_sp/cpx/Kconfig
M src/soc/intel/xeon_sp/spr/Kconfig
5 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/80579/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/80579?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
Gerrit-Change-Number: 80579
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Andrey Petrov, Chen, Gang C, David Hendricks, Johnny Lin, Lean Sheng Tan, Martin L Roth, Nico Huber, Patrick Rudolph, Paul Menzel, Ronak Kanabar, Shuo Liu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80328?usp=email )
Change subject: drivers/intel/fsp2_0: Add FSP_ALLOCATES_TEMP_RAM
......................................................................
Patch Set 10:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80328/comment/aa1deafe_d2acb806 :
PS10, Line 11: This is required by some FSP implementations
: of Xeon SP multi-socket platforms.
Please change this to "This works around a flaw in FSP for Xeon SP multi-socket platforms". Also reflect in the Kconfig naming that this is a workaround.
Patchset:
PS10:
I still think this is not the right approach. Explicit allocation is desirable or at least some asserts that the default values are not overlapping with anything else used by coreboot. Try CB:80579 ?
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/80328/comment/f1ff0d78_7d10fded :
PS10, Line 46: FSP allocates temporary ram in its reserved memory ranges
: (covered in FSP_M_RC_HEAP_SIZE) instead of requesting coreboot to
: do the allocation. This is required by some FSP implementations of
: Xeon SP multi-socket platforms.
:
This is misleading: You just use the default value. FSP isn't doing allocating anything.
https://review.coreboot.org/c/coreboot/+/80328/comment/05e06446_ca618d75 :
PS10, Line 53: default 0x0 if FSP_ALLOCATES_TEMP_RAM
I don't think the option
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80328/comment/29460d42_253f1379 :
PS10, Line 178: Bootloader should not override these values otherwise multi-socket
: * platform booting will be broken.
That is simply not true. FSP makes some wrong assumption about where the heap is. Leaving it default is not the only way of solving this.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80328?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I17a73e4f627c91808ed17c712b72937662ae9293
Gerrit-Change-Number: 80328
Gerrit-PatchSet: 10
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Comment-Date: Sun, 18 Feb 2024 10:54:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Andrey Petrov, Arthur Heymans, Christian Walter, Johnny Lin, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu.
Hello Andrey Petrov, Christian Walter, Johnny Lin, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/80579?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified-1 by build bot (Jenkins)
Change subject: drivers/intel/fsp: Add a workaround for multi socket xeon-sp
......................................................................
drivers/intel/fsp: Add a workaround for multi socket xeon-sp
Starting with Intel CPX there is a bug in there reference code during
the Pipe init. This code synchronises the CAR between sockets in FSP-M.
This code implicitly assumes that the top of FSP heap is FspStackBase +
FspStackSize and the bottom is the bottom of CAR.
Work around this issue by making that implicit assumption done in FSP
explicit in the coreboot linker script and allocation.
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
M src/soc/intel/xeon_sp/cpx/Kconfig
M src/soc/intel/xeon_sp/spr/Kconfig
5 files changed, 32 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/80579/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/80579?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
Gerrit-Change-Number: 80579
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-MessageType: newpatchset
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80579?usp=email )
Change subject: drivers/intel/fsp: Add a workaround for multi socket xeon-sp
......................................................................
drivers/intel/fsp: Add a workaround for multi socket xeon-sp
Starting with Intel CPX there is a bug in there reference code during
the Pipe init. This code synchronises the CAR between sockets in FSP-M.
This code implicitly assumes that the top of FSP heap is FspStackBase +
FspStackSize and the bottom is the bottom of CAR.
Work around this issue by making that implicit assumption done in FSP
explicit in the coreboot linker script and allocation.
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
M src/soc/intel/xeon_sp/cpx/Kconfig
M src/soc/intel/xeon_sp/spr/Kconfig
5 files changed, 32 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/80579/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld
index 17e6eea..3a96d46 100644
--- a/src/arch/x86/car.ld
+++ b/src/arch/x86/car.ld
@@ -9,6 +9,10 @@
.car.data . (NOLOAD) : {
_car_region_start = . ;
. += CONFIG_FSP_M_RC_HEAP_SIZE;
+#if CONFIG(FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND)
+ . += CONFIG_FSP_TEMP_RAM_SIZE;
+#endif
+
#if CONFIG(PAGING_IN_CACHE_AS_RAM)
/* Page table pre-allocation. CONFIG_DCACHE_RAM_BASE should be 4KiB
* aligned when using this option. */
@@ -110,6 +114,14 @@
. += CONFIG_FSP_M_RC_HEAP_SIZE;
}
+#if CONFIG(FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND)
+_fspm_heap = .;
+.car.fspm_heap . (NOLOAD) : {
+ . += CONFIG_FSP_TEMP_RAM_SIZE;
+}
+_efspm_heap = .;
+#endif
+
. = _car_region_end;
.car.mrc_var . (NOLOAD) : {
. += CONFIG_DCACHE_RAM_MRC_VAR_SIZE;
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig
index e27249f..98ff3be 100644
--- a/src/drivers/intel/fsp2_0/Kconfig
+++ b/src/drivers/intel/fsp2_0/Kconfig
@@ -221,6 +221,18 @@
without reinitializing stack pointer. This feature is
supported Icelake onwards.
+config FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND
+ bool
+ help
+ Starting with Intel CPX there is a bug in there reference code during
+ the Pipe init. This code synchronises the CAR between sockets in FSP-M.
+ This code implicitly assumes that the top of FSP heap is FspStackBase +
+ FspStackSize and the bottom is the bottom of CAR. This causes problem if
+ the stack is in between there. Select this to have an explicit handling
+ of the FSP StackBase to work around this issue. This is needed on multi-
+ socket xeon-sp systems. This will place the FSP heap right above the
+ FSP-M RC heap.
+
config FSP_TEMP_RAM_SIZE
hex
help
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index f5de5c3..b128d66 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -170,7 +170,12 @@
[FSP_BOOT_IN_RECOVERY_MODE] = "boot in recovery mode",
};
- if (CONFIG(FSP_USES_CB_STACK) || !ENV_CACHE_AS_RAM) {
+ if (CONFIG(FSP_USES_CB_STACK) && ENV_RAMINIT && CONFIG(FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND)) {
+ extern char *_fspm_heap;
+ extern char *_efspm_heap;
+ arch_upd->StackBase = (uintptr_t)_fspm_heap;
+ arch_upd->StackSize = (size_t)(_efspm_heap - _fspm_heap);
+ } else if (CONFIG(FSP_USES_CB_STACK) || !ENV_CACHE_AS_RAM) {
arch_upd->StackBase = (uintptr_t)temp_ram;
arch_upd->StackSize = sizeof(temp_ram);
} else if (setup_fsp_stack_frame(arch_upd, memmap)) {
diff --git a/src/soc/intel/xeon_sp/cpx/Kconfig b/src/soc/intel/xeon_sp/cpx/Kconfig
index ac166c3..83a1af7 100644
--- a/src/soc/intel/xeon_sp/cpx/Kconfig
+++ b/src/soc/intel/xeon_sp/cpx/Kconfig
@@ -7,6 +7,7 @@
select CACHE_MRC_SETTINGS
select NO_FSP_TEMP_RAM_EXIT
select HAVE_INTEL_FSP_REPO
+ select FSP_SPEC_VIOLATION_HEAP_WORKAROUND
help
Intel Cooper Lake-SP support
diff --git a/src/soc/intel/xeon_sp/spr/Kconfig b/src/soc/intel/xeon_sp/spr/Kconfig
index bb88bec..6046f96 100644
--- a/src/soc/intel/xeon_sp/spr/Kconfig
+++ b/src/soc/intel/xeon_sp/spr/Kconfig
@@ -13,6 +13,7 @@
select SOC_INTEL_CSE_SERVER_SKU
select XEON_SP_COMMON_BASE
select HAVE_IOAT_DOMAINS
+ select FSP_SPEC_VIOLATION_HEAP_WORKAROUND
help
Intel Sapphire Rapids-SP support
--
To view, visit https://review.coreboot.org/c/coreboot/+/80579?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I38a4f4b7470556e528a1672044c31f8bd92887d4
Gerrit-Change-Number: 80579
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newchange