Attention is currently required from: Arthur Heymans. Hello Arthur Heymans,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/59508
to review the following change.
Change subject: arch/x86: Allocate stack on .bss ......................................................................
arch/x86: Allocate stack on .bss
Just like ramstage, the stack can be allocated on .bss instead of requiring special linker script symbols.
Change-Id: I7504f78b1dc214f51caa85d0fbff5daf70251a31 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/arch/x86/include/arch/symbols.h M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/intel/car/romstage.c M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/cpu/x86/entry32.S M src/drivers/amd/agesa/cache_as_ram.S M src/drivers/intel/fsp1_1/cache_as_ram.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S M src/soc/intel/quark/bootblock/esram_init.S M src/soc/intel/quark/romstage/fsp_params.c 16 files changed, 46 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/59508/1
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index 6e73027..889ae09 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -9,11 +9,19 @@ * clear .bss variables that are stage specific. */
-#if CONFIG(RESET_VECTOR_IN_RAM) - #define _STACK_TOP _eearlyram_stack -#else - #define _STACK_TOP _ecar_stack -#endif + +/* Place the stack in the bss section. It's not necessary to define it in the + * the linker script. */ + .section .bss, "aw", @nobits +.global _stack +.global _estack +.global _stack_size + +.align 8 +_stack: +.space CONFIG_DCACHE_BSP_STACK_SIZE +_estack: +.set _stack_size, _estack - _stack
#if ENV_X86_64 .code64 @@ -33,7 +41,7 @@ #endif
/* reset stack pointer to CAR/EARLYRAM stack */ - mov $_STACK_TOP, %esp + mov $_estack, %esp
/* clear .bss section as it is not shared */ cld diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 5a46b8b..09e171f 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -23,10 +23,6 @@ * Needs to be transferred until CBMEM is available */ TPM_TCPA_LOG(., 2K) #endif - /* Stack for CAR stages. Since it persists across all stages that - * use CAR it can be reused. The chipset/SoC is expected to provide - * the stack size. */ - REGION(car_stack, ., CONFIG_DCACHE_BSP_STACK_SIZE, 4) /* The pre-ram cbmem console as well as the timestamp region are fixed * in size. Therefore place them above the car global section so that * multiple stages (romstage and verstage) have a consistent diff --git a/src/arch/x86/include/arch/symbols.h b/src/arch/x86/include/arch/symbols.h index caafdfa..df2536e 100644 --- a/src/arch/x86/include/arch/symbols.h +++ b/src/arch/x86/include/arch/symbols.h @@ -12,14 +12,6 @@ extern char _car_region_end[]; #define _car_region_size (_car_region_end - _car_region_start)
-/* - * This is the stack area used for all stages that execute when cache-as-ram - * is up. Area is not cleared in between stages. - */ -extern char _car_stack[]; -extern char _ecar_stack[]; -#define _car_stack_size (_ecar_stack - _car_stack) - extern char _car_unallocated_start[];
extern char _car_ehci_dbg_info[]; diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index f47ba5f..29b617b 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -152,7 +152,7 @@ movl %eax, %cr0
/* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/non-evict/cache_as_ram.S b/src/cpu/intel/car/non-evict/cache_as_ram.S index 0451bb4..e006f04 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -204,7 +204,7 @@ movl %eax, %cr0
/* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 887bb22..2becd1e 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -139,7 +139,7 @@ movl %eax, %cr0
/* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index 9ac9e22..7187bc9 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -352,7 +352,7 @@ rep stosl
/* Setup the stack. */ - mov $_ecar_stack, %esp + mov $_estack, %esp
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 63ddd74..6b9a50f 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -15,6 +15,9 @@ #define DCACHE_RAM_ROMSTAGE_STACK_SIZE 0x2000
static struct postcar_frame early_mtrrs; +extern uint8_t *_stack; +extern uint8_t *_estack; +extern size_t _stack_size;
static void romstage_main(void) { @@ -27,14 +30,14 @@ DCACHE_RAM_ROMSTAGE_STACK_SIZE);
/* Size of unallocated CAR. */ - size = ALIGN_DOWN(_car_stack_size, 16); + size = ALIGN_DOWN(_stack_size, 16);
size = MIN(size, stack_size); if (size < stack_size) printk(BIOS_DEBUG, "Romstage stack size limited to 0x%x!\n", size);
- stack_base = (u32 *) (_ecar_stack - size); + stack_base = (u32 *) (_stack - size);
for (i = 0; i < num_guards; i++) stack_base[i] = stack_guard; diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index f828d6f..38bea60 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -74,7 +74,7 @@ pages_done: #endif
- movl $_ecar_stack, %esp + movl $_estack, %esp
/* Align the stack and keep aligned for call to bootblock_c_entry() */ and $0xfffffff0, %esp diff --git a/src/cpu/x86/entry32.S b/src/cpu/x86/entry32.S index 215d601..78552db 100644 --- a/src/cpu/x86/entry32.S +++ b/src/cpu/x86/entry32.S @@ -15,6 +15,19 @@ #include <cpu/x86/cr.h> #include <cpu/x86/post_code.h>
+/* Place the stack in the bss section. It's not necessary to define it in the + * the linker script. */ + .section .bss, "aw", @nobits +.global _stack +.global _estack +.global _stack_size + +.align 8 +_stack: +.space CONFIG_DCACHE_BSP_STACK_SIZE +_estack: +.set _stack_size, _estack - _stack + .section .init, "ax", @progbits
.code32 diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 0d678d1..0a66012 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -38,14 +38,14 @@ * Set up bootblock stack on BSP. * AMD_ENABLE_STACK macro sets up a stack for BSP at BSP_STACK_BASE_ADDR * which is 0x30000 (_car_region_end), but for C bootblock the stack - * begins at _ecar_stack (see arch/x86/car.ld) + * begins at _estack (in .bss) */ mov $LAPIC_BASE_MSR, %ecx rdmsr test $LAPIC_BASE_MSR_BOOTSTRAP_PROCESSOR, %eax jz ap_entry
- mov $_ecar_stack, %esp + mov $_estack, %esp
/* Align the stack and keep aligned for call to bootblock_c_entry() */ and $0xfffffff0, %esp diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index e20d527..b2bb62d 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -135,7 +135,7 @@ jne halt2
/* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp
/* * ebp: FSP_INFO_HEADER address diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index cac7854..2443dc6 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -271,7 +271,7 @@ post_code(0x29)
/* Setup bootblock stack */ - mov $_ecar_stack, %esp + mov $_estack, %esp
/* Need to align stack to 16 bytes at call instruction. Account for the two pushes below. */ diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S index 173ebf7..51ba0cb 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram_fsp.S @@ -64,7 +64,7 @@ jnz .halt_forever
/* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp /* * temp_memory_start/end reside in the .bss section, which gets cleared * below. Save the FSP return value to the stack before writing those diff --git a/src/soc/intel/quark/bootblock/esram_init.S b/src/soc/intel/quark/bootblock/esram_init.S index 39ce7bd..b146f26 100644 --- a/src/soc/intel/quark/bootblock/esram_init.S +++ b/src/soc/intel/quark/bootblock/esram_init.S @@ -476,7 +476,7 @@ */
/* Setup bootblock stack */ - movl $_ecar_stack, %esp + movl $_estack, %esp
before_carstage: post_code(0x2b) diff --git a/src/soc/intel/quark/romstage/fsp_params.c b/src/soc/intel/quark/romstage/fsp_params.c index 11f7059..2ffe92f 100644 --- a/src/soc/intel/quark/romstage/fsp_params.c +++ b/src/soc/intel/quark/romstage/fsp_params.c @@ -12,6 +12,7 @@ #include <soc/romstage.h> #include <soc/reg_access.h> #include <soc/storage_test.h> +#include <stdint.h>
void mainboard_romstage_entry(void) { @@ -103,8 +104,9 @@ printk(BIOS_SPEW, "+-------------------+ %p\n", _car_unallocated_start); printk(BIOS_SPEW, "| coreboot data |\n"); + extern uint8_t *_estack; printk(BIOS_SPEW, "+-------------------+ %p\n", - _ecar_stack); + _estack); printk(BIOS_SPEW, "| coreboot stack |\n"); printk(BIOS_SPEW, "+-------------------+ 0x80000000 - ESRAM start\n\n");