Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
The FSP info header was using size_t instead of the native data type used to compile the binary. This caused the FSP-M to not being recognized by the loader.
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c 6 files changed, 92 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/1
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 03b9c2b..4e88795 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -1,5 +1,18 @@ # SPDX-License-Identifier: GPL-2.0-only
+config PLATFORM_USES_FSP2_X86_32 + bool + default y + help + The FSP 2.0 build runs in x86_32 protected mode. + +config PLATFORM_USES_FSP2_X86_64 + bool + depends on !PLATFORM_USES_FSP2_X86_32 + default y + help + The FSP 2.0 build runs in x86_64 long mode. + config PLATFORM_USES_FSP2_0 bool default n diff --git a/src/drivers/intel/fsp2_0/include/fsp/info_header.h b/src/drivers/intel/fsp2_0/include/fsp/info_header.h index f237a37..7f09cc6 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h +++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h @@ -4,6 +4,7 @@ #define _FSP2_0_INFO_HEADER_H_
#include <types.h> +#include <commonlib/bsd/compiler.h>
#define FSP_HDR_OFFSET 0x94 #if CONFIG(PLATFORM_USES_FSP2_2) @@ -16,24 +17,46 @@ #define FSP_HDR_ATTRIB_FSPM 2 #define FSP_HDR_ATTRIB_FSPS 3
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct fsp_header { uint32_t fsp_revision; - size_t image_size; - uintptr_t image_base; + uint32_t image_size; + uint32_t image_base; uint16_t image_attribute; uint8_t spec_version; uint16_t component_attribute; - size_t cfg_region_offset; - size_t cfg_region_size; - size_t temp_ram_init_entry; - size_t temp_ram_exit_entry; - size_t notify_phase_entry_offset; - size_t memory_init_entry_offset; - size_t silicon_init_entry_offset; - size_t multi_phase_si_init_entry_offset; + uint32_t cfg_region_offset; + uint32_t cfg_region_size; + uint32_t temp_ram_init_entry; + uint32_t temp_ram_exit_entry; + uint32_t notify_phase_entry_offset; + uint32_t memory_init_entry_offset; + uint32_t silicon_init_entry_offset; + uint32_t multi_phase_si_init_entry_offset; char image_id[sizeof(uint64_t) + 1]; uint8_t revision; -}; +} __packed; +#else +struct fsp_header { + uint32_t fsp_revision; + uint64_t image_size; + uint64_t image_base; + uint16_t image_attribute; + uint8_t spec_version; + uint16_t component_attribute; + uint64_t cfg_region_offset; + uint64_t cfg_region_size; + uint64_t temp_ram_init_entry; + uint64_t temp_ram_exit_entry; + uint64_t notify_phase_entry_offset; + uint64_t memory_init_entry_offset; + uint64_t silicon_init_entry_offset; + uint64_t multi_phase_si_init_entry_offset; + char image_id[sizeof(uint64_t) + 1]; + uint8_t revision; +} __packed; +#endif +
enum cb_err fsp_identify(struct fsp_header *hdr, const void *fsp_blob);
diff --git a/src/drivers/intel/fsp2_0/include/fsp/upd.h b/src/drivers/intel/fsp2_0/include/fsp/upd.h index 979cff3..370fcd3 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/upd.h +++ b/src/drivers/intel/fsp2_0/include/fsp/upd.h @@ -21,6 +21,7 @@ uint8_t Reserved[23]; } __packed;
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct FSPM_ARCH_UPD { /// /// Revision of the structure. For FSP v2.0 value is 1. @@ -31,12 +32,12 @@ /// Pointer to the non-volatile storage (NVS) data buffer. /// If it is NULL it indicates the NVS data is not available. /// - void *NvsBufferPtr; + uint32_t NvsBufferPtr; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - void *StackBase; + uint32_t StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. @@ -53,7 +54,40 @@ uint32_t BootMode; uint8_t Reserved1[8]; } __packed; - +#else +struct FSPM_ARCH_UPD { + /// + /// Revision of the structure. For FSP v2.0 value is 1. + /// + uint8_t Revision; + uint8_t Reserved[3]; + /// + /// Pointer to the non-volatile storage (NVS) data buffer. + /// If it is NULL it indicates the NVS data is not available. + /// + uint64_t NvsBufferPtr; + /// + /// Pointer to the temporary stack base address to be + /// consumed inside FspMemoryInit() API. + /// + uint64_t StackBase; + /// + /// Temporary stack size to be consumed inside + /// FspMemoryInit() API. + /// + uint32_t StackSize; + /// + /// Size of memory to be reserved by FSP below "top + /// of low usable memory" for bootloader usage. + /// + uint32_t BootLoaderTolumSize; + /// + /// Current boot mode. + /// + uint32_t BootMode; + uint8_t Reserved1[8]; +} __packed; +#endif struct FSPS_ARCH_UPD { /// /// Revision of the structure. For FSP v2.2 value is 1. diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 27e34fe..d62888e 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -101,7 +101,7 @@ return;
/* MRC cache found */ - arch_upd->NvsBufferPtr = data; + arch_upd->NvsBufferPtr = (uintptr_t)data;
printk(BIOS_SPEW, "MRC cache found, size %zx\n", mrc_size); } @@ -142,7 +142,7 @@ stack_end) != CB_SUCCESS) return CB_ERR;
- arch_upd->StackBase = (void *)stack_begin; + arch_upd->StackBase = stack_begin; return CB_SUCCESS; }
@@ -237,7 +237,7 @@
fsp_version = fsp_memory_settings_version(hdr);
- upd = (FSPM_UPD *)(hdr->cfg_region_offset + hdr->image_base); + upd = (FSPM_UPD *)(uintptr_t)(hdr->cfg_region_offset + hdr->image_base);
if (upd->FspUpdHeader.Signature != FSPM_UPD_SIGNATURE) die_with_post_code(POST_INVALID_VENDOR_BINARY, @@ -291,7 +291,7 @@ post_code(POST_MEM_PREINIT_PREP_END);
/* Call FspMemoryInit */ - fsp_raminit = (void *)(hdr->image_base + hdr->memory_init_entry_offset); + fsp_raminit = (void *)(uintptr_t)(hdr->image_base + hdr->memory_init_entry_offset); fsp_debug_before_memory_init(fsp_raminit, upd, &fspm_upd);
post_code(POST_FSP_MEMORY_INIT); diff --git a/src/drivers/intel/fsp2_0/notify.c b/src/drivers/intel/fsp2_0/notify.c index ee04630..73de605 100644 --- a/src/drivers/intel/fsp2_0/notify.c +++ b/src/drivers/intel/fsp2_0/notify.c @@ -15,7 +15,7 @@ if (!fsps_hdr.notify_phase_entry_offset) die("Notify_phase_entry_offset is zero!\n");
- fspnotify = (void *) (fsps_hdr.image_base + + fspnotify = (void *) (uintptr_t)(fsps_hdr.image_base + fsps_hdr.notify_phase_entry_offset); fsp_before_debug_notify(fspnotify, ¬ify_params);
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index 0b6540e..d180060 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -85,7 +85,7 @@ struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number;
- supd = (FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base); + supd = (FSPS_UPD *) (uintptr_t)(hdr->cfg_region_offset + hdr->image_base);
if (supd->FspUpdHeader.Signature != FSPS_UPD_SIGNATURE) die_with_post_code(POST_INVALID_VENDOR_BINARY, @@ -111,7 +111,7 @@ logo_entry = soc_load_logo(upd);
/* Call SiliconInit */ - silicon_init = (void *) (hdr->image_base + + silicon_init = (void *) (uintptr_t)(hdr->image_base + hdr->silicon_init_entry_offset); fsp_debug_before_silicon_init(silicon_init, supd, upd);
@@ -139,7 +139,7 @@ return;
/* Call MultiPhaseSiInit */ - multi_phase_si_init = (void *) (hdr->image_base + + multi_phase_si_init = (void *) (uintptr_t)(hdr->image_base + hdr->multi_phase_si_init_entry_offset);
/* Implementing multi_phase_si_init() is optional as per FSP 2.2 spec */
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#5).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/soc/amd/picasso/Kconfig M src/soc/intel/alderlake/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/elkhartlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/quark/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/xeon_sp/Kconfig M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 21 files changed, 234 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 5:
Please review
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 36: default y Since you defaulted this to y, and still selected X86_32 in many soc Kconfig files, are you intending to flip the polarity down the road? That is, to make 64 the default and 32 the exception?
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... PS5, Line 39: #else Is there a new spec, or maybe an addendum, that defines the new header for 64-bit? I'm not seeing anything at intel.com/fsp yet.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 36: default y
Since you defaulted this to y, and still selected X86_32 in many soc Kconfig files, are you intendin […]
This would require to recompile all existing blobs to x86_64 mode. If silicon vendors decide to do so, this is a yes.
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... PS5, Line 39: #else
Is there a new spec, or maybe an addendum, that defines the new header for 64-bit? I'm not seeing a […]
I'm not aware of such a spec, but it would just make integrating of such blobs easier. I can remove it for now if it causes to much confusion.
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Angel Pons, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#6).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/romstage.c M src/soc/intel/alderlake/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/elkhartlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/quark/Kconfig M src/soc/intel/quark/romstage/fsp_params.c M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/xeon_sp/Kconfig M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 25 files changed, 99 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/6
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Angel Pons, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#7).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/romstage.c M src/soc/intel/alderlake/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/elkhartlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/quark/Kconfig M src/soc/intel/quark/romstage/fsp_params.c M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/xeon_sp/Kconfig M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 26 files changed, 102 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/7
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 36: default y
This would require to recompile all existing blobs to x86_64 mode. […]
It was really more of a Kconfig question. Unless I'm reading it wrong, the selects in the various Kconfig files shouldn't be necessary since line 36 defaults to 'y'.
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 40: config PLATFORM_USES_FSP2_X86_64 It looks like this is currently unused and could removed. Maybe handle any clarifications with comments in the source.
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... PS5, Line 39: #else
I'm not aware of such a spec, but it would just make integrating of such blobs easier. […]
No, it looks important and surely wouldn't work right if you removed it. My question is mostly self-serving re. whether it's documented (de-facto or otherwise) so that we can plan for it to be consistent. We may want it someday for AMD too.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/7/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/7/src/soc/intel/elkhartlake/K... PS7, Line 33: 2_2 did you mean to change this one?
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Angel Pons, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#8).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 14 files changed, 81 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 36: default y
It was really more of a Kconfig question. […]
Removed the explicit select.
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/Kc... PS5, Line 40: config PLATFORM_USES_FSP2_X86_64
It looks like this is currently unused and could removed. […]
Removed for now.
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/48174/5/src/drivers/intel/fsp2_0/in... PS5, Line 39: #else
No, it looks important and surely wouldn't work right if you removed it. […]
Removed for now as no such spec exists.
https://review.coreboot.org/c/coreboot/+/48174/7/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48174/7/src/soc/intel/elkhartlake/K... PS7, Line 33: 2_2
did you mean to change this one?
Ack
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8: Code-Review+2
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8: Code-Review+1
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 145: UINT32 This is a file coming from the edk2 stable release. Can we get this updated at the source rather than making a local change here?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 169: FSP_EVENT_HANDLER *FspEventHandler; This needs a change too on ocp/deltalake.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 145: UINT32
This is a file coming from the edk2 stable release. […]
Not sure what you mean. It's in src/vendorcode as we don't use edk2-stable repository.
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 169: FSP_EVENT_HANDLER *FspEventHandler;
This needs a change too on ocp/deltalake.
Is this a function pointer? This won't work in cross arch environments as the ABI is different.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 169: FSP_EVENT_HANDLER *FspEventHandler;
Is this a function pointer? This won't work in cross arch environments as the ABI is different.
Coreboot does not use this optional callback, but it's indeed probably not possible to use this if desired. Should this be set to UINT8 Reserverd1[8] on ENV_X86_64 to make sure this does not build?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 145: UINT32
Not sure what you mean. It's in src/vendorcode as we don't use edk2-stable repository.
Yes, but isn't this copied *as is* from the edk2 repo? I meant that making local changes here can make it difficult to pick up any future changes from edk2 upstream repo.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 145: UINT32
Yes, but isn't this copied *as is* from the edk2 repo? I meant that making local changes here can ma […]
Opened an issue here to let the original author fix it: https://github.com/intel/FSP/issues/59 Yes it can be difficult to pick up future changes. My guess it that business goes as usual and somebody drops another copy of the upstream file here.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 8: Code-Review+2
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Subrata Banik, Angel Pons, Arthur Heymans, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#9).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 14 files changed, 98 insertions(+), 48 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/9
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Subrata Banik, Angel Pons, Arthur Heymans, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#10).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/mainboard/purism/librem_cnl/romstage.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 15 files changed, 100 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/10
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48174/10//COMMIT_MSG@11 PS10, Line 11: depending on the used mode to compile the FSP. Maybe reference the FSP upstream issue, where you brought this up?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/8/src/vendorcode/intel/edk2/e... PS8, Line 169: FSP_EVENT_HANDLER *FspEventHandler;
Is this a function pointer? […]
Done
Hello build bot (Jenkins), Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Subrata Banik, Angel Pons, Arthur Heymans, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#11).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/mainboard/purism/librem_cnl/romstage.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 15 files changed, 100 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/11
Attention is currently required from: Raul Rangel, Patrick Rudolph. Hello build bot (Jenkins), Raul Rangel, Mariusz Szafrański, Maulik V Vaghela, Sugnan Prabhu S, Subrata Banik, Angel Pons, Arthur Heymans, Krishna P Bhat D, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Marshall Dawson, Caveh Jalali, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Furquan Shaikh, Lean Sheng Tan, Ronak Kanabar, Michal Motyl, Srinidhi N Kaushik, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48174
to look at the new patch set (#12).
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This issue has been reported here: https://github.com/intel/FSP/issues/59
This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/mainboard/purism/librem_cnl/romstage.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 16 files changed, 109 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/48174/12
Attention is currently required from: Raul Rangel, Furquan Shaikh, Paul Menzel, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 11:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/48174/comment/8638bc28_b368083d PS10, Line 11: depending on the used mode to compile the FSP.
Maybe reference the FSP upstream issue, where you brought this up?
Done
Patchset:
PS11: Rebase and fixed the build.
File src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h:
https://review.coreboot.org/c/coreboot/+/48174/comment/5646cebe_d3925007 PS8, Line 145: UINT32
Opened an issue here to let the original author fix it: https://github.com/intel/FSP/issues/59 […]
Marking as resolved.
Attention is currently required from: Raul Rangel, Furquan Shaikh, Patrick Rudolph, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48174 )
Change subject: drivers/intel/fsp2_0: Fix running on x86_64 ......................................................................
drivers/intel/fsp2_0: Fix running on x86_64
Add new Kconfig symbols to mark FSP binary as x86_32. Fix the FSP headers and replace void pointers by fixed sized integers depending on the used mode to compile the FSP. This issue has been reported here: https://github.com/intel/FSP/issues/59
This is necessary to run on x86_64, as pointers have different size.
Add preprocessor error to warn that x86_64 FSP isn't supported by the current code.
Tested on Intel Skylake. FSP-M no longer returns the error "Invalid Parameter".
Change-Id: I6015005c4ee3fc2f361985cf8cff896bcefd04fb Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48174 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/header_display.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/mainboard/google/hatch/romstage_spd_smbus.c M src/mainboard/purism/librem_cnl/romstage.c M src/soc/amd/picasso/romstage.c M src/soc/intel/quark/romstage/fsp_params.c M src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h M src/vendorcode/amd/fsp/picasso/fsp_h_c99.h M src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h M src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h 16 files changed, 109 insertions(+), 52 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 14d9742..3cff8fa 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -31,6 +31,13 @@
if PLATFORM_USES_FSP2_0
+config PLATFORM_USES_FSP2_X86_32 + bool + default y + help + The FSP 2.0 runs in x86_32 protected mode. + Once there's a x86_64 FSP this needs to default to n. + config HAVE_INTEL_FSP_REPO bool help diff --git a/src/drivers/intel/fsp2_0/header_display.c b/src/drivers/intel/fsp2_0/header_display.c index a134fed..4f93666 100644 --- a/src/drivers/intel/fsp2_0/header_display.c +++ b/src/drivers/intel/fsp2_0/header_display.c @@ -19,24 +19,24 @@ printk(BIOS_SPEW, "Type: %s/%s\n", (hdr->component_attribute & 1) ? "release" : "debug", (hdr->component_attribute & 2) ? "official" : "test"); - printk(BIOS_SPEW, "image ID: %s, base 0x%lx + 0x%zx\n", - hdr->image_id, hdr->image_base, hdr->image_size); + printk(BIOS_SPEW, "image ID: %s, base 0x%zx + 0x%zx\n", + hdr->image_id, (size_t)hdr->image_base, (size_t)hdr->image_size); printk(BIOS_SPEW, "\tConfig region 0x%zx + 0x%zx\n", - hdr->cfg_region_offset, hdr->cfg_region_size); + (size_t)hdr->cfg_region_offset, (size_t)hdr->cfg_region_size);
if ((hdr->component_attribute >> 12) == FSP_HDR_ATTRIB_FSPM) { printk(BIOS_SPEW, "\tMemory init offset 0x%zx\n", - hdr->memory_init_entry_offset); + (size_t)hdr->memory_init_entry_offset); }
if ((hdr->component_attribute >> 12) == FSP_HDR_ATTRIB_FSPS) { printk(BIOS_SPEW, "\tSilicon init offset 0x%zx\n", - hdr->silicon_init_entry_offset); + (size_t)hdr->silicon_init_entry_offset); if (CONFIG(PLATFORM_USES_FSP2_2)) printk(BIOS_SPEW, "\tMultiPhaseSiInit offset 0x%zx\n", - hdr->multi_phase_si_init_entry_offset); + (size_t)hdr->multi_phase_si_init_entry_offset); printk(BIOS_SPEW, "\tNotify phase offset 0x%zx\n", - hdr->notify_phase_entry_offset); + (size_t)hdr->notify_phase_entry_offset); }
} diff --git a/src/drivers/intel/fsp2_0/include/fsp/info_header.h b/src/drivers/intel/fsp2_0/include/fsp/info_header.h index f237a37..aa9a435 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h +++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h @@ -4,6 +4,7 @@ #define _FSP2_0_INFO_HEADER_H_
#include <types.h> +#include <commonlib/bsd/compiler.h>
#define FSP_HDR_OFFSET 0x94 #if CONFIG(PLATFORM_USES_FSP2_2) @@ -16,24 +17,29 @@ #define FSP_HDR_ATTRIB_FSPM 2 #define FSP_HDR_ATTRIB_FSPS 3
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct fsp_header { uint32_t fsp_revision; - size_t image_size; - uintptr_t image_base; + uint32_t image_size; + uint32_t image_base; uint16_t image_attribute; uint8_t spec_version; uint16_t component_attribute; - size_t cfg_region_offset; - size_t cfg_region_size; - size_t temp_ram_init_entry; - size_t temp_ram_exit_entry; - size_t notify_phase_entry_offset; - size_t memory_init_entry_offset; - size_t silicon_init_entry_offset; - size_t multi_phase_si_init_entry_offset; + uint32_t cfg_region_offset; + uint32_t cfg_region_size; + uint32_t temp_ram_init_entry; + uint32_t temp_ram_exit_entry; + uint32_t notify_phase_entry_offset; + uint32_t memory_init_entry_offset; + uint32_t silicon_init_entry_offset; + uint32_t multi_phase_si_init_entry_offset; char image_id[sizeof(uint64_t) + 1]; uint8_t revision; -}; +} __packed; +#else +#error You need to implement this struct for x86_64 FSP +#endif +
enum cb_err fsp_identify(struct fsp_header *hdr, const void *fsp_blob);
diff --git a/src/drivers/intel/fsp2_0/include/fsp/upd.h b/src/drivers/intel/fsp2_0/include/fsp/upd.h index 979cff3..827c95d 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/upd.h +++ b/src/drivers/intel/fsp2_0/include/fsp/upd.h @@ -21,6 +21,7 @@ uint8_t Reserved[23]; } __packed;
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct FSPM_ARCH_UPD { /// /// Revision of the structure. For FSP v2.0 value is 1. @@ -31,12 +32,12 @@ /// Pointer to the non-volatile storage (NVS) data buffer. /// If it is NULL it indicates the NVS data is not available. /// - void *NvsBufferPtr; + uint32_t NvsBufferPtr; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - void *StackBase; + uint32_t StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. @@ -53,7 +54,11 @@ uint32_t BootMode; uint8_t Reserved1[8]; } __packed; +#else +#error You need to implement this struct for x86_64 FSP +#endif
+#endif struct FSPS_ARCH_UPD { /// /// Revision of the structure. For FSP v2.2 value is 1. diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 92f3d9d..f2fcec4 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -87,7 +87,7 @@ void *data; size_t mrc_size;
- arch_upd->NvsBufferPtr = NULL; + arch_upd->NvsBufferPtr = 0;
if (!CONFIG(CACHE_MRC_SETTINGS)) return; @@ -101,7 +101,7 @@ return;
/* MRC cache found */ - arch_upd->NvsBufferPtr = data; + arch_upd->NvsBufferPtr = (uintptr_t)data;
printk(BIOS_SPEW, "MRC cache found, size %zx\n", mrc_size); } @@ -142,7 +142,7 @@ stack_end) != CB_SUCCESS) return CB_ERR;
- arch_upd->StackBase = (void *)stack_begin; + arch_upd->StackBase = stack_begin; return CB_SUCCESS; }
@@ -159,7 +159,7 @@ * Non-CAR FSP 2.0 platforms pass a DRAM location for the FSP stack. */ if (CONFIG(FSP_USES_CB_STACK) || !ENV_CACHE_AS_RAM) { - arch_upd->StackBase = temp_ram; + arch_upd->StackBase = (uintptr_t)temp_ram; arch_upd->StackSize = sizeof(temp_ram); } else if (setup_fsp_stack_frame(arch_upd, memmap)) { return CB_ERR; @@ -237,7 +237,7 @@
fsp_version = fsp_memory_settings_version(hdr);
- upd = (FSPM_UPD *)(hdr->cfg_region_offset + hdr->image_base); + upd = (FSPM_UPD *)(uintptr_t)(hdr->cfg_region_offset + hdr->image_base);
fsp_verify_upd_header_signature(upd->FspUpdHeader.Signature, FSPM_UPD_SIGNATURE);
@@ -289,12 +289,12 @@ post_code(POST_MEM_PREINIT_PREP_END);
/* Call FspMemoryInit */ - fsp_raminit = (void *)(hdr->image_base + hdr->memory_init_entry_offset); + fsp_raminit = (void *)(uintptr_t)(hdr->image_base + hdr->memory_init_entry_offset); fsp_debug_before_memory_init(fsp_raminit, upd, &fspm_upd);
post_code(POST_FSP_MEMORY_INIT); timestamp_add_now(TS_FSP_MEMORY_INIT_START); - if (ENV_X86_64) + if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) status = protected_mode_call_2arg(fsp_raminit, (uintptr_t)&fspm_upd, (uintptr_t)fsp_get_hob_list_ptr()); diff --git a/src/drivers/intel/fsp2_0/notify.c b/src/drivers/intel/fsp2_0/notify.c index 8a51c0b..cbccc6e 100644 --- a/src/drivers/intel/fsp2_0/notify.c +++ b/src/drivers/intel/fsp2_0/notify.c @@ -16,7 +16,7 @@ if (!fsps_hdr.notify_phase_entry_offset) die("Notify_phase_entry_offset is zero!\n");
- fspnotify = (void *) (fsps_hdr.image_base + + fspnotify = (void *) (uintptr_t)(fsps_hdr.image_base + fsps_hdr.notify_phase_entry_offset); fsp_before_debug_notify(fspnotify, ¬ify_params);
@@ -31,7 +31,7 @@ post_code(POST_FSP_NOTIFY_BEFORE_END_OF_FIRMWARE); }
- if (ENV_X86_64) + if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) ret = protected_mode_call_1arg(fspnotify, (uintptr_t)¬ify_params); else ret = fspnotify(¬ify_params); diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index 26ff59d..8572b24 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -86,7 +86,7 @@ struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number;
- supd = (FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base); + supd = (FSPS_UPD *) (uintptr_t)(hdr->cfg_region_offset + hdr->image_base);
fsp_verify_upd_header_signature(supd->FspUpdHeader.Signature, FSPS_UPD_SIGNATURE);
@@ -110,14 +110,14 @@ logo_entry = soc_load_logo(upd);
/* Call SiliconInit */ - silicon_init = (void *) (hdr->image_base + + silicon_init = (void *) (uintptr_t)(hdr->image_base + hdr->silicon_init_entry_offset); fsp_debug_before_silicon_init(silicon_init, supd, upd);
timestamp_add_now(TS_FSP_SILICON_INIT_START); post_code(POST_FSP_SILICON_INIT);
- if (ENV_X86_64) + if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) status = protected_mode_call_1arg(silicon_init, (uintptr_t)upd); else status = silicon_init(upd); @@ -145,7 +145,7 @@ return;
/* Call MultiPhaseSiInit */ - multi_phase_si_init = (void *) (hdr->image_base + + multi_phase_si_init = (void *) (uintptr_t)(hdr->image_base + hdr->multi_phase_si_init_entry_offset);
/* Implementing multi_phase_si_init() is optional as per FSP 2.2 spec */ diff --git a/src/mainboard/google/hatch/romstage_spd_smbus.c b/src/mainboard/google/hatch/romstage_spd_smbus.c index e697379..3d84e52 100644 --- a/src/mainboard/google/hatch/romstage_spd_smbus.c +++ b/src/mainboard/google/hatch/romstage_spd_smbus.c @@ -28,10 +28,10 @@ printk(BIOS_WARNING, "Invalid SPD cache\n"); } else { dimm_changed = check_if_dimm_changed(spd_cache, &blk); - if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != NULL) { + if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != 0) { /* Set mrc_cache as invalid */ printk(BIOS_INFO, "Set mrc_cache as invalid\n"); - memupd->FspmArchUpd.NvsBufferPtr = NULL; + memupd->FspmArchUpd.NvsBufferPtr = 0; } } need_update_cache = true; diff --git a/src/mainboard/purism/librem_cnl/romstage.c b/src/mainboard/purism/librem_cnl/romstage.c index fd3154f..5d92316 100644 --- a/src/mainboard/purism/librem_cnl/romstage.c +++ b/src/mainboard/purism/librem_cnl/romstage.c @@ -59,10 +59,10 @@ printk(BIOS_WARNING, "Invalid SPD cache\n"); } else { dimm_changed = check_if_dimm_changed(spd_cache, &blk); - if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != NULL) { + if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != 0) { /* Set mrc_cache as invalid */ printk(BIOS_INFO, "Set mrc_cache as invalid\n"); - memupd->FspmArchUpd.NvsBufferPtr = NULL; + memupd->FspmArchUpd.NvsBufferPtr = 0; } } need_update_cache = true; diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c index bc51456..1391536 100644 --- a/src/soc/amd/picasso/romstage.c +++ b/src/soc/amd/picasso/romstage.c @@ -92,7 +92,7 @@ FSP_M_CONFIG *mcfg = &mupd->FspmConfig; const struct soc_amd_picasso_config *config = config_of_soc();
- mupd->FspmArchUpd.NvsBufferPtr = soc_fill_mrc_cache(); + mupd->FspmArchUpd.NvsBufferPtr = (uintptr_t)soc_fill_mrc_cache();
mcfg->pci_express_base_addr = CONFIG_MMCONF_BASE_ADDRESS; mcfg->tseg_size = CONFIG_SMM_TSEG_SIZE; diff --git a/src/soc/intel/quark/romstage/fsp_params.c b/src/soc/intel/quark/romstage/fsp_params.c index efe3c10..11f7059 100644 --- a/src/soc/intel/quark/romstage/fsp_params.c +++ b/src/soc/intel/quark/romstage/fsp_params.c @@ -84,7 +84,7 @@ /* Update the architectural UPD values. */ aupd = &fspm_upd->FspmArchUpd; aupd->BootLoaderTolumSize = cbmem_overhead_size(); - aupd->StackBase = (void *)(CONFIG_FSP_ESRAM_LOC - aupd->StackSize); + aupd->StackBase = (uintptr_t)(CONFIG_FSP_ESRAM_LOC - aupd->StackSize); aupd->BootMode = FSP_BOOT_WITH_FULL_CONFIGURATION;
/* Display the ESRAM layout */ @@ -97,8 +97,8 @@ "+-------------------+ 0x%08x (CONFIG_FSP_ESRAM_LOC)\n", CONFIG_FSP_ESRAM_LOC); printk(BIOS_SPEW, "| FSP stack |\n"); - printk(BIOS_SPEW, "+-------------------+ %p\n", - aupd->StackBase); + printk(BIOS_SPEW, "+-------------------+ 0x%zx\n", + (size_t)aupd->StackBase); printk(BIOS_SPEW, "| |\n"); printk(BIOS_SPEW, "+-------------------+ %p\n", _car_unallocated_start); diff --git a/src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h b/src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h index c477a4f..1a295f5 100644 --- a/src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h +++ b/src/vendorcode/amd/fsp/cezanne/fsp_h_c99.h @@ -35,11 +35,15 @@
_Static_assert(sizeof(FSP_UPD_HEADER) == 32, "FSP_UPD_HEADER not packed");
+ +#if CONFIG(PLATFORM_USES_FSP2_X86_32) typedef struct __packed { uint8_t Revision; uint8_t Reserved[3]; - void *NvsBufferPtr; - void *StackBase; + /* Note: This ought to be void*, but that won't allow calling this binary on x86_64. */ + uint32_t NvsBufferPtr; + /* Note: This ought to be void*, but that won't allow calling this binary on x86_64. */ + uint32_t StackBase; uint32_t StackSize; uint32_t BootLoaderTolumSize; uint32_t BootMode; @@ -47,5 +51,8 @@ } FSPM_ARCH_UPD;
_Static_assert(sizeof(FSPM_ARCH_UPD) == 32, "FSPM_ARCH_UPD not packed"); +#else +#error You need to implement this struct for x86_64 FSP +#endif
#endif /* FSP_H_C99_H */ diff --git a/src/vendorcode/amd/fsp/picasso/fsp_h_c99.h b/src/vendorcode/amd/fsp/picasso/fsp_h_c99.h index c477a4f..79ef925 100644 --- a/src/vendorcode/amd/fsp/picasso/fsp_h_c99.h +++ b/src/vendorcode/amd/fsp/picasso/fsp_h_c99.h @@ -35,11 +35,14 @@
_Static_assert(sizeof(FSP_UPD_HEADER) == 32, "FSP_UPD_HEADER not packed");
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) typedef struct __packed { uint8_t Revision; uint8_t Reserved[3]; - void *NvsBufferPtr; - void *StackBase; + /* Note: This ought to be void*, but that won't allow calling this binary on x86_64. */ + uint32_t NvsBufferPtr; + /* Note: This ought to be void*, but that won't allow calling this binary on x86_64. */ + uint32_t StackBase; uint32_t StackSize; uint32_t BootLoaderTolumSize; uint32_t BootMode; @@ -47,5 +50,8 @@ } FSPM_ARCH_UPD;
_Static_assert(sizeof(FSPM_ARCH_UPD) == 32, "FSPM_ARCH_UPD not packed"); +#else +#error You need to implement this struct for x86_64 FSP +#endif
#endif /* FSP_H_C99_H */ diff --git a/src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h b/src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h index c58b169..df2b7dc 100644 --- a/src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h +++ b/src/vendorcode/intel/edk2/UDK2015/IntelFsp2Pkg/Include/FspEas/FspApi.h @@ -50,6 +50,7 @@ UINT8 Reserved[23]; } FSP_UPD_HEADER;
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) /// /// FSPM_ARCH_UPD Configuration. /// @@ -63,12 +64,16 @@ /// Pointer to the non-volatile storage (NVS) data buffer. /// If it is NULL it indicates the NVS data is not available. /// - VOID *NvsBufferPtr; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 NvsBufferPtr; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - VOID *StackBase; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. @@ -85,6 +90,9 @@ UINT32 BootMode; UINT8 Reserved1[8]; } FSPM_ARCH_UPD; +#else +#error You need to implement this struct for x86_64 FSP +#endif
/// /// FSPT_UPD_COMMON Configuration. diff --git a/src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h b/src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h index 29c98ee..c22b701 100644 --- a/src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h +++ b/src/vendorcode/intel/edk2/UDK2017/IntelFsp2Pkg/Include/FspEas/FspApi.h @@ -50,6 +50,7 @@ UINT8 Reserved[23]; } FSP_UPD_HEADER;
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) /// /// FSPM_ARCH_UPD Configuration. /// @@ -63,12 +64,16 @@ /// Pointer to the non-volatile storage (NVS) data buffer. /// If it is NULL it indicates the NVS data is not available. /// - VOID *NvsBufferPtr; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 NvsBufferPtr; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - VOID *StackBase; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. @@ -85,6 +90,9 @@ UINT32 BootMode; UINT8 Reserved1[8]; } FSPM_ARCH_UPD; +#else +#error You need to implement this struct for x86_64 FSP +#endif
/// /// FSPT_UPD_COMMON Configuration. diff --git a/src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h b/src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h index eb9ce86..8314f0b 100644 --- a/src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h +++ b/src/vendorcode/intel/edk2/edk2-stable202005/IntelFsp2Pkg/Include/FspEas/FspApi.h @@ -128,6 +128,7 @@ UINT8 Reserved1[20]; } FSPT_ARCH_UPD;
+#if CONFIG(PLATFORM_USES_FSP2_X86_32) /// /// FSPM_ARCH_UPD Configuration. /// @@ -141,12 +142,16 @@ /// Pointer to the non-volatile storage (NVS) data buffer. /// If it is NULL it indicates the NVS data is not available. /// - VOID *NvsBufferPtr; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 NvsBufferPtr; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - VOID *StackBase; + /// Note: This ought to be VOID*, but that won't allow calling this binary on x86_64. + /// + UINT32 StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. @@ -165,9 +170,14 @@ /// Optional event handler for the bootloader to be informed of events occurring during FSP execution. /// This value is only valid if Revision is >= 2. /// - FSP_EVENT_HANDLER *FspEventHandler; + /// Note: This ought to be FSP_EVENT_HANDLER*, but that won't allow calling this binary on x86_64. + /// + UINT32 FspEventHandler; UINT8 Reserved1[4]; } FSPM_ARCH_UPD; +#else +#error You need to implement this struct for x86_64 FSP +#endif
typedef struct { ///