Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
drivers/intel/fsp1_1: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols.
Tested on Facebook FBG1701
Change-Id: Ice748e542180f6e1dc1505e7f37b6b6c68772bda Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S A src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp1_1/include/fsp/util.h 4 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47758/1
diff --git a/src/drivers/intel/fsp1_1/Makefile.inc b/src/drivers/intel/fsp1_1/Makefile.inc index 5fc100a..dc9dcb3 100644 --- a/src/drivers/intel/fsp1_1/Makefile.inc +++ b/src/drivers/intel/fsp1_1/Makefile.inc @@ -7,6 +7,7 @@ verstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += verstage.c
bootblock-$(CONFIG_USE_GENERIC_FSP_CAR_INC) += cache_as_ram.S +bootblock-y += fsp_report.c bootblock-y += fsp_util.c bootblock-y += ../../../cpu/intel/microcode/microcode_asm.S
diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index 31c3580..07e91d4 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -144,6 +144,8 @@ * mm0: low 32-bits of TSC value * mm1: high 32-bits of TSC value */ + movl %edx, temp_memory_end + movl %ecx, temp_memory_start
/* coreboot assumes stack/heap region will be zero */ cld diff --git a/src/drivers/intel/fsp1_1/fsp_report.c b/src/drivers/intel/fsp1_1/fsp_report.c new file mode 100644 index 0000000..0103cfa --- /dev/null +++ b/src/drivers/intel/fsp1_1/fsp_report.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/symbols.h> +#include <console/console.h> +#include <fsp/util.h> + +/* filled in assembly after FSP-T ran */ +uintptr_t temp_memory_start; +uintptr_t temp_memory_end; + +void report_fsp_output(void) +{ + const struct region fsp_car_region = { + .offset = temp_memory_start, + .size = temp_memory_end - temp_memory_start, + }; + const struct region coreboot_car_region = { + .offset = (uintptr_t)_car_region_start, + .size = (uintptr_t)_car_region_size, + }; + printk(BIOS_DEBUG, "FSP: reported temp_mem region: [0x%08lx,0x%08lx)\n", + temp_memory_start, temp_memory_end); + if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { + printk(BIOS_ERR, "Wrong CAR region used!\n"); + printk(BIOS_ERR, "Adapt CONFIG_DCACHE_RAM_BASE and CONFIG_DCACHE_RAM_SIZE to match FSP-T\n"); + } +} diff --git a/src/drivers/intel/fsp1_1/include/fsp/util.h b/src/drivers/intel/fsp1_1/include/fsp/util.h index 41ffedd..cab867a 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/util.h +++ b/src/drivers/intel/fsp1_1/include/fsp/util.h @@ -31,6 +31,7 @@ void *get_first_resource_hob(const EFI_GUID *guid); void fsp_display_upd_value(const char *name, uint32_t size, uint64_t old, uint64_t new); +void report_fsp_output(void);
/* Return version of FSP associated with fih. */ static inline uint32_t fsp_version(FSP_INFO_HEADER *fih)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/fs... File src/drivers/intel/fsp1_1/fsp_report.c:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/fs... PS1, Line 25: CONFIG_ No need to print CONFIG_
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... PS1, Line 147: movl %edx, temp_memory_end : movl %ecx, temp_memory_start These variables are in .bss which is cleared below. Either you push them on the stack and only .bss below or you temporarily save them in different registers.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Lee Leahy, Angel Pons, Huang Jin, Arthur Heymans, Patrick Rudolph, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47758
to look at the new patch set (#2).
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
drivers/intel/fsp1_1: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols.
Tested on Facebook FBG1701
Change-Id: Ice748e542180f6e1dc1505e7f37b6b6c68772bda Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S A src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp1_1/include/fsp/util.h 4 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47758/2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... PS1, Line 147: movl %edx, temp_memory_end : movl %ecx, temp_memory_start
These variables are in .bss which is cleared below. Either you push them on the stack and only . […]
Sorry, my mistake. Forgot to include push/pop in this patchset.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Lee Leahy, Angel Pons, Huang Jin, Arthur Heymans, Patrick Rudolph, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47758
to look at the new patch set (#3).
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
drivers/intel/fsp1_1: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols.
Tested on Facebook FBG1701
Change-Id: Ice748e542180f6e1dc1505e7f37b6b6c68772bda Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S A src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp1_1/include/fsp/util.h 4 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47758/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... PS1, Line 147: movl %edx, temp_memory_end : movl %ecx, temp_memory_start
Sorry, my mistake. Forgot to include push/pop in this patchset.
You want CB:47760 in first btw.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/fs... File src/drivers/intel/fsp1_1/fsp_report.c:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/fs... PS1, Line 25: CONFIG_
No need to print CONFIG_
Updated.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Lee Leahy, Angel Pons, Huang Jin, Arthur Heymans, Patrick Rudolph, Wim Vervoorn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47758
to look at the new patch set (#4).
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
drivers/intel/fsp1_1: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols.
Tested on Facebook FBG1701
Change-Id: Ice748e542180f6e1dc1505e7f37b6b6c68772bda Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S A src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp1_1/include/fsp/util.h 4 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/47758/4
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47758/1/src/drivers/intel/fsp1_1/ca... PS1, Line 147: movl %edx, temp_memory_end : movl %ecx, temp_memory_start
You want CB:47760 in first btw.
Yes. I will create relation chain
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47758 )
Change subject: drivers/intel/fsp1_1: Add function to report FSP-T output ......................................................................
drivers/intel/fsp1_1: Add function to report FSP-T output
This allows to compare the FSP-T output in %ecx and %edx to coreboot's CAR symbols.
Tested on Facebook FBG1701
Change-Id: Ice748e542180f6e1dc1505e7f37b6b6c68772bda Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47758 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S A src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp1_1/include/fsp/util.h 4 files changed, 42 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/Makefile.inc b/src/drivers/intel/fsp1_1/Makefile.inc index 5fc100a..abac7fb 100644 --- a/src/drivers/intel/fsp1_1/Makefile.inc +++ b/src/drivers/intel/fsp1_1/Makefile.inc @@ -9,6 +9,7 @@ bootblock-$(CONFIG_USE_GENERIC_FSP_CAR_INC) += cache_as_ram.S bootblock-y += fsp_util.c bootblock-y += ../../../cpu/intel/microcode/microcode_asm.S +bootblock-y += fsp_report.c
romstage-y += car.c romstage-y += fsp_util.c diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index b5b47ce..f2d29aa 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -145,6 +145,14 @@ * mm1: high 32-bits of TSC value */
+ /* + * temp_memory_start/end reside in the .bss section, which gets cleared + * below. Save the FSP return value to the stack before writing those + * variables. + */ + push %ecx + push %edx + /* clear .bss section */ cld xor %eax, %eax @@ -154,6 +162,11 @@ shrl $2, %ecx rep stosl
+ pop %edx + movl %edx, temp_memory_end + pop %ecx + movl %ecx, temp_memory_start + /* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ andl $0xfffffff0, %esp diff --git a/src/drivers/intel/fsp1_1/fsp_report.c b/src/drivers/intel/fsp1_1/fsp_report.c new file mode 100644 index 0000000..884218d --- /dev/null +++ b/src/drivers/intel/fsp1_1/fsp_report.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/symbols.h> +#include <console/console.h> +#include <fsp/util.h> + +/* filled in assembly after FSP-T ran */ +uintptr_t temp_memory_start; +uintptr_t temp_memory_end; + +void report_fsp_output(void) +{ + const struct region fsp_car_region = { + .offset = temp_memory_start, + .size = temp_memory_end - temp_memory_start, + }; + const struct region coreboot_car_region = { + .offset = (uintptr_t)_car_region_start, + .size = (uintptr_t)_car_region_size, + }; + printk(BIOS_DEBUG, "FSP: reported temp_mem region: [0x%08lx,0x%08lx)\n", + temp_memory_start, temp_memory_end); + if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { + printk(BIOS_ERR, "Wrong CAR region used!\n"); + printk(BIOS_ERR, "Adapt DCACHE_RAM_BASE and DCACHE_RAM_SIZE to match FSP-T\n"); + } +} diff --git a/src/drivers/intel/fsp1_1/include/fsp/util.h b/src/drivers/intel/fsp1_1/include/fsp/util.h index 41ffedd..cab867a 100644 --- a/src/drivers/intel/fsp1_1/include/fsp/util.h +++ b/src/drivers/intel/fsp1_1/include/fsp/util.h @@ -31,6 +31,7 @@ void *get_first_resource_hob(const EFI_GUID *guid); void fsp_display_upd_value(const char *name, uint32_t size, uint64_t old, uint64_t new); +void report_fsp_output(void);
/* Return version of FSP associated with fih. */ static inline uint32_t fsp_version(FSP_INFO_HEADER *fih)
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Frans Hendriks (Code Review)
-
Patrick Georgi (Code Review)