Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Top align bootblock program ......................................................................
arch/x86: Top align bootblock program
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/memlayout.ld M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/cpu/x86/16bit/entry16.inc M src/cpu/x86/16bit/reset16.ld M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/cache_as_ram.S 12 files changed, 62 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/1
diff --git a/src/arch/x86/bootblock_crt0.S b/src/arch/x86/bootblock_crt0.S index 3f41464..20262eb 100644 --- a/src/arch/x86/bootblock_crt0.S +++ b/src/arch/x86/bootblock_crt0.S @@ -10,7 +10,7 @@
#include <cpu/x86/cr.h>
-.section .text +.section ".init", "ax", @progbits
/* * Include the old code for reset vector and protected mode entry. That code has @@ -53,3 +53,5 @@
/* We're done. Now it's up to platform-specific code */ jmp bootblock_pre_c_entry + +.previous diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index b487e48..a68dac5 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -28,8 +28,6 @@
#include "car.ld" #elif ENV_BOOTBLOCK - BOOTBLOCK(0xffffffff - CONFIG_C_ENV_BOOTBLOCK_SIZE + 1, - CONFIG_C_ENV_BOOTBLOCK_SIZE)
#include "car.ld"
diff --git a/src/arch/x86/walkcbfs.S b/src/arch/x86/walkcbfs.S index b8d4fb9..66b97e3b 100644 --- a/src/arch/x86/walkcbfs.S +++ b/src/arch/x86/walkcbfs.S @@ -21,7 +21,7 @@ #define CBFS_FILE_STRUCTSIZE (CBFS_FILE_OFFSET + 4)
.code32 -.section .text +.section ".init" .global walkcbfs_asm
/* @@ -121,3 +121,5 @@
filemagic: .ascii "LARCHIVE" + +.previous diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index 837394c..40cd60d 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -4,6 +4,7 @@ #include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
+.section ".init" .global bootblock_pre_c_entry
.code32 @@ -190,3 +191,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous 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 cde1ca3..1846408 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -7,6 +7,7 @@ #define NoEvictMod_MSR 0x2e0 #define BBL_CR_CTL3_MSR 0x11e
+.section ".init" .global bootblock_pre_c_entry
#include <cpu/intel/car/cache_as_ram_symbols.inc> @@ -257,3 +258,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 71e3447..b48afac 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -4,6 +4,7 @@ #include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
+.section ".init" .global bootblock_pre_c_entry
.code32 @@ -179,3 +180,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous 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 4e36538..b02fced 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -8,6 +8,7 @@ /* Macro to access Local APIC registers at default base. */ #define LAPIC(x) $(LAPIC_DEFAULT_BASE | LAPIC_ ## x)
+.section ".init" .global bootblock_pre_c_entry
.code32 @@ -390,3 +391,4 @@ fixed_mtrr_list_size = . - fixed_mtrr_list
_cache_as_ram_setup_end: +.previous diff --git a/src/cpu/intel/microcode/microcode_asm.S b/src/cpu/intel/microcode/microcode_asm.S index f02351a..8bef63d 100644 --- a/src/cpu/intel/microcode/microcode_asm.S +++ b/src/cpu/intel/microcode/microcode_asm.S @@ -43,7 +43,7 @@ * if the revision of the update is newer than what is installed */
-.section .text +.section ".init" .global update_bsp_microcode
update_bsp_microcode: @@ -151,3 +151,5 @@ .string "cpu_microcode_blob.bin"
_update_bsp_microcode_end: + +.previous diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 13d12be..e9ebac6 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -28,10 +28,9 @@ #include <arch/rom_segs.h> #include <cpu/x86/post_code.h>
-/* Symbol _start16bit must be aligned to 4kB to start AP CPUs with - * Startup IPI message without RAM. +/* Symbol _start16bit must reachable from the reset vector, and be aligned to + * 4kB to start AP CPUs with Startup IPI message without RAM. */ -.align 4096 .code16 .globl _start16bit .type _start16bit, @function diff --git a/src/cpu/x86/16bit/reset16.ld b/src/cpu/x86/16bit/reset16.ld index d356034..be1209a 100644 --- a/src/cpu/x86/16bit/reset16.ld +++ b/src/cpu/x86/16bit/reset16.ld @@ -11,14 +11,28 @@ ap_sipi_vector_in_rom = (_start16bit >> 12) & 0xff; #endif
-/* - * _ROMTOP : The top of the ROM used where we - * need to put the reset vector. - */ - SECTIONS { - /* Trigger an error if I have an unuseable start address */ - _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); + . = BOOTBLOCK_TOP - PROGRAM_SZ; + . = ALIGN(16); + + _bootblock = .; + + INCLUDE "bootblock/lib/program.ld" + + . = ID_SECTION - EARLYASM_SZ; + . = CONFIG(SIPI_VECTOR_IN_ROM) ? ALIGN(4096) : ALIGN(16); + BOOTBLOCK_TOP = .; + .init . : { + *(.init); + } + + /* + * Allocation reserves extra space here. Alignment requirements + * may cause the total size of a section to change when the start + * address gets applied. + */ + PROGRAM_SZ = SIZEOF(.text) + 32; + EARLYASM_SZ = SIZEOF(.init) + (CONFIG(SIPI_VECTOR_IN_ROM) ? 4096 : 16);
. = ID_SECTION; .id (.): { @@ -40,4 +54,9 @@ . = 15; BYTE(0x00); } + _ebootblock = .; } + +_bogus1 = ASSERT(_bootblock & 0x80000000, "Invalid linker script"); +_bogus2 = ASSERT(_start16bit & 0x80000000, "Invalid linker script"); +_bogus3 = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report."); diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 8e7ea29..9235008 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -10,16 +10,19 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/lapic_def.h> #include <cpu/x86/post_code.h>
+.section ".init" + .code32 .globl _cache_as_ram_setup, _cache_as_ram_setup_end .global bootblock_pre_c_entry
_cache_as_ram_setup:
+#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) @@ -73,3 +76,4 @@ jmp stop
_cache_as_ram_setup_end: +.previous diff --git a/src/soc/amd/common/block/cpu/car/cache_as_ram.S b/src/soc/amd/common/block/cpu/car/cache_as_ram.S index 3eb7670..9d35d5c 100644 --- a/src/soc/amd/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/amd/common/block/cpu/car/cache_as_ram.S @@ -8,9 +8,17 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/post_code.h>
+.section ".init" + +.code32 +.globl _cache_as_ram_setup, _cache_as_ram_setup_end + +_cache_as_ram_setup: + +#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) @@ -43,3 +51,6 @@ post_code(POST_DEAD_CODE) hlt jmp .halt_forever + +_cache_as_ram_setup_end: +.previous
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Looks good.
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... File src/arch/x86/bootblock_crt0.S:
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 19: #include <cpu/x86/16bit/entry16.inc> : #include <cpu/x86/16bit/reset16.inc> : #include <cpu/x86/32bit/entry32.inc> So this is the only thing that really needs to be in the init section. All the rest is just to be more efficient if SIPI is in ROM (to fill up the top 4K)?
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 57: .previous So directive swaps this to the previous section? What does this mean here?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/47599/2/src/cpu/x86/16bit/reset16.l... PS2, Line 61: _bogus2 = ASSERT(_start16bit & 0x80000000, "Invalid linker script"); Maybe use different assertion error messages to avoid confusion?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Top align bootblock program ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... File src/arch/x86/bootblock_crt0.S:
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 19: #include <cpu/x86/16bit/entry16.inc> : #include <cpu/x86/16bit/reset16.inc> : #include <cpu/x86/32bit/entry32.inc>
So this is the only thing that really needs to be in the init section. […]
Section .init description for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs)
I think CAR init and clearing stack/bss fits the description precisely. Rest of the .S files changed here fit the "called before main program entry" criteria.
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 57: .previous
So directive swaps this to the previous section? What does this mean here?
I might have replicated this from id.S or fit.S, but at the end of the file it indeed makes little sense now. Mentioned .S files were previously included from booblock0.S CB:11792, prior to that use of the directive probably made a difference.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#3).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/drivers/amd/agesa/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S 11 files changed, 28 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 3: Code-Review+1
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons, Julius Werner, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#4).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/drivers/amd/agesa/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S 11 files changed, 28 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs). I haven't looked into it but how does this work in general for ELF entry point? So the current use case is X86 bootblock/reset vector, but if on a different stage/arch something is to be put into the .init section, one would expect this to run before .text._start ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
I haven't looked into it but how does this work in general for ELF entry point? So the current use c […]
ELF entry point is declared separately from sections. Should one put something in .init one would have to place the entry point symbol (_start or stage_entry) there too.
I wonder if we could first rename existing .text._start and .text.stage_entry both to .init._start.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
I wonder if we could first rename existing .text._start and .text.stage_entry both to .init._start.
I don't really understand the reason for not calling this .text.init anyway? We've always relied on those things being matched before .text.*, and it has always worked (I don't remember the exact rules, I think it might just be order of appearance in the linker script).
I mean, really, I think the .text._start and .text.stage_entry we have are already exactly for what you're trying to do here anyway, why not just use those rather than making another one?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
I wonder if we could first rename existing .text._start and .text.stage_entry both to .init. […]
For x86 bootblocks .text._start must be at most 64 KiB from .reset. Or in other words, C_ENV_BOOTBLOCK_SIZE must not grow over 64 KiB while latest soc/intel have reached 48 KiB already.
There is followup CB:47970 that locates this newly introduced .init after .text.* rules.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
For x86 bootblocks .text._start must be at most 64 KiB from .reset. […]
But can't you just put the BOOTBLOCK_TOP parts before the INCLUDE "...program.ld" in that file? I think it's order of occurrence in the script file, not in linear address space. (I mean, I'm also fine with adding a new .init section instead if you consider that too hacky. But I'd like to keep .text._start and .text.stage_entry the way they are because that way they match the automatic -ffunction-section names for those functions.)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
But can't you just put the BOOTBLOCK_TOP parts before the INCLUDE "...program. […]
I would rather not rely on order of occurence, specially when .text._start would appear in two different files then.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Julius Werner, Angel Pons, Arthur Heymans, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#5).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/drivers/amd/agesa/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S 11 files changed, 28 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/5
Hello build bot (Jenkins), Jason Glenesk, Furquan Shaikh, Patrick Georgi, Martin Roth, Marshall Dawson, Julius Werner, Angel Pons, Arthur Heymans, Aaron Durbin, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#6).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/drivers/amd/agesa/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S M src/soc/amd/common/block/cpu/noncar/pre_c.S 12 files changed, 32 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/6
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... File src/arch/x86/bootblock_crt0.S:
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 19: #include <cpu/x86/16bit/entry16.inc> : #include <cpu/x86/16bit/reset16.inc> : #include <cpu/x86/32bit/entry32.inc>
Section .init description for ELF: […]
Ack
https://review.coreboot.org/c/coreboot/+/47599/1/src/arch/x86/bootblock_crt0... PS1, Line 57: .previous
I might have replicated this from id.S or fit. […]
Done
https://review.coreboot.org/c/coreboot/+/47599/2/src/cpu/x86/16bit/reset16.l... File src/cpu/x86/16bit/reset16.ld:
https://review.coreboot.org/c/coreboot/+/47599/2/src/cpu/x86/16bit/reset16.l... PS2, Line 61: _bogus2 = ASSERT(_start16bit & 0x80000000, "Invalid linker script");
Maybe use different assertion error messages to avoid confusion?
Done
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 7:
(1 comment)
Looks OK to me.
https://review.coreboot.org/c/coreboot/+/47599/7/src/soc/amd/common/block/cp... File src/soc/amd/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47599/7/src/soc/amd/common/block/cp... PS7, Line 16: _cache_as_ram_setup_end I assume this is .globl only to match drivers/amd/agesa/cache_as_ram.S? It looks like the intel/car implementations aren't global.
Hello build bot (Jenkins), Jason Glenesk, Furquan Shaikh, Patrick Georgi, Martin Roth, Marshall Dawson, Julius Werner, Angel Pons, Arthur Heymans, Aaron Durbin, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#8).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/drivers/amd/agesa/cache_as_ram.S M src/drivers/intel/fsp1_1/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S M src/soc/amd/common/block/cpu/noncar/pre_c.S M src/soc/example/min86/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 18 files changed, 44 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/8
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Julius Werner, Angel Pons, Arthur Heymans, Aaron Durbin, Patrick Rudolph, Jason Glenesk, Martin Roth, Lee Leahy, Marshall Dawson, Huang Jin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47599
to look at the new patch set (#9).
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/drivers/amd/agesa/cache_as_ram.S M src/drivers/intel/fsp1_1/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S M src/soc/amd/common/block/cpu/noncar/pre_c.S M src/soc/example/min86/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 18 files changed, 44 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47599/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/7/src/soc/amd/common/block/cp... File src/soc/amd/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/47599/7/src/soc/amd/common/block/cp... PS7, Line 16: _cache_as_ram_setup_end
I assume this is .globl only to match drivers/amd/agesa/cache_as_ram. […]
Should be no need for .global here either.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 10: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47599/4//COMMIT_MSG@17 PS4, Line 17: Description of .init section for ELF: : : This section holds executable instructions that contribute to the : process initialization code. When a program starts to run, the : system arranges to execute the code in this section before calling the : main program entry point (called main for C programs).
I would rather not rely on order of occurence, specially when .text. […]
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 11: Code-Review+1
I would like to test this, but I don't have any hardware at hand right now.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 11: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
Patch Set 11:
Patch Set 11: Code-Review+1
I would like to test this, but I don't have any hardware at hand right now.
I only tried with combination of FIXED_BOOTBLOCK_SIZE=y and C_ENV_BOOTBLOCK_SIZE=0x20000 with qemu-q35 with the rest of topic:x86-bootblock applied.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47599 )
Change subject: arch/x86: Move prologue to .init section ......................................................................
arch/x86: Move prologue to .init section
For arch/x86 the realmode part has to be located within the same 64 KiB as the reset vector. Some older intel platforms also require 4 KiB alignment for _start16bit.
To enforce the above, and to separate required parts of .text without matching *(.text.*) rules in linker scripts, tag the pre-C environment assembly code with section .init directive.
Description of .init section for ELF:
This section holds executable instructions that contribute to the process initialization code. When a program starts to run, the system arranges to execute the code in this section before calling the main program entry point (called main for C programs).
Change-Id: If32518b1c19d08935727330314904b52a246af3c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47599 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/bootblock_crt0.S M src/arch/x86/gdt_init.S M src/arch/x86/walkcbfs.S 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/microcode/microcode_asm.S M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/drivers/amd/agesa/cache_as_ram.S M src/drivers/intel/fsp1_1/cache_as_ram.S M src/lib/program.ld M src/soc/amd/common/block/cpu/car/cache_as_ram.S M src/soc/amd/common/block/cpu/noncar/pre_c.S M src/soc/example/min86/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 18 files changed, 44 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/bootblock_crt0.S b/src/arch/x86/bootblock_crt0.S index 82ae97f..387920e 100644 --- a/src/arch/x86/bootblock_crt0.S +++ b/src/arch/x86/bootblock_crt0.S @@ -10,7 +10,7 @@
#include <cpu/x86/cr.h>
-.section .text._start +.section .init._start, "ax", @progbits
/* * Include the old code for reset vector and protected mode entry. That code has diff --git a/src/arch/x86/gdt_init.S b/src/arch/x86/gdt_init.S index 1558ac6..f33a151 100644 --- a/src/arch/x86/gdt_init.S +++ b/src/arch/x86/gdt_init.S @@ -1,7 +1,10 @@ /* SPDX-License-Identifier: GPL-2.0-only */
.code32 -.section ".text._gdt_", "ax", @progbits + +.section .init, "ax", @progbits + +.section .init._gdt_, "ax", @progbits
.globl gdt_init gdt_init: @@ -17,7 +20,7 @@
#ifdef __x86_64__ .code64 -.section ".text._gdt64_", "ax", @progbits +.section .init._gdt64_, "ax", @progbits .globl gdt_init64 gdt_init64: /* Workaround a bug in the assembler. diff --git a/src/arch/x86/walkcbfs.S b/src/arch/x86/walkcbfs.S index b8d4fb9..393bcf5 100644 --- a/src/arch/x86/walkcbfs.S +++ b/src/arch/x86/walkcbfs.S @@ -21,7 +21,7 @@ #define CBFS_FILE_STRUCTSIZE (CBFS_FILE_OFFSET + 4)
.code32 -.section .text +.section .init .global walkcbfs_asm
/* diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index 837394c..2c67207 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -4,6 +4,7 @@ #include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
+.section .init .global bootblock_pre_c_entry
.code32 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 cde1ca3..0451bb4 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -7,6 +7,7 @@ #define NoEvictMod_MSR 0x2e0 #define BBL_CR_CTL3_MSR 0x11e
+.section .init .global bootblock_pre_c_entry
#include <cpu/intel/car/cache_as_ram_symbols.inc> diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 71e3447..887bb22 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -4,6 +4,7 @@ #include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
+.section .init .global bootblock_pre_c_entry
.code32 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 4e365384..103d9e9 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -8,6 +8,7 @@ /* Macro to access Local APIC registers at default base. */ #define LAPIC(x) $(LAPIC_DEFAULT_BASE | LAPIC_ ## x)
+.section .init .global bootblock_pre_c_entry
.code32 diff --git a/src/cpu/intel/microcode/microcode_asm.S b/src/cpu/intel/microcode/microcode_asm.S index 5173ae5..2870523 100644 --- a/src/cpu/intel/microcode/microcode_asm.S +++ b/src/cpu/intel/microcode/microcode_asm.S @@ -44,7 +44,7 @@ */
.code32 -.section .text +.section .init .global update_bsp_microcode
update_bsp_microcode: diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index 197e0fd..e3a26b0 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -2,6 +2,8 @@
#include <cpu/x86/post_code.h>
+.section .init, "ax", @progbits + .global bootblock_pre_c_entry bootblock_pre_c_entry:
diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 8e7ea29..33940cb 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -10,16 +10,19 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/lapic_def.h> #include <cpu/x86/post_code.h>
+.section .init + .code32 -.globl _cache_as_ram_setup, _cache_as_ram_setup_end + .global bootblock_pre_c_entry
_cache_as_ram_setup:
+#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index f1cfff7..e20d527 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -14,6 +14,8 @@
#define LHLT_DELAY 0x50000 /* I/O delay between post codes on failure */
+.section .init, "ax", @progbits + .global bootblock_pre_c_entry bootblock_pre_c_entry: /* diff --git a/src/lib/program.ld b/src/lib/program.ld index d419ab6..3eebd6c 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -13,6 +13,9 @@ .text . : { _program = .; _text = .; + *(.init._start); + *(.init); + *(.init.*); *(.text._start); *(.text.stage_entry); KEEP(*(.metadata_hash_anchor)); diff --git a/src/soc/amd/common/block/cpu/car/cache_as_ram.S b/src/soc/amd/common/block/cpu/car/cache_as_ram.S index 3eb7670..6282d7e 100644 --- a/src/soc/amd/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/amd/common/block/cpu/car/cache_as_ram.S @@ -8,9 +8,16 @@ ****************************************************************************** */
-#include "gcccar.inc" #include <cpu/x86/post_code.h>
+.section .init + +.code32 + +_cache_as_ram_setup: + +#include "gcccar.inc" + /* * on entry: * mm0: BIST (ignored) @@ -43,3 +50,5 @@ post_code(POST_DEAD_CODE) hlt jmp .halt_forever + +_cache_as_ram_setup_end: diff --git a/src/soc/amd/common/block/cpu/noncar/pre_c.S b/src/soc/amd/common/block/cpu/noncar/pre_c.S index 6fae1ed..520e3c0 100644 --- a/src/soc/amd/common/block/cpu/noncar/pre_c.S +++ b/src/soc/amd/common/block/cpu/noncar/pre_c.S @@ -2,6 +2,8 @@
#include <cpu/x86/post_code.h>
+.section .init, "ax", @progbits + .global bootblock_resume_entry bootblock_resume_entry: post_code(0xb0) diff --git a/src/soc/example/min86/cache_as_ram.S b/src/soc/example/min86/cache_as_ram.S index a350143..5c5066d 100644 --- a/src/soc/example/min86/cache_as_ram.S +++ b/src/soc/example/min86/cache_as_ram.S @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+.section .init, "ax", @progbits + .global bootblock_pre_c_entry
.code32 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 aaf6af7..49b40a8 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 @@ -9,6 +9,8 @@ #include <rules.h> #include <intelblocks/msr.h>
+.section .init, "ax", @progbits + .code32 .global bootblock_pre_c_entry bootblock_pre_c_entry: 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 04dc533..a2e85b9 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 @@ -10,6 +10,8 @@ #define CBFS_FILE_CHECKSUM (CBFS_FILE_TYPE + 4) #define CBFS_FILE_OFFSET (CBFS_FILE_CHECKSUM + 4)
+.section .init, "ax", @progbits + .extern temp_ram_init_params
.global bootblock_pre_c_entry diff --git a/src/soc/intel/quark/bootblock/esram_init.S b/src/soc/intel/quark/bootblock/esram_init.S index fc1c7c9..39ce7bd 100644 --- a/src/soc/intel/quark/bootblock/esram_init.S +++ b/src/soc/intel/quark/bootblock/esram_init.S @@ -71,6 +71,8 @@ .equ CFGNONSTICKY_W1_OFFSET, (0x52) .equ FORCE_WARM_RESET, (0x00000001)
+.section .init, "ax", @progbits + .global bootblock_pre_c_entry
bootblock_pre_c_entry:
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)