coreboot-gerrit January 2024

coreboot-gerrit@coreboot.org

1 participants
3018 discussions

Change in coreboot[master]: [TEST] Add support for link time optimization
by Jacob Garber (Code Review) 22 Feb '24

22 Feb '24

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38989 ) Change subject: [TEST] Add support for link time optimization ...................................................................... [TEST] Add support for link time optimization - Code generation is not done until after linking, so the compiler must be invoked at all linking stages instead of the linker. As a consequence all linker arguments must be prefixed with -Wl. - Partial linking is not supported. Instead, object files are collected into thin archives that are linked instead. - The dead_code() macro causes linking errors, since dead functions aren't optimized out until after linking has begun. This macro could be replaced with the preprocessor if necessary, or just disabled for LTO builds. - Wrapping libgcc functions causes a symbol mismatch when using LTO. Wrapping these functions was originally done to support alternate regparam values, though AFAICT this isn't used anywhere. Using LTO leads to a ~10% decrease in stage size for QEMU and ~18% for the Thinkpad T500, and both targets boot successfully. Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> --- M Makefile.inc M src/Kconfig M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/include/assert.h M src/lib/Makefile.inc M src/lib/gcc.c M toolchain.inc M util/xcompile/xcompile 9 files changed, 42 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38989/1 diff --git a/Makefile.inc b/Makefile.inc index 1f18726..b85c11b 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -434,6 +434,10 @@ endif endif +ifeq ($(CONFIG_LTO),y) +CFLAGS_common += -flto -fuse-linker-plugin -fno-fat-lto-objects +endif + ADAFLAGS_common += -gnatp ADAFLAGS_common += -Wuninitialized -Wall -Werror ADAFLAGS_common += -pipe -g -nostdinc @@ -478,7 +482,7 @@ # Disable style checks for now ADAFLAGS_common += -gnatyN -LDFLAGS_common := --gc-sections -nostdlib -nostartfiles -static --emit-relocs +LDFLAGS_common := -nostdlib -nostartfiles -static -Wl,--emit-relocs,--gc-sections ifeq ($(CONFIG_WARNINGS_ARE_ERRORS),y) CFLAGS_common += -Werror diff --git a/src/Kconfig b/src/Kconfig index f75f942..4b81818 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -116,6 +116,15 @@ For details see https://ccache.samba.org. +config LTO + bool "Use link time optimization" + # Enable now for testing + default y + depends on COMPILER_GCC + help + Compile with link time optimization. This can often decrease the + final binary size, but may increase compilation time. + config FMD_GENPARSER bool "Generate flashmap descriptor parser using flex and bison" default n diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 534f2ce..75f96d2 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -80,7 +80,7 @@ $$(objcbfs)/$(1).debug: $$$$($(1)-libs) $$$$($(1)-objs) @printf " LINK $$(subst $$(obj)/,,$$(@))\n" - $$(LD_$(1)) $$(LDFLAGS_$(1)) -o $$@ -L$$(obj) $$(COMPILER_RT_FLAGS_$(1)) --whole-archive --start-group $$(filter-out %.ld,$$($(1)-objs)) $$($(1)-libs) --no-whole-archive $$(COMPILER_RT_$(1)) --end-group -T $(call src-to-obj,$(1),$(dir)/memlayout.ld) --oformat $(2) + $$(LD_$(1)) $$(LDFLAGS_$(1)) -o $$@ -L$$(obj) $$(COMPILER_RT_FLAGS_$(1)) -Wl,--whole-archive,--start-group $$(filter-out %.ld,$$($(1)-objs)) $$($(1)-libs) -Wl,--no-whole-archive $$(COMPILER_RT_$(1)) -Wl,--end-group -T $(call src-to-obj,$(1),$(dir)/memlayout.ld) -Wl,--oformat=$(2) -LANG=C LC_ALL= $$(OBJCOPY_$(1)) --only-section .illegal_globals $$(@) $$(objcbfs)/$(1)_null.offenders >/dev/null 2>&1 if [ -z "$$$$($$(NM_$(1)) $$(objcbfs)/$(1)_null.offenders 2>&1 | grep 'no symbols')" ];then \ echo "Forbidden global variables in $(1):"; \ @@ -212,11 +212,11 @@ postcar-y += postcar.c postcar-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c -LDFLAGS_postcar += -Map $(objcbfs)/postcar.map +LDFLAGS_postcar += -Wl,-Map,$(objcbfs)/postcar.map $(objcbfs)/postcar.debug: $$(postcar-objs) @printf " LINK $(subst $(obj)/,,$(@))\n" - $(LD_postcar) $(LDFLAGS_postcar) -o $@ -L$(obj) $(COMPILER_RT_FLAGS_postcar) --whole-archive --start-group $(filter-out %.ld,$^) --no-whole-archive $(COMPILER_RT_postcar) --end-group -T $(call src-to-obj,postcar,src/arch/x86/memlayout.ld) + $(LD_postcar) $(LDFLAGS_postcar) -o $@ -L$(obj) $(COMPILER_RT_FLAGS_postcar) -Wl,--whole-archive,--start-group $(filter-out %.ld,$^) -Wl,--no-whole-archive $(COMPILER_RT_postcar) -Wl,--end-group -T $(call src-to-obj,postcar,src/arch/x86/memlayout.ld) $(objcbfs)/postcar.elf: $(objcbfs)/postcar.debug.rmod cp $< $@ @@ -309,18 +309,20 @@ endif -$(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.o $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) - @printf " CC $(subst $(obj)/,,$(@))\n" - $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) -o $@ -L$(obj) $< -T $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) - -$(objgenerated)/ramstage.o: $$(ramstage-objs) $(COMPILER_RT_ramstage) $$(ramstage-libs) - @printf " CC $(subst $(obj)/,,$(@))\n" ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) - $(LD_ramstage) -m elf_i386 -r -o $@ $(COMPILER_RT_FLAGS_ramstage) --whole-archive --start-group $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) --no-whole-archive $(COMPILER_RT_ramstage) --end-group +LDFLAGS_ramstage += -Wl,-m,elf_i386 else - $(LD_ramstage) -m elf_x86_64 -r -o $@ $(COMPILER_RT_FLAGS_ramstage) --whole-archive --start-group $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) --no-whole-archive $(COMPILER_RT_ramstage) --end-group +LDFLAGS_ramstage += -Wl,-m,elf_x86_64 endif +$(objcbfs)/ramstage.debug: $(objgenerated)/ramstage.a $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) + @printf " CC $(subst $(obj)/,,$(@))\n" + $(LD_ramstage) $(CPPFLAGS) $(LDFLAGS_ramstage) $(COMPILER_RT_FLAGS_ramstage) -o $@ -L$(obj) -Wl,--whole-archive,--start-group $< -Wl,--no-whole-archive $(COMPILER_RT_ramstage) -Wl,--end-group -T $(call src-to-obj,ramstage,src/arch/x86/memlayout.ld) + +$(objgenerated)/ramstage.a: $$(ramstage-objs) $$(ramstage-libs) + @printf " AR $(subst $(obj)/,,$(@))\n" + $(AR_ramstage) rcT $@ $(filter-out %.ld,$(ramstage-objs)) $(ramstage-libs) + endif # CONFIG_ARCH_RAMSTAGE_X86_32 / CONFIG_ARCH_RAMSTAGE_X86_64 smm-$(CONFIG_IDT_IN_EVERY_STAGE) += exception.c diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 11a4e67..f582a31 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -25,8 +25,8 @@ smm-generic-ccopts += -D__SMM__ smm-c-deps:=$$(OPTION_TABLE_H) -$(obj)/smm/smm.o: $$(smm-objs) $(COMPILER_RT_smm) - $(LD_smm) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smm) --whole-archive --start-group $(smm-objs) --no-whole-archive $(COMPILER_RT_smm) --end-group +$(obj)/smm/smm.a: $$(smm-objs) + $(AR_smm) rcT $@ $^ # change to the target path because objcopy will use the path name in its # ELF symbol names. @@ -53,7 +53,7 @@ # SMM Stub Module. The stub is used as a trampoline for relocation and normal # SMM handling. $(obj)/smmstub/smmstub.o: $$(smmstub-objs) $(COMPILER_RT_smmstub) - $(LD_smmstub) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smmstub) --whole-archive --start-group $(smmstub-objs) --no-whole-archive $(COMPILER_RT_smmstub) --end-group + $(LD_smmstub) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smmstub) -Wl,--whole-archive,--start-group $(smmstub-objs) -Wl,--no-whole-archive $(COMPILER_RT_smmstub) -Wl,--end-group # Link the SMM stub module with a 0-byte heap. ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) @@ -72,9 +72,9 @@ # C-based SMM handler. ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_32)) +$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.a, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_32)) else -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_64)) +$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.a, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_64)) endif $(obj)/smm/smm: $(obj)/smm/smm.elf.rmod @@ -82,8 +82,8 @@ else # CONFIG_SMM_TSEG -$(obj)/smm/smm: $(obj)/smm/smm.o $(src)/cpu/x86/smm/smm.ld - $(LD_smm) $(LDFLAGS_smm) -o $(obj)/smm/smm.elf -T $(src)/cpu/x86/smm/smm.ld $(obj)/smm/smm.o +$(obj)/smm/smm: $(obj)/smm/smm.a $(src)/cpu/x86/smm/smm.ld + $(LD_smm) $(LDFLAGS_smm) -o $(obj)/smm/smm.elf -T $(src)/cpu/x86/smm/smm.ld -Wl,--whole-archive,--start-group $(obj)/smm/smm.a -Wl,--no-whole-archive $(COMPILER_RT_smm) --end-group $(NM_smm) -n $(obj)/smm/smm.elf | sort > $(obj)/smm/smm.map $(OBJCOPY_smm) -O binary $(obj)/smm/smm.elf $@ diff --git a/src/include/assert.h b/src/include/assert.h index e0db0bc..6901953 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -61,7 +61,8 @@ dead_code_assertion_failed_at_line_##line(); \ } while (0) #define _dead_code(line) __dead_code(line) -#define dead_code() _dead_code(__LINE__) +//#define dead_code() _dead_code(__LINE__) +#define dead_code() /* This can be used in the context of an expression of type 'type'. */ #define dead_code_t(type) ({ \ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 2333f64..ac6d624 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -291,7 +291,7 @@ # rmdoule is named $(1).rmod define rmodule_link $(strip $(1)): $(strip $(2)) $$(COMPILER_RT_rmodules_$(4)) $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) | $$(RMODTOOL) - $$(LD_rmodules_$(4)) $$(LDFLAGS_rmodules_$(4)) $(RMODULE_LDFLAGS) -T $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) --defsym=__heap_size=$(strip $(3)) -o $$@ --whole-archive --start-group $(filter-out %.ld,$(2)) --end-group + $$(LD_rmodules_$(4)) $$(LDFLAGS_rmodules_$(4)) $(RMODULE_LDFLAGS) -T $(call src-to-obj,rmodules_$(4),src/lib/rmodule.ld) -Wl,--defsym=__heap_size=$(strip $(3)) -o $$@ -Wl,--whole-archive,--start-group $(filter-out %.ld,$(2)) -Wl,--no-whole-archive $$(COMPILER_RT_rmodules_$(4)) -Wl,--end-group $$(NM_rmodules_$(4)) -n $$@ > $$(basename $$(a)).map endef diff --git a/src/lib/gcc.c b/src/lib/gcc.c index 5a93f45..66a7788 100644 --- a/src/lib/gcc.c +++ b/src/lib/gcc.c @@ -24,6 +24,7 @@ /* TODO: maybe this code should move to arch/x86 as architecture * specific implementations may vary */ +#if 0 #define WRAP_LIBGCC_CALL(type, name) \ asmlinkage type __real_##name(type a, type b); \ type __wrap_##name(type a, type b); \ @@ -33,3 +34,4 @@ WRAP_LIBGCC_CALL(unsigned long long, __udivdi3) WRAP_LIBGCC_CALL(long long, __moddi3) WRAP_LIBGCC_CALL(unsigned long long, __umoddi3) +#endif diff --git a/toolchain.inc b/toolchain.inc index 865227b..f52a518 100644 --- a/toolchain.inc +++ b/toolchain.inc @@ -117,7 +117,7 @@ $(error Check your .config file for CONFIG_ARCH_$(1)_* settings)) CC_$(1) := $(CC_$(2)) GCC_$(1) := $(GCC_CC_$(2)) -LD_$(1) := $(LD_$(2)) +LD_$(1) := $(CC_$(2)) NM_$(1) := $(NM_$(2)) AR_$(1) := $(AR_$(2)) GNATBIND_$(1) := $(GNATBIND_$(2)) @@ -130,7 +130,7 @@ CPPFLAGS_$(1) = $$(CPPFLAGS_common) $$(CPPFLAGS_$(2)) -D__ARCH_$(2)__ COMPILER_RT_$(1) := $$(COMPILER_RT_$(2)) COMPILER_RT_FLAGS_$(1) := $$(COMPILER_RT_FLAGS_$(2)) -LDFLAGS_$(1) = $$(LDFLAGS_common) $$(LDFLAGS_$(2)) +LDFLAGS_$(1) = $$(CFLAGS_$(1)) $$(LDFLAGS_common) $$(LDFLAGS_$(2)) endef # define_class: Allows defining any program as dynamic class and compiler tool diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile index 3203d71..8515813 100755 --- a/util/xcompile/xcompile +++ b/util/xcompile/xcompile @@ -363,7 +363,7 @@ TCLIST="i386 x86_64" TWIDTH="32" TABI="elf" - CC_RT_EXTRA_GCC="--wrap __divdi3 --wrap __udivdi3 --wrap __moddi3 --wrap __umoddi3" + #CC_RT_EXTRA_GCC="-Wl,--wrap=__divdi3,--wrap=__udivdi3,--wrap=__moddi3,--wrap=__umoddi3" } arch_config_ppc64() { -- To view, visit https://review.coreboot.org/c/coreboot/+/38989 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I48c31ea8b1b57276125cffdac44c7c16642547ac Gerrit-Change-Number: 38989 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

9 34

Change in coreboot[master]: nb/amd/agesa: select ROMSTAGE_CACHED_CBMEM
by Arthur Heymans (Code Review) 20 Feb '24

20 Feb '24

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37198 ) Change subject: nb/amd/agesa: select ROMSTAGE_CACHED_CBMEM ...................................................................... nb/amd/agesa: select ROMSTAGE_CACHED_CBMEM AGESA sets up MTRR's to cache the whole dram, so decompressing postcar stage to cbmem should be fast and is now selected by default. Change-Id: I62ffe1bd646e9ddad77be240f030601790f4da4b Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/northbridge/amd/agesa/family14/Kconfig M src/northbridge/amd/agesa/family15tn/Kconfig M src/northbridge/amd/agesa/family16kb/Kconfig 3 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/37198/1 diff --git a/src/northbridge/amd/agesa/family14/Kconfig b/src/northbridge/amd/agesa/family14/Kconfig index 173714f..cfafb23 100644 --- a/src/northbridge/amd/agesa/family14/Kconfig +++ b/src/northbridge/amd/agesa/family14/Kconfig @@ -14,6 +14,7 @@ ## config NORTHBRIDGE_AMD_AGESA_FAMILY14 bool + select ROMSTAGE_CACHED_CBMEM if NORTHBRIDGE_AMD_AGESA_FAMILY14 diff --git a/src/northbridge/amd/agesa/family15tn/Kconfig b/src/northbridge/amd/agesa/family15tn/Kconfig index a0841eb..e6a469f 100644 --- a/src/northbridge/amd/agesa/family15tn/Kconfig +++ b/src/northbridge/amd/agesa/family15tn/Kconfig @@ -14,6 +14,7 @@ ## config NORTHBRIDGE_AMD_AGESA_FAMILY15_TN bool + select ROMSTAGE_CACHED_CBMEM if NORTHBRIDGE_AMD_AGESA_FAMILY15_TN diff --git a/src/northbridge/amd/agesa/family16kb/Kconfig b/src/northbridge/amd/agesa/family16kb/Kconfig index 2be2fd3..e34ef60 100644 --- a/src/northbridge/amd/agesa/family16kb/Kconfig +++ b/src/northbridge/amd/agesa/family16kb/Kconfig @@ -15,6 +15,7 @@ ## config NORTHBRIDGE_AMD_AGESA_FAMILY16_KB bool + select ROMSTAGE_CACHED_CBMEM if NORTHBRIDGE_AMD_AGESA_FAMILY16_KB -- To view, visit https://review.coreboot.org/c/coreboot/+/37198 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I62ffe1bd646e9ddad77be240f030601790f4da4b Gerrit-Change-Number: 37198 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

6 28

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Ronak Kanabar, Wonkyu Kim. Hello Andrey Petrov, Bora Guvendik, Ronak Kanabar, Wonkyu Kim, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80277?usp=email to look at the new patch set (#2). Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... drivers/intel/fsp2_0: Support FSP 2.4 64-bits FSP 2.4 brings FSP 64-bits support which requires some adjustments in coreboot: - Stack alignment: 1. FSP functions must be called with the stack 16-bytes aligned. This is already setup properly with the default value of the `mpreferred-stack-boundary' compiler option (4). 2. The FSP stack buffer supplied by coreboot through the `StackBase' UPD must be 16-bytes aligned. - The EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which is not working well when included by coreboot. While it has no side-effect on i386 because the C calling convention used by coreboot and FSP are the same, it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling convention while coreboot uses the System V AMD64 ABI. Fortunately, EDK2 header allows to override the EFIAPI definition. The __ms_abi__ attribute works for both i386 and x86_64. This attribute has to be set to all functions calling or called by the FSP. - The EFI_STATUS/efi_return_status_t size changes with the architecture (32-bits vs 64-bits). To print statuses independently of the architecture we leverage the size_t 'z' print format which is generally aligned with the architecture size too. In addition, this commit sets`PLATFORM_USES_FSP2_X86_32' to `n' by default if FSP 2.4 is enabled as 64-bits FSP should be norm moving forward. Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/debug.c M src/drivers/intel/fsp2_0/fsp_debug_event.c M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/include/fsp/debug.h M src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/soc_binding.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/ppi/mp_service1.c M src/drivers/intel/fsp2_0/ppi/mp_service2.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/efi/efi_datatype.h M src/soc/amd/common/fsp/fsp_reset.c M src/soc/intel/common/fsp_reset.c M src/soc/intel/xeon_sp/bootblock.c 18 files changed, 94 insertions(+), 63 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/80277/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80277?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Gerrit-Change-Number: 80277 Gerrit-PatchSet: 2 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in coreboot[main]: util/ifdtool: Add new cmdline to enable GPR0 protection
by Reka Norman (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Stefan Reinauer, Subrata Banik. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80235?usp=email ) Change subject: util/ifdtool: Add new cmdline to enable GPR0 protection ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80235?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: I27c533ae4109c79299f4e7ff75e750d7cc64280f Gerrit-Change-Number: 80235 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 31 Jan 2024 23:41:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: [Squash]: Add ACPI BDAT support
by Gaggery (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Arthur Heymans, Cliff Huang, Karthik Ramasubramanian, Lance Zhao, Pratikkumar V Prajapati, Ravishankar Sarawadi, Subrata Banik, Tim Wawrzynczak. Gaggery has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77255?usp=email ) Change subject: [Squash]: Add ACPI BDAT support ...................................................................... Patch Set 14: (1 comment) File src/soc/intel/common/block/acpi/acpi_bdat.c: https://review.coreboot.org/c/coreboot/+/77255/comment/47794d4e_1fab10fb : PS12, Line 16: #define GUID_TAIL_BYTES 8 > nit: line up the right sides Done -- To view, visit https://review.coreboot.org/c/coreboot/+/77255?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: I5eb57f65ef5f24458f09587b7c7694156f2ed1ce Gerrit-Change-Number: 77255 Gerrit-PatchSet: 14 Gerrit-Owner: Gaggery <gaggery.tsai(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 31 Jan 2024 23:41:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: commonlib: cbmem_id: Add id for ACPI BDAT
by Gaggery (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Paul Menzel, Subrata Banik. Gaggery has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77254?usp=email ) Change subject: commonlib: cbmem_id: Add id for ACPI BDAT ...................................................................... Patch Set 11: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/77254/comment/a2074dfd_d3271a45 : PS8, Line 7: src/commonlib: Add ACPI BDAT in cbmem support > Please remove the leading `src/`. […] Done https://review.coreboot.org/c/coreboot/+/77254/comment/60d9cc06_7b0bb8d3 : PS8, Line 9: BDAT(BIOS Data) > Please add a space before the (. Done File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h: https://review.coreboot.org/c/coreboot/+/77254/comment/7dc0d26e_05679e9a : PS8, Line 7: #define CBMEM_ID_ACPI_BERT 0x42455254 : #define CBMEM_ID_ACPI_BDAT 0x42444154 > Please sort it. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/77254?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: Ic4925adfdaff36e28d47829322a8405ed92cad17 Gerrit-Change-Number: 77254 Gerrit-PatchSet: 11 Gerrit-Owner: Gaggery <gaggery.tsai(a)intel.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 31 Jan 2024 23:40:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 01 Feb '24

01 Feb '24

Jérémy Compostella has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/80276?usp=email ) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/coreboot/+/80276?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: I3fa012fa6f98e4bb3c2a033189554ad5a6ddfced Gerrit-Change-Number: 80276 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-MessageType: abandon

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Andrey Petrov, Ronak Kanabar. Jérémy Compostella has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80277?usp=email ) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... drivers/intel/fsp2_0: Support FSP 2.4 64-bits FSP 2.4 brings FSP 64-bits support which requires some adjustments in coreboot: - Stack alignment: 1. FSP functions must be called with the stack 16-bytes aligned. This is already setup properly with the default value of the `mpreferred-stack-boundary' compiler option (4). 2. The FSP stack buffer supplied by coreboot through the `StackBase' UPD must be 16-bytes aligned. - The EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which is not working well when included by coreboot. While it has no side-effect on i386 because the C calling convention used by coreboot and FSP are the same, it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling convention while coreboot uses the System V AMD64 ABI. Fortunately, EDK2 header allows to override the EFIAPI definition. The __ms_abi__ attribute works for both i386 and x86_64. This attribute has to be set to all functions calling or called by the FSP. - The EFI_STATUS/efi_return_status_t size changes with the architecture (32-bits vs 64-bits). To print statuses independently of the architecture we leverage the size_t 'z' print format which is generally aligned with the architecture size too. In addition, this commit sets`PLATFORM_USES_FSP2_X86_32' to `n' by default if FSP 2.4 is enabled as 64-bits FSP should be norm moving forward. Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/debug.c M src/drivers/intel/fsp2_0/fsp_debug_event.c M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/include/fsp/debug.h M src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/soc_binding.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/ppi/mp_service1.c M src/drivers/intel/fsp2_0/ppi/mp_service2.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/efi/efi_datatype.h M src/soc/intel/common/fsp_reset.c 16 files changed, 88 insertions(+), 59 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/80277/1 diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 2c8bdc4..61d68f6 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -55,10 +55,11 @@ config PLATFORM_USES_FSP2_X86_32 bool + default n if PLATFORM_USES_FSP2_4 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. + Specify if the FSP binaries are 32-bits (yes) or 64-bits + (no). Starting with FSP specification 2.4 they can be 64-bits. config HAVE_INTEL_FSP_REPO bool diff --git a/src/drivers/intel/fsp2_0/debug.c b/src/drivers/intel/fsp2_0/debug.c index 8c0d67d..e74e745 100644 --- a/src/drivers/intel/fsp2_0/debug.c +++ b/src/drivers/intel/fsp2_0/debug.c @@ -91,10 +91,10 @@ printk(BIOS_SPEW, "\t%p: &hob_list_ptr\n", fsp_get_hob_list_ptr()); } -void fsp_debug_after_memory_init(uint32_t status) +void fsp_debug_after_memory_init(efi_return_status_t status) { if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspMemoryInit returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspMemoryInit returned 0x%zx\n", (size_t)status); if (status != FSP_SUCCESS) return; @@ -136,13 +136,13 @@ printk(BIOS_SPEW, "\t%p: upd\n", fsps_new_upd); } -void fsp_debug_after_silicon_init(uint32_t status) +void fsp_debug_after_silicon_init(efi_return_status_t status) { if (CONFIG(CHECK_GPIO_CONFIG_CHANGES)) fsp_gpio_config_check(AFTER_FSP_CALL, "FSP Silicon Init"); if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspSiliconInit returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspSiliconInit returned 0x%zx\n", (size_t)status); /* Display the HOBs */ if (CONFIG(DISPLAY_HOBS)) @@ -170,13 +170,13 @@ printk(BIOS_SPEW, "\t%p: notify_params\n", notify_params); } -void fsp_debug_after_notify(uint32_t status) +void fsp_debug_after_notify(efi_return_status_t status) { if (CONFIG(CHECK_GPIO_CONFIG_CHANGES)) fsp_gpio_config_check(AFTER_FSP_CALL, "FSP Notify"); if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspNotify returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspNotify returned 0x%zx\n", (size_t)status); /* Display the HOBs */ if (CONFIG(DISPLAY_HOBS)) diff --git a/src/drivers/intel/fsp2_0/fsp_debug_event.c b/src/drivers/intel/fsp2_0/fsp_debug_event.c index 2ebecc0..4b3e151a 100644 --- a/src/drivers/intel/fsp2_0/fsp_debug_event.c +++ b/src/drivers/intel/fsp2_0/fsp_debug_event.c @@ -17,7 +17,7 @@ return FSP_SUCCESS; } -efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, +__efiapi efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, efi_status_code_value_t ignored2, efi_uint32_t ignored3, efi_guid_t *ignored4, efi_status_code_data_t *data) { diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index 971be0d..7d7494c 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -8,6 +8,8 @@ #include <fsp/soc_binding.h> #include <soc/intel/common/mma.h> +#define __efiapi EFIAPI + #define FSP_SUCCESS EFI_SUCCESS #define FSP_INVALID_PARAMETER EFI_INVALID_PARAMETER #define FSP_DEVICE_ERROR EFI_DEVICE_ERROR diff --git a/src/drivers/intel/fsp2_0/include/fsp/debug.h b/src/drivers/intel/fsp2_0/include/fsp/debug.h index 044ce95..0e89fac 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/debug.h +++ b/src/drivers/intel/fsp2_0/include/fsp/debug.h @@ -19,14 +19,14 @@ void fsp_debug_before_memory_init(fsp_memory_init_fn memory_init, const FSPM_UPD *fspm_old_upd, const FSPM_UPD *fspm_new_upd); -void fsp_debug_after_memory_init(uint32_t status); +void fsp_debug_after_memory_init(efi_return_status_t status); void fsp_debug_before_silicon_init(fsp_silicon_init_fn silicon_init, const FSPS_UPD *fsps_old_upd, const FSPS_UPD *fsps_new_upd); -void fsp_debug_after_silicon_init(uint32_t status); +void fsp_debug_after_silicon_init(efi_return_status_t status); void fsp_before_debug_notify(fsp_notify_fn notify, const struct fsp_notify_params *notify_params); -void fsp_debug_after_notify(uint32_t status); +void fsp_debug_after_notify(efi_return_status_t status); void fspm_display_upd_values(const FSPM_UPD *old, const FSPM_UPD *new); void fsp_display_hobs(void); diff --git a/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h b/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h index 7d50c1e..b6f7a51 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h +++ b/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h @@ -8,11 +8,12 @@ * More details about this structure can be found here : * http://github.com/tianocore/edk2/blob/master/IntelFsp2Pkg/Include/FspEas/Fs… */ -#include <efi/efi_datatype.h> #include <fsp/soc_binding.h> +#include <fsp/api.h> +#include <efi/efi_datatype.h> /* fsp debug event handler */ -efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, +__efiapi efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, efi_status_code_value_t ignored2, efi_uint32_t ignored3, efi_guid_t *ignored4, efi_status_code_data_t *data); 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 563a7e6..e8d853b 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h +++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h @@ -12,7 +12,6 @@ #define FSP_HDR_ATTRIB_FSPS 3 #define FSP_IMAGE_ID_LENGTH 8 -#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct fsp_header { uint32_t signature; //FSPH uint32_t header_length; @@ -42,9 +41,6 @@ uint32_t fsp_smm_init_entry_offset; #endif } __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/soc_binding.h b/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h index ba3e289..f6369b5 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h +++ b/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h @@ -5,6 +5,18 @@ #include <stddef.h> +/* + * EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which + * is not working well when included by coreboot. While it has no side-effect on + * i386 because the C calling convention used by coreboot and FSP are the same, + * it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling + * convention while coreboot uses the System V AMD64 ABI. + * + * Fortunately, EDK2 header allows to override the EFIAPI. The __ms_abi__ + * attribute works for both i386 and x86_64. + */ +#define EFIAPI __attribute__((__ms_abi__)) + #pragma pack(push) /** diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index becf91a..f64fdc3 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -187,10 +187,10 @@ * SoC. If the requested status is not a reboot status or unhandled, this * function does nothing. */ -void fsp_handle_reset(uint32_t status); +void fsp_handle_reset(efi_return_status_t status); /* SoC/chipset must provide this to handle platform-specific reset codes */ -void chipset_handle_reset(uint32_t status); +void chipset_handle_reset(efi_return_status_t status); #if CONFIG(PLATFORM_USES_SECOND_FSP) /* The SoC must implement these to choose the appropriate FSP-M/FSP-S binary. */ @@ -207,12 +207,12 @@ } #endif -typedef asmlinkage uint32_t (*temp_ram_exit_fn)(void *param); -typedef asmlinkage uint32_t (*fsp_memory_init_fn) +typedef __efiapi efi_return_status_t (*temp_ram_exit_fn)(void *param); +typedef __efiapi efi_return_status_t (*fsp_memory_init_fn) (void *raminit_upd, void **hob_list); -typedef asmlinkage uint32_t (*fsp_silicon_init_fn)(void *silicon_upd); -typedef asmlinkage uint32_t (*fsp_multi_phase_init_fn)(struct fsp_multi_phase_params *); -typedef asmlinkage uint32_t (*fsp_notify_fn)(struct fsp_notify_params *); +typedef __efiapi efi_return_status_t (*fsp_silicon_init_fn)(void *silicon_upd); +typedef __efiapi efi_return_status_t (*fsp_multi_phase_init_fn)(struct fsp_multi_phase_params *); +typedef __efiapi efi_return_status_t (*fsp_notify_fn)(struct fsp_notify_params *); #include <fsp/debug.h> #endif /* _FSP2_0_UTIL_H_ */ diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index b48cc93..30b3f1d 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -34,7 +34,11 @@ /* Leave for the SoC/Mainboard to implement if necessary. */ } +#if CONFIG(PLATFORM_USES_FSP2_X86_32) static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t)); +#else +static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(16); +#endif /* * Helper function to store the MRC cache version into CBMEM @@ -275,15 +279,14 @@ return ver; } -static void error_handler(const char *context, uint32_t status, bool die_on_error) +static void error_handler(const char *context, efi_return_status_t status, bool die_on_error) { if (status == FSP_SUCCESS) return; fsp_handle_reset(status); if (die_on_error) - die_with_post_code(POSTCODE_RAM_FAILURE, - "%s returned with error 0x%zx!\n", + die_with_post_code(POSTCODE_RAM_FAILURE, "%s returned with error 0x%zx!\n", context, (size_t)status); printk(BIOS_SPEW, "%s returned 0x%zx\n", context, (size_t)status); @@ -292,7 +295,7 @@ #if CONFIG(PLATFORM_USES_FSP2_4) static void multi_phase_init(const struct fsp_header *hdr) { - uint32_t status; + efi_return_status_t status; fsp_multi_phase_init_fn fsp_multi_phase_init; struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number; @@ -333,7 +336,7 @@ static void do_fsp_memory_init(const struct fspm_context *context, bool s3wake) { - uint32_t status; + efi_return_status_t status; fsp_memory_init_fn fsp_raminit; FSPM_UPD fspm_upd, *upd; FSPM_ARCHx_UPD *arch_upd; diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service1.c b/src/drivers/intel/fsp2_0/ppi/mp_service1.c index 879a8e8..3c036c1 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service1.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service1.c @@ -6,14 +6,14 @@ typedef EFI_PEI_MP_SERVICES_PPI efi_pei_mp_services_ppi; -static efi_return_status_t mps1_get_number_of_processors(const +static __efiapi efi_return_status_t mps1_get_number_of_processors(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t *number_of_processors, efi_uintn_t *number_of_enabled_processors) { return mp_get_number_of_processors(number_of_processors, number_of_enabled_processors); } -static efi_return_status_t mps1_get_processor_info(const +static __efiapi efi_return_status_t mps1_get_processor_info(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) @@ -21,7 +21,7 @@ return mp_get_processor_info(processor_number, processor_info_buffer); } -static efi_return_status_t mps1_startup_all_aps(const +static __efiapi efi_return_status_t mps1_startup_all_aps(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_ap_procedure procedure, efi_boolean_t run_serial, efi_uintn_t timeout_usec, void *argument) @@ -29,7 +29,7 @@ return mp_startup_all_aps(procedure, run_serial, timeout_usec, argument); } -static efi_return_status_t mps1_startup_this_ap(const +static __efiapi efi_return_status_t mps1_startup_this_ap(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) @@ -37,21 +37,21 @@ return mp_startup_this_ap(procedure, processor_number, timeout_usec, argument); } -static efi_return_status_t mps1_switch_bsp(const efi_pei_services **ignored1, +static __efiapi efi_return_status_t mps1_switch_bsp(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t ignored3, efi_boolean_t ignored4) { return mp_api_unsupported(); } -static efi_return_status_t mps1_enable_disable_ap(const +static __efiapi efi_return_status_t mps1_enable_disable_ap(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t ignored3, efi_boolean_t ignored4, efi_uint32_t *ignored5) { return mp_api_unsupported(); } -static efi_return_status_t mps1_identify_processor(const +static __efiapi efi_return_status_t mps1_identify_processor(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t *processor_number) { diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service2.c b/src/drivers/intel/fsp2_0/ppi/mp_service2.c index fd91b46..28bcadf 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service2.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service2.c @@ -6,7 +6,7 @@ typedef EDKII_PEI_MP_SERVICES2_PPI efi_pei_mp_services_ppi; -static efi_return_status_t mps2_get_number_of_processors( +static __efiapi efi_return_status_t mps2_get_number_of_processors( efi_pei_mp_services_ppi *ignored1, efi_uintn_t *number_of_processors, efi_uintn_t *number_of_enabled_processors) @@ -14,7 +14,7 @@ return mp_get_number_of_processors(number_of_processors, number_of_enabled_processors); } -static efi_return_status_t mps2_get_processor_info( +static __efiapi efi_return_status_t mps2_get_processor_info( efi_pei_mp_services_ppi *ignored1, efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) @@ -22,7 +22,7 @@ return mp_get_processor_info(processor_number, processor_info_buffer); } -static efi_return_status_t mps2_startup_all_aps( +static __efiapi efi_return_status_t mps2_startup_all_aps( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_boolean_t run_serial, efi_uintn_t timeout_usec, void *argument) @@ -30,7 +30,7 @@ return mp_startup_all_aps(procedure, run_serial, timeout_usec, argument); } -static efi_return_status_t mps2_startup_all_cpus( +static __efiapi efi_return_status_t mps2_startup_all_cpus( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_uintn_t timeout_usec, void *argument) @@ -38,7 +38,7 @@ return mp_startup_all_cpus(procedure, timeout_usec, argument); } -static efi_return_status_t mps2_startup_this_ap( +static __efiapi efi_return_status_t mps2_startup_this_ap( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) @@ -46,21 +46,21 @@ return mp_startup_this_ap(procedure, processor_number, timeout_usec, argument); } -static efi_return_status_t mps2_switch_bsp( +static __efiapi efi_return_status_t mps2_switch_bsp( efi_pei_mp_services_ppi *ignored1, efi_uintn_t ignored2, efi_boolean_t ignored3) { return mp_api_unsupported(); } -static efi_return_status_t mps2_enable_disable_ap( +static __efiapi efi_return_status_t mps2_enable_disable_ap( efi_pei_mp_services_ppi *ignored1, efi_uintn_t ignored2, efi_boolean_t ignored3, efi_uint32_t *ignored4) { return mp_api_unsupported(); } -static efi_return_status_t mps2_identify_processor( +static __efiapi efi_return_status_t mps2_identify_processor( efi_pei_mp_services_ppi *ignored1, efi_uintn_t *processor_number) { diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index 69b626b..4e48085 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -36,7 +36,8 @@ FSP_MULTI_PHASE_SI_INIT_EXECUTE_PHASE_API }; -static void fsps_return_value_handler(enum fsp_silicon_init_phases phases, uint32_t status) +static void fsps_return_value_handler(enum fsp_silicon_init_phases phases, + efi_return_status_t status) { uint8_t postcode; @@ -54,16 +55,17 @@ switch (phases) { case FSP_SILICON_INIT_API: - die_with_post_code(postcode, "FspSiliconInit returned with error 0x%08x\n", - status); + die_with_post_code(postcode, + "FspSiliconInit returned with error 0x%zx\n", + (size_t)status); break; case FSP_MULTI_PHASE_SI_INIT_GET_NUMBER_OF_PHASES_API: - printk(BIOS_SPEW, "FspMultiPhaseSiInit NumberOfPhases returned 0x%08x\n", - status); + printk(BIOS_SPEW, "FspMultiPhaseSiInit NumberOfPhases returned 0x%zx\n", + (size_t)status); break; case FSP_MULTI_PHASE_SI_INIT_EXECUTE_PHASE_API: - printk(BIOS_SPEW, "FspMultiPhaseSiInit ExecutePhase returned 0x%08x\n", - status); + printk(BIOS_SPEW, "FspMultiPhaseSiInit ExecutePhase returned 0x%zx\n", + (size_t)status); break; default: break; @@ -88,7 +90,7 @@ { FSPS_UPD *upd, *supd; fsp_silicon_init_fn silicon_init; - uint32_t status; + efi_return_status_t status; fsp_multi_phase_init_fn multi_phase_si_init; struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number; @@ -139,7 +141,7 @@ status = silicon_init(upd); null_breakpoint_init(); - printk(BIOS_INFO, "FSPS returned %x\n", status); + printk(BIOS_INFO, "FSPS returned %zx\n", (size_t)status); timestamp_add_now(TS_FSP_SILICON_INIT_END); post_code(POSTCODE_FSP_SILICON_EXIT); diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index f912d9a..fab9958 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -83,18 +83,18 @@ return CB_SUCCESS; } -static bool fsp_reset_requested(uint32_t status) +static bool fsp_reset_requested(efi_return_status_t status) { return (status >= FSP_STATUS_RESET_REQUIRED_COLD && status <= FSP_STATUS_RESET_REQUIRED_8); } -void fsp_handle_reset(uint32_t status) +void fsp_handle_reset(efi_return_status_t status) { if (!fsp_reset_requested(status)) return; - printk(BIOS_SPEW, "FSP: handling reset type %x\n", status); + printk(BIOS_SPEW, "FSP: handling reset type %zx\n", (size_t)status); switch (status) { case FSP_STATUS_RESET_REQUIRED_COLD: diff --git a/src/include/efi/efi_datatype.h b/src/include/efi/efi_datatype.h index 917d0c8..0c33dcd 100644 --- a/src/include/efi/efi_datatype.h +++ b/src/include/efi/efi_datatype.h @@ -76,4 +76,16 @@ void (EFIAPI *efi_ap_procedure)(void *buffer); +/* + * EFI_STATUS/efi_return_status_t size changes with the architecture (32-bits vs + * 64-bits). To print statuses independently of the architecture we leverage the + * size_t 'z' print format which is generally aligned with the architecture size + * too. + * + * This assert is intended to detect unexpected mismatch which could lead to + * incorrect logs. + */ +_Static_assert(sizeof(size_t) == sizeof(efi_return_status_t), + "Unexpected EFI_STATUS size"); + #endif diff --git a/src/soc/intel/common/fsp_reset.c b/src/soc/intel/common/fsp_reset.c index 2bb39bd..0765710 100644 --- a/src/soc/intel/common/fsp_reset.c +++ b/src/soc/intel/common/fsp_reset.c @@ -30,14 +30,14 @@ struct pch_reset_data reset_data; }; -void chipset_handle_reset(uint32_t status) +void chipset_handle_reset(efi_return_status_t status) { if (status == CONFIG_FSP_STATUS_GLOBAL_RESET) { printk(BIOS_DEBUG, "GLOBAL RESET!\n"); global_reset(); } - printk(BIOS_ERR, "unhandled reset type %x\n", status); + printk(BIOS_ERR, "unhandled reset type %zx\n", (size_t)status); die("unknown reset type"); } -- To view, visit https://review.coreboot.org/c/coreboot/+/80277?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Gerrit-Change-Number: 80277 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support
by Jérémy Compostella (Code Review) 01 Feb '24

01 Feb '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Subrata Banik, Tarun, Wonkyu Kim. Hello Andrey Petrov, Bora Guvendik, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Tarun, Wonkyu Kim, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80275?usp=email to look at the new patch set (#6). Change subject: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support ...................................................................... drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support Intel Firmware Support Package 2.4 specification brings some significant changes compared to version 2.3 (cf. documents 736809 and 644852 respectively): 1. It supports FSP-M multi-phase init. Some fields have been added to the FSP header data structure for this purpose. 2. The `FSPM_ARCH2_UPD' and `FSPS_ARCH2_UPD' data structures must be used in place of `FSPM_ARCH_UPD' and `FSPS_ARCH_UPD' respectively. 3. It support 64-bits FSP but 64-bits support will be provided by subsequent patch. Note that similarly to what is done for silicon initialization, timestamps and post-codes are used during the memory initialization multi-phase. However, since post-codes are in short supply, memory and silicon multi-phase init share the same post-codes. Change-Id: I1c24d26e105c3dcbd9cca0e7197ab1362344aa97 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/commonlib/include/commonlib/console/post_codes.h M src/commonlib/include/commonlib/timestamp_serialized.h M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/upd_display.c M src/soc/intel/alderlake/fsp_params.c M src/soc/intel/meteorlake/fsp_params.c M src/soc/intel/tigerlake/fsp_params.c M util/cbfstool/eventlog.c 13 files changed, 128 insertions(+), 35 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/80275/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/80275?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: I1c24d26e105c3dcbd9cca0e7197ab1362344aa97 Gerrit-Change-Number: 80275 Gerrit-PatchSet: 6 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Tarun <tstuli(a)gmail.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Tarun <tstuli(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 31 Jan '24

31 Jan '24

Attention is currently required from: Andrey Petrov, Ronak Kanabar. Jérémy Compostella has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80276?usp=email ) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... drivers/intel/fsp2_0: Support FSP 2.4 64-bits FSP 2.4 brings FSP 64-bits support which requires some adjustments in coreboot: - Stack alignment: 1. FSP functions must be called with the stack 16-bytes aligned. This is already setup properly with the default value of the `mpreferred-stack-boundary' compiler option (4). 2. The FSP stack buffer supplied by coreboot through the `StackBase' UPD must be 16-bytes aligned. - The EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which is not working well when included by coreboot. While it has no side-effect on i386 because the C calling convention used by coreboot and FSP are the same, it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling convention while coreboot uses the System V AMD64 ABI. Fortunately, EDK2 header allows to override the EFIAPI definition. The __ms_abi__ attribute works for both i386 and x86_64. This attribute has to be set to all functions calling or called by the FSP. - The EFI_STATUS/efi_return_status_t size changes with the architecture (32-bits vs 64-bits). To print statuses independently of the architecture we leverage the size_t 'z' print format which is generally aligned with the architecture size too. In addition, this commit sets`PLATFORM_USES_FSP2_X86_32' to `n' by default if FSP 2.4 is enabled as 64-bits FSP should be norm moving forward. Change-Id: I3fa012fa6f98e4bb3c2a033189554ad5a6ddfced Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/debug.c M src/drivers/intel/fsp2_0/fsp_debug_event.c M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/include/fsp/debug.h M src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/soc_binding.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/ppi/mp_service1.c M src/drivers/intel/fsp2_0/ppi/mp_service2.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/efi/efi_datatype.h M src/soc/intel/common/fsp_reset.c 16 files changed, 90 insertions(+), 57 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/80276/1 diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 2c8bdc4..61d68f6 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -55,10 +55,11 @@ config PLATFORM_USES_FSP2_X86_32 bool + default n if PLATFORM_USES_FSP2_4 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. + Specify if the FSP binaries are 32-bits (yes) or 64-bits + (no). Starting with FSP specification 2.4 they can be 64-bits. config HAVE_INTEL_FSP_REPO bool diff --git a/src/drivers/intel/fsp2_0/debug.c b/src/drivers/intel/fsp2_0/debug.c index 8c0d67d..1152413 100644 --- a/src/drivers/intel/fsp2_0/debug.c +++ b/src/drivers/intel/fsp2_0/debug.c @@ -91,10 +91,10 @@ printk(BIOS_SPEW, "\t%p: &hob_list_ptr\n", fsp_get_hob_list_ptr()); } -void fsp_debug_after_memory_init(uint32_t status) +void fsp_debug_after_memory_init(efi_return_status_t status) { if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspMemoryInit returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspMemoryInit returned 0x%zx\n", (size_t)status); if (status != FSP_SUCCESS) return; @@ -136,13 +136,14 @@ printk(BIOS_SPEW, "\t%p: upd\n", fsps_new_upd); } -void fsp_debug_after_silicon_init(uint32_t status) +void fsp_debug_after_silicon_init(efi_return_status_t status) { if (CONFIG(CHECK_GPIO_CONFIG_CHANGES)) fsp_gpio_config_check(AFTER_FSP_CALL, "FSP Silicon Init"); if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspSiliconInit returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspSiliconInit returned 0x%zx\n", + (size_t)status); /* Display the HOBs */ if (CONFIG(DISPLAY_HOBS)) @@ -170,13 +171,14 @@ printk(BIOS_SPEW, "\t%p: notify_params\n", notify_params); } -void fsp_debug_after_notify(uint32_t status) +void fsp_debug_after_notify(efi_return_status_t status) { if (CONFIG(CHECK_GPIO_CONFIG_CHANGES)) fsp_gpio_config_check(AFTER_FSP_CALL, "FSP Notify"); if (CONFIG(DISPLAY_FSP_CALLS_AND_STATUS)) - printk(BIOS_SPEW, "FspNotify returned 0x%08x\n", status); + printk(BIOS_SPEW, "FspNotify returned 0x%zx\n", + (size_t)status); /* Display the HOBs */ if (CONFIG(DISPLAY_HOBS)) diff --git a/src/drivers/intel/fsp2_0/fsp_debug_event.c b/src/drivers/intel/fsp2_0/fsp_debug_event.c index 2ebecc0..4b3e151a 100644 --- a/src/drivers/intel/fsp2_0/fsp_debug_event.c +++ b/src/drivers/intel/fsp2_0/fsp_debug_event.c @@ -17,7 +17,7 @@ return FSP_SUCCESS; } -efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, +__efiapi efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, efi_status_code_value_t ignored2, efi_uint32_t ignored3, efi_guid_t *ignored4, efi_status_code_data_t *data) { diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index 971be0d..7d7494c 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -8,6 +8,8 @@ #include <fsp/soc_binding.h> #include <soc/intel/common/mma.h> +#define __efiapi EFIAPI + #define FSP_SUCCESS EFI_SUCCESS #define FSP_INVALID_PARAMETER EFI_INVALID_PARAMETER #define FSP_DEVICE_ERROR EFI_DEVICE_ERROR diff --git a/src/drivers/intel/fsp2_0/include/fsp/debug.h b/src/drivers/intel/fsp2_0/include/fsp/debug.h index 044ce95..0e89fac 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/debug.h +++ b/src/drivers/intel/fsp2_0/include/fsp/debug.h @@ -19,14 +19,14 @@ void fsp_debug_before_memory_init(fsp_memory_init_fn memory_init, const FSPM_UPD *fspm_old_upd, const FSPM_UPD *fspm_new_upd); -void fsp_debug_after_memory_init(uint32_t status); +void fsp_debug_after_memory_init(efi_return_status_t status); void fsp_debug_before_silicon_init(fsp_silicon_init_fn silicon_init, const FSPS_UPD *fsps_old_upd, const FSPS_UPD *fsps_new_upd); -void fsp_debug_after_silicon_init(uint32_t status); +void fsp_debug_after_silicon_init(efi_return_status_t status); void fsp_before_debug_notify(fsp_notify_fn notify, const struct fsp_notify_params *notify_params); -void fsp_debug_after_notify(uint32_t status); +void fsp_debug_after_notify(efi_return_status_t status); void fspm_display_upd_values(const FSPM_UPD *old, const FSPM_UPD *new); void fsp_display_hobs(void); diff --git a/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h b/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h index 7d50c1e..b6f7a51 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h +++ b/src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h @@ -8,11 +8,12 @@ * More details about this structure can be found here : * http://github.com/tianocore/edk2/blob/master/IntelFsp2Pkg/Include/FspEas/Fs… */ -#include <efi/efi_datatype.h> #include <fsp/soc_binding.h> +#include <fsp/api.h> +#include <efi/efi_datatype.h> /* fsp debug event handler */ -efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, +__efiapi efi_return_status_t fsp_debug_event_handler(efi_status_code_type_t ignored1, efi_status_code_value_t ignored2, efi_uint32_t ignored3, efi_guid_t *ignored4, efi_status_code_data_t *data); 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 563a7e6..e8d853b 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h +++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h @@ -12,7 +12,6 @@ #define FSP_HDR_ATTRIB_FSPS 3 #define FSP_IMAGE_ID_LENGTH 8 -#if CONFIG(PLATFORM_USES_FSP2_X86_32) struct fsp_header { uint32_t signature; //FSPH uint32_t header_length; @@ -42,9 +41,6 @@ uint32_t fsp_smm_init_entry_offset; #endif } __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/soc_binding.h b/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h index ba3e289..f6369b5 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h +++ b/src/drivers/intel/fsp2_0/include/fsp/soc_binding.h @@ -5,6 +5,18 @@ #include <stddef.h> +/* + * EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which + * is not working well when included by coreboot. While it has no side-effect on + * i386 because the C calling convention used by coreboot and FSP are the same, + * it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling + * convention while coreboot uses the System V AMD64 ABI. + * + * Fortunately, EDK2 header allows to override the EFIAPI. The __ms_abi__ + * attribute works for both i386 and x86_64. + */ +#define EFIAPI __attribute__((__ms_abi__)) + #pragma pack(push) /** diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h index becf91a..f64fdc3 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/util.h +++ b/src/drivers/intel/fsp2_0/include/fsp/util.h @@ -187,10 +187,10 @@ * SoC. If the requested status is not a reboot status or unhandled, this * function does nothing. */ -void fsp_handle_reset(uint32_t status); +void fsp_handle_reset(efi_return_status_t status); /* SoC/chipset must provide this to handle platform-specific reset codes */ -void chipset_handle_reset(uint32_t status); +void chipset_handle_reset(efi_return_status_t status); #if CONFIG(PLATFORM_USES_SECOND_FSP) /* The SoC must implement these to choose the appropriate FSP-M/FSP-S binary. */ @@ -207,12 +207,12 @@ } #endif -typedef asmlinkage uint32_t (*temp_ram_exit_fn)(void *param); -typedef asmlinkage uint32_t (*fsp_memory_init_fn) +typedef __efiapi efi_return_status_t (*temp_ram_exit_fn)(void *param); +typedef __efiapi efi_return_status_t (*fsp_memory_init_fn) (void *raminit_upd, void **hob_list); -typedef asmlinkage uint32_t (*fsp_silicon_init_fn)(void *silicon_upd); -typedef asmlinkage uint32_t (*fsp_multi_phase_init_fn)(struct fsp_multi_phase_params *); -typedef asmlinkage uint32_t (*fsp_notify_fn)(struct fsp_notify_params *); +typedef __efiapi efi_return_status_t (*fsp_silicon_init_fn)(void *silicon_upd); +typedef __efiapi efi_return_status_t (*fsp_multi_phase_init_fn)(struct fsp_multi_phase_params *); +typedef __efiapi efi_return_status_t (*fsp_notify_fn)(struct fsp_notify_params *); #include <fsp/debug.h> #endif /* _FSP2_0_UTIL_H_ */ diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index b48cc93..fb1a5eb 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -34,7 +34,11 @@ /* Leave for the SoC/Mainboard to implement if necessary. */ } +#if CONFIG(PLATFORM_USES_FSP2_X86_32) static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t)); +#else +static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(16); +#endif /* * Helper function to store the MRC cache version into CBMEM @@ -275,7 +279,8 @@ return ver; } -static void error_handler(const char *context, uint32_t status, bool die_on_error) +static void error_handler(const char *context, efi_return_status_t status, + bool die_on_error) { if (status == FSP_SUCCESS) return; @@ -292,7 +297,7 @@ #if CONFIG(PLATFORM_USES_FSP2_4) static void multi_phase_init(const struct fsp_header *hdr) { - uint32_t status; + efi_return_status_t status; fsp_multi_phase_init_fn fsp_multi_phase_init; struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number; @@ -333,7 +338,7 @@ static void do_fsp_memory_init(const struct fspm_context *context, bool s3wake) { - uint32_t status; + efi_return_status_t status; fsp_memory_init_fn fsp_raminit; FSPM_UPD fspm_upd, *upd; FSPM_ARCHx_UPD *arch_upd; diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service1.c b/src/drivers/intel/fsp2_0/ppi/mp_service1.c index 879a8e8..3c036c1 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service1.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service1.c @@ -6,14 +6,14 @@ typedef EFI_PEI_MP_SERVICES_PPI efi_pei_mp_services_ppi; -static efi_return_status_t mps1_get_number_of_processors(const +static __efiapi efi_return_status_t mps1_get_number_of_processors(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t *number_of_processors, efi_uintn_t *number_of_enabled_processors) { return mp_get_number_of_processors(number_of_processors, number_of_enabled_processors); } -static efi_return_status_t mps1_get_processor_info(const +static __efiapi efi_return_status_t mps1_get_processor_info(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) @@ -21,7 +21,7 @@ return mp_get_processor_info(processor_number, processor_info_buffer); } -static efi_return_status_t mps1_startup_all_aps(const +static __efiapi efi_return_status_t mps1_startup_all_aps(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_ap_procedure procedure, efi_boolean_t run_serial, efi_uintn_t timeout_usec, void *argument) @@ -29,7 +29,7 @@ return mp_startup_all_aps(procedure, run_serial, timeout_usec, argument); } -static efi_return_status_t mps1_startup_this_ap(const +static __efiapi efi_return_status_t mps1_startup_this_ap(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) @@ -37,21 +37,21 @@ return mp_startup_this_ap(procedure, processor_number, timeout_usec, argument); } -static efi_return_status_t mps1_switch_bsp(const efi_pei_services **ignored1, +static __efiapi efi_return_status_t mps1_switch_bsp(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t ignored3, efi_boolean_t ignored4) { return mp_api_unsupported(); } -static efi_return_status_t mps1_enable_disable_ap(const +static __efiapi efi_return_status_t mps1_enable_disable_ap(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t ignored3, efi_boolean_t ignored4, efi_uint32_t *ignored5) { return mp_api_unsupported(); } -static efi_return_status_t mps1_identify_processor(const +static __efiapi efi_return_status_t mps1_identify_processor(const efi_pei_services **ignored1, efi_pei_mp_services_ppi *ignored2, efi_uintn_t *processor_number) { diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service2.c b/src/drivers/intel/fsp2_0/ppi/mp_service2.c index fd91b46..28bcadf 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service2.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service2.c @@ -6,7 +6,7 @@ typedef EDKII_PEI_MP_SERVICES2_PPI efi_pei_mp_services_ppi; -static efi_return_status_t mps2_get_number_of_processors( +static __efiapi efi_return_status_t mps2_get_number_of_processors( efi_pei_mp_services_ppi *ignored1, efi_uintn_t *number_of_processors, efi_uintn_t *number_of_enabled_processors) @@ -14,7 +14,7 @@ return mp_get_number_of_processors(number_of_processors, number_of_enabled_processors); } -static efi_return_status_t mps2_get_processor_info( +static __efiapi efi_return_status_t mps2_get_processor_info( efi_pei_mp_services_ppi *ignored1, efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) @@ -22,7 +22,7 @@ return mp_get_processor_info(processor_number, processor_info_buffer); } -static efi_return_status_t mps2_startup_all_aps( +static __efiapi efi_return_status_t mps2_startup_all_aps( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_boolean_t run_serial, efi_uintn_t timeout_usec, void *argument) @@ -30,7 +30,7 @@ return mp_startup_all_aps(procedure, run_serial, timeout_usec, argument); } -static efi_return_status_t mps2_startup_all_cpus( +static __efiapi efi_return_status_t mps2_startup_all_cpus( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_uintn_t timeout_usec, void *argument) @@ -38,7 +38,7 @@ return mp_startup_all_cpus(procedure, timeout_usec, argument); } -static efi_return_status_t mps2_startup_this_ap( +static __efiapi efi_return_status_t mps2_startup_this_ap( efi_pei_mp_services_ppi *ignored1, efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) @@ -46,21 +46,21 @@ return mp_startup_this_ap(procedure, processor_number, timeout_usec, argument); } -static efi_return_status_t mps2_switch_bsp( +static __efiapi efi_return_status_t mps2_switch_bsp( efi_pei_mp_services_ppi *ignored1, efi_uintn_t ignored2, efi_boolean_t ignored3) { return mp_api_unsupported(); } -static efi_return_status_t mps2_enable_disable_ap( +static __efiapi efi_return_status_t mps2_enable_disable_ap( efi_pei_mp_services_ppi *ignored1, efi_uintn_t ignored2, efi_boolean_t ignored3, efi_uint32_t *ignored4) { return mp_api_unsupported(); } -static efi_return_status_t mps2_identify_processor( +static __efiapi efi_return_status_t mps2_identify_processor( efi_pei_mp_services_ppi *ignored1, efi_uintn_t *processor_number) { diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index c1953b4..8a51d88 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -36,7 +36,8 @@ FSP_MULTI_PHASE_SI_INIT_EXECUTE_PHASE_API }; -static void fsps_return_value_handler(enum fsp_silicon_init_phases phases, uint32_t status) +static void fsps_return_value_handler(enum fsp_silicon_init_phases phases, + efi_return_status_t status) { uint8_t postcode; @@ -54,16 +55,17 @@ switch (phases) { case FSP_SILICON_INIT_API: - die_with_post_code(postcode, "FspSiliconInit returned with error 0x%08x\n", - status); + die_with_post_code(postcode, + "FspSiliconInit returned with error 0x%zx\n", + (size_t)status); break; case FSP_MULTI_PHASE_SI_INIT_GET_NUMBER_OF_PHASES_API: - printk(BIOS_SPEW, "FspMultiPhaseSiInit NumberOfPhases returned 0x%08x\n", - status); + printk(BIOS_SPEW, "FspMultiPhaseSiInit NumberOfPhases returned 0x%zx\n", + (size_t)status); break; case FSP_MULTI_PHASE_SI_INIT_EXECUTE_PHASE_API: - printk(BIOS_SPEW, "FspMultiPhaseSiInit ExecutePhase returned 0x%08x\n", - status); + printk(BIOS_SPEW, "FspMultiPhaseSiInit ExecutePhase returned 0x%zx\n", + (size_t)status); break; default: break; @@ -88,7 +90,7 @@ { FSPS_UPD *upd, *supd; fsp_silicon_init_fn silicon_init; - uint32_t status; + efi_return_status_t status; fsp_multi_phase_init_fn multi_phase_si_init; struct fsp_multi_phase_params multi_phase_params; struct fsp_multi_phase_get_number_of_phases_params multi_phase_get_number; @@ -139,7 +141,7 @@ status = silicon_init(upd); null_breakpoint_init(); - printk(BIOS_INFO, "FSPS returned %x\n", status); + printk(BIOS_INFO, "FSPS returned %zx\n", (size_t)status); timestamp_add_now(TS_FSP_SILICON_INIT_END); post_code(POSTCODE_FSP_SILICON_EXIT); diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index f912d9a..fab9958 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -83,18 +83,18 @@ return CB_SUCCESS; } -static bool fsp_reset_requested(uint32_t status) +static bool fsp_reset_requested(efi_return_status_t status) { return (status >= FSP_STATUS_RESET_REQUIRED_COLD && status <= FSP_STATUS_RESET_REQUIRED_8); } -void fsp_handle_reset(uint32_t status) +void fsp_handle_reset(efi_return_status_t status) { if (!fsp_reset_requested(status)) return; - printk(BIOS_SPEW, "FSP: handling reset type %x\n", status); + printk(BIOS_SPEW, "FSP: handling reset type %zx\n", (size_t)status); switch (status) { case FSP_STATUS_RESET_REQUIRED_COLD: diff --git a/src/include/efi/efi_datatype.h b/src/include/efi/efi_datatype.h index 917d0c8..0c33dcd 100644 --- a/src/include/efi/efi_datatype.h +++ b/src/include/efi/efi_datatype.h @@ -76,4 +76,16 @@ void (EFIAPI *efi_ap_procedure)(void *buffer); +/* + * EFI_STATUS/efi_return_status_t size changes with the architecture (32-bits vs + * 64-bits). To print statuses independently of the architecture we leverage the + * size_t 'z' print format which is generally aligned with the architecture size + * too. + * + * This assert is intended to detect unexpected mismatch which could lead to + * incorrect logs. + */ +_Static_assert(sizeof(size_t) == sizeof(efi_return_status_t), + "Unexpected EFI_STATUS size"); + #endif diff --git a/src/soc/intel/common/fsp_reset.c b/src/soc/intel/common/fsp_reset.c index 2bb39bd..0765710 100644 --- a/src/soc/intel/common/fsp_reset.c +++ b/src/soc/intel/common/fsp_reset.c @@ -30,14 +30,14 @@ struct pch_reset_data reset_data; }; -void chipset_handle_reset(uint32_t status) +void chipset_handle_reset(efi_return_status_t status) { if (status == CONFIG_FSP_STATUS_GLOBAL_RESET) { printk(BIOS_DEBUG, "GLOBAL RESET!\n"); global_reset(); } - printk(BIOS_ERR, "unhandled reset type %x\n", status); + printk(BIOS_ERR, "unhandled reset type %zx\n", (size_t)status); die("unknown reset type"); } -- To view, visit https://review.coreboot.org/c/coreboot/+/80276?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: I3fa012fa6f98e4bb3c2a033189554ad5a6ddfced Gerrit-Change-Number: 80276 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2024