Change in coreboot[master]: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
[POC,RFC] Pass cbmem_top from romstage to relocatable stages param
Currently all stages that need cbmem need an implementation of a cbmem_top function. On FSP and AGESA platforms this proves to be painful and using the top of lower memory if often passed via lower memory or via a PCI scratchpad register.
The problem with writing to lower memory is that also need to be written on S3 as one cannot assume it to be still there. Writing things on S3 is always a fragile thing to do.
A better and more generic solution is to pass along cbmem_top via relocatable module parameters.
TODO: other platforms, nb/intel/i945 only at the moment.
Tested on Thinkpad X60.
Change-Id: I3195560f110720959fd7389b366d001bfde4da05 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S A src/arch/x86/cbmem_top.c M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/lib/program.ld M src/northbridge/intel/i945/memmap.c 8 files changed, 64 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/36140/1
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 612424d..8d90fc5 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -277,6 +277,7 @@ postcar-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c postcar-y += postcar.c postcar-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c +postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_top.c
LDFLAGS_postcar += -Map $(objcbfs)/postcar.map
@@ -330,6 +331,7 @@ ramstage-$(CONFIG_COOP_MULTITASKING) += thread_switch.S ramstage-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c ramstage-$(CONFIG_HAVE_ACPI_RESUME) += wakeup.S +ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_top.c
ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) rmodules_x86_32-y += memcpy.c diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S index 32b848d..8112b31 100644 --- a/src/arch/x86/c_start.S +++ b/src/arch/x86/c_start.S @@ -13,6 +13,15 @@
#include <cpu/x86/post_code.h>
+ .section ".module_parameters", "aw", @progbits +.global cbmem_top_ptr +cbmem_top_ptr: +#if defined(__X86_64__) + .quad 0 +#else + .long 0 +#endif + /* Place the stack in the bss section. It's not necessary to define it in the * the linker script. */ .section .bss, "aw", @nobits diff --git a/src/arch/x86/cbmem_top.c b/src/arch/x86/cbmem_top.c new file mode 100644 index 0000000..0c42035 --- /dev/null +++ b/src/arch/x86/cbmem_top.c @@ -0,0 +1,21 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <cbmem.h> + +extern uintptr_t cbmem_top_ptr; + +void *cbmem_top(void) +{ + return (void *)cbmem_top_ptr; +} diff --git a/src/arch/x86/exit_car.S b/src/arch/x86/exit_car.S index 679e335e..0440a0b 100644 --- a/src/arch/x86/exit_car.S +++ b/src/arch/x86/exit_car.S @@ -18,9 +18,18 @@ .section ".module_parameters", "aw", @progbits /* stack_top indicates the stack to pull MTRR information from. */ .global post_car_stack_top +.global cbmem_top_ptr +#if defined(__X86_64__) +post_car_stack_top: +.quad 0 +cbmem_top_ptr: +.quad 0 +#else post_car_stack_top: .long 0 +cbmem_top_ptr: .long 0 +#endif
.text .global _start diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index c6149ab..87e55a5 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -153,15 +153,20 @@ stack_push(pcf, pcf->max_var_mtrrs); }
-static void finalize_load(uintptr_t *stack_top_ptr, uintptr_t stack_top) +struct postcar_param { + uintptr_t stack_top_ptr; + uintptr_t cbmem_top_ptr; +}; + +static void finalize_load(struct postcar_param *param, uintptr_t stack_top) { - *stack_top_ptr = stack_top; + param->stack_top_ptr = stack_top; + param->cbmem_top_ptr = (uintptr_t)cbmem_top(); /* * Signal to rest of system that another update was made to the * postcar program prior to running it. */ - prog_segment_loaded((uintptr_t)stack_top_ptr, sizeof(uintptr_t), - SEG_FINAL); + prog_segment_loaded((uintptr_t)param, sizeof(uintptr_t), SEG_FINAL); }
static void load_postcar_cbfs(struct prog *prog, struct postcar_frame *pcf) diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 2ef6bdf..ccf9f4a 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -99,12 +99,22 @@
static int load_relocatable_ramstage(struct prog *ramstage) { + int ret; struct rmod_stage_load rmod_ram = { .cbmem_id = CBMEM_ID_RAMSTAGE, .prog = ramstage, };
- return rmodule_stage_load(&rmod_ram); + ret = rmodule_stage_load(&rmod_ram); + + if (rmod_ram.params == NULL) + die_with_post_code(POST_INVALID_ROM, + "No parameters found in ramstagege program.\n"); + *(uintptr_t *)rmod_ram.params = (uintptr_t)cbmem_top(); + + prog_segment_loaded((uintptr_t)rmod_ram.params, sizeof(uintptr_t), SEG_FINAL); + + return ret; }
void run_ramstage(void) diff --git a/src/lib/program.ld b/src/lib/program.ld index a9d4e48..317e720 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -97,7 +97,7 @@ * traditional stages in the coreboot infrastructure. Therefore it's easier * to specialize this case. */ -#if ENV_RMODULE || ENV_POSTCAR +#if ENV_RMODULE || ENV_POSTCAR || ENV_RAMSTAGE _rmodule_params = .; KEEP(*(.module_parameters)); _ermodule_params = .; diff --git a/src/northbridge/intel/i945/memmap.c b/src/northbridge/intel/i945/memmap.c index 8207d06..b1326a3 100644 --- a/src/northbridge/intel/i945/memmap.c +++ b/src/northbridge/intel/i945/memmap.c @@ -71,11 +71,13 @@ * 1 MiB alignment. As this may cause very greedy MTRR setup, push * CBMEM top downwards to 4 MiB boundary. */ +#if ENV_ROMSTAGE void *cbmem_top(void) { uintptr_t top_of_ram = ALIGN_DOWN(northbridge_get_tseg_base(), 4*MiB); return (void *) top_of_ram; } +#endif
/** Decodes used Graphics Mode Select (GMS) to kilobytes. */ u32 decode_igd_memory_size(const u32 gms)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36140/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36140/1/src/arch/x86/postcar_loader... PS1, Line 158: uintptr_t cbmem_top_ptr; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/36140/1/src/arch/x86/postcar_loader... PS1, Line 158: uintptr_t cbmem_top_ptr; please, no spaces at the start of a line
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Michael Niewöhner, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36140
to look at the new patch set (#2).
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
[POC,RFC] Pass cbmem_top from romstage to relocatable stages param
Currently all stages that need cbmem need an implementation of a cbmem_top function. On FSP and AGESA platforms this proves to be painful and using the top of lower memory if often passed via lower memory or via a PCI scratchpad register.
The problem with writing to lower memory is that also need to be written on S3 as one cannot assume it to be still there. Writing things on S3 is always a fragile thing to do.
A better and more generic solution is to pass along cbmem_top via relocatable module parameters.
TODO: other platforms, nb/intel/i945 only at the moment.
Tested on Thinkpad X60.
Change-Id: I3195560f110720959fd7389b366d001bfde4da05 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S A src/arch/x86/cbmem_top.c M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/lib/program.ld M src/northbridge/intel/i945/memmap.c 8 files changed, 64 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/36140/2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36140/2/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36140/2/src/lib/prog_loaders.c@112 PS2, Line 112: ramstagege ramstage
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Patch Set 2:
An alternative solution is CB:36144.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Patch Set 2:
Tested successfully on X11SSM after adapting cbmem_top() in memmap. System boots fine without EBDA.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36140/2/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36140/2/src/arch/x86/postcar_loader... PS2, Line 152: stack_push(pcf, pcf->num_var_mtrrs); : stack_push(pcf, pcf->max_var_mtrrs); : } : : struct postcar_param { : uintptr_t stack_top_ptr; : uintptr_t cbmem_top_ptr; : }; : : static void finalize_load(struct postcar_param *param, uintptr_t stack_top) : { : param->stack_top_ptr = stack_top; : param->cbmem_top_ptr = (uintptr_t)cbmem_top(); isn't this mixing up two different approaches?
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36140 )
Change subject: [POC,RFC] Pass cbmem_top from romstage to relocatable stages param ......................................................................
Abandoned
Alternative solution was found.
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Michael Niewöhner (Code Review)