Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link NO_XIP_EARLY_STAGES like other stages ......................................................................
arch/x86: Link NO_XIP_EARLY_STAGES like other stages
If stages are not run XIP they can be linked like other stages, which includes a (*.data) section.
Change-Id: Ib165058abfb07e385461cdcca4aef31928ec7572 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/include/rules.h M src/lib/program.ld 4 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36612/1
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index c36dc1c..a838b6d 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -32,7 +32,8 @@ /* reset stack pointer to CAR stack */ mov $_car_stack_end, %esp
- /* clear .bss section as it is not shared */ + /* clear .bss section as it is not shared if stages are run XIP*/ +#if !ENV_STAGE_HAS_DATA_SECTION cld xor %eax, %eax movl $(_ebss), %ecx @@ -40,6 +41,7 @@ sub %edi, %ecx shrl $2, %ecx rep stosl +#endif
#if ((ENV_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ || (ENV_ROMSTAGE && CONFIG(ROMSTAGE_DEBUG_SPINLOOP))) diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 6ccbd8c..a68b635 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -69,6 +69,7 @@ * cbmem console. This is useful for clearing this area on a per-stage * basis when more than one stage uses cache-as-ram for CAR_GLOBALs. */
+#if !ENV_STAGE_HAS_DATA_SECTION . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .; #if ENV_STAGE_HAS_BSS_SECTION @@ -84,6 +85,7 @@ #endif . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _ebss = .; +#endif _car_unallocated_start = .;
#if !CONFIG(C_ENVIRONMENT_BOOTBLOCK) diff --git a/src/include/rules.h b/src/include/rules.h index 0436198..fe162f8 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -274,7 +274,7 @@ /* Indicates memory layout is determined with arch/x86/car.ld. */ #define ENV_CACHE_AS_RAM (ENV_ROMSTAGE_OR_BEFORE && !CONFIG(RESET_VECTOR_IN_RAM)) /* No .data sections with execute-in-place from ROM. */ -#define ENV_STAGE_HAS_DATA_SECTION !ENV_CACHE_AS_RAM +#define ENV_STAGE_HAS_DATA_SECTION (!ENV_CACHE_AS_RAM || CONFIG(NO_XIP_EARLY_STAGES)) /* No .bss sections for stage with CAR teardown. */ #define ENV_STAGE_HAS_BSS_SECTION !(ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION)) #else diff --git a/src/lib/program.ld b/src/lib/program.ld index a9d4e48..d323ac1 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -125,7 +125,7 @@ } #endif
-#if ENV_STAGE_HAS_BSS_SECTION && !ENV_CACHE_AS_RAM +#if ENV_STAGE_HAS_BSS_SECTION && ENV_STAGE_HAS_DATA_SECTION .bss . : { . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .;
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link NO_XIP_EARLY_STAGES like other stages ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION Why does no data section imply a bss section?
.bss section is not shared across stages no matter what. There isn't a conditional applied.
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h@277 PS1, Line 277: CONFIG(NO_XIP_EARLY_STAGES)) This doesn't make sense to me.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link NO_XIP_EARLY_STAGES like other stages ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
Why does no data section imply a bss section?
.bss section is not shared across stages no matter what. There isn't a conditional applied.
Right. It should state that .bss only needs clearing if the stage is run XIP.
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h@277 PS1, Line 277: CONFIG(NO_XIP_EARLY_STAGES))
This doesn't make sense to me.
Should it be guarded differently with something like a !ENV_XIP parameter?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link NO_XIP_EARLY_STAGES like other stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
Why does no data section imply a bss section? […]
That's assuming we have memlen set correctly in the case you are describing. I'm pretty sure we don't, but there are some funny combinations to deal with. What have you done to ensure the assertions are held you are implying here?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link NO_XIP_EARLY_STAGES like other stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
That's assuming we have memlen set correctly in the case you are describing. I'm pretty sure we don't, but there are some funny combinations to deal with. What have you done to ensure the assertions are held you are implying here?
You mean that the memlen that is currently hardcoded to 1M and should be checked if it fits in CAR? That should be done regardless of this patch?
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36612
to look at the new patch set (#2).
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
arch/x86: Link .bss and .data in in-CAR stages
If stages are not run XIP, they can be linked like other stages, which includes a (*.data) section.
Change-Id: Ib165058abfb07e385461cdcca4aef31928ec7572 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/assembly_entry.S M src/arch/x86/car.ld M src/include/rules.h M src/lib/program.ld 4 files changed, 10 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/36612/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36612/2/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/36612/2/src/lib/program.ld@128 PS2, Line 128: ENV_STAGE_IN_CAR Not defined on non-x86. Probably best to inverse this logic in arch/x86/car.ld
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
That's assuming we have memlen set correctly in the case you are describing. […]
I mean in the stage struct in cbfs. We're currently ignoring the .car.data section in romstage and verstage (should for bootblock, but that's a weird special case) utilizing '-S ".car.data"' option to cbfstool. Currently .bss, stack, and 'persistent' car globals across stages are in there.
Here you're trying to remove where all thosee regions are being linked in and from what linker script (currently utilizing all car.ld) and changing the semantics.
1. We should ensure what the linked program looks like. readelf -e is helpful. And that things are added to cbfs correctly. (cbfstool print). 2. We should provide an indicator that provides the semantics you are desiring so it's clear in the code.
The reason I say the above is that it is possible to get data sections in XIP code but we'd need an entry in this path to copy from spi into CAR where the data section resides. In this patch you just want to enable the data section outright since data (and potentially bss) can just move entirely with the program itself during relocation. I think it's helpful to call out those scenarios explicitly with a macro that makes sense while reading the code.
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/36612/1/src/include/rules.h@277 PS1, Line 277: CONFIG(NO_XIP_EARLY_STAGES))
This doesn't make sense to me. […]
Please see my other comment about not using a proxy for the semantics we're looking for. We probably want something like.
#if CONFIG(NO_XIP_EARLY_STAGES) set some very explicit macro names indicating semantics #else same as above but for XIP stages #endif
The above get a little more difficult because once you link .data and .bss you need corresponding changes in assembly_entry.S (as you have) to handle the runtime assumptions of the stage loader clearing bss, but as I noted in the other comment we can get data section w/ XIP so we should be explicit about the scenarios.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
I mean in the stage struct in cbfs. We're currently ignoring the .car.data section in romstage and verstage (should for bootblock, but that's a weird special case) utilizing '-S ".car.data"' option to cbfstool. Currently .bss, stack, and 'persistent' car globals across stages are in there.
Here you're trying to remove where all thosee regions are being linked in and from what linker script (currently utilizing all car.ld) and changing the semantics.
- We should ensure what the linked program looks like. readelf -e is helpful. And that things are added to cbfs correctly. (cbfstool print).
It looks fine,
- We should provide an indicator that provides the semantics you are desiring so it's clear in the code.
The reason I say the above is that it is possible to get data sections in XIP code but we'd need an entry in this path to copy from spi into CAR where the data section resides. In this patch you just want to enable the data section outright since data (and potentially bss) can just move entirely with the program itself during relocation. I think it's helpful to call out those scenarios explicitly with a macro that makes sense while reading the code.
So if I understand it correctly it should deal with 1) moving sections like .bss from car.data in the cbfs stage when not running XIP 2) adding a .data section to early X86 stages (except bootblock) 3) improve some semantics in rules.h in different CL?
readelf -e for the apollolake romstage.elf
ELF Header: Magic: 7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00 Class: ELF32 Data: 2's complement, little endian Version: 1 (current) OS/ABI: UNIX - System V ABI Version: 0 Type: EXEC (Executable file) Machine: Intel 80386 Version: 0x1 Entry point address: 0xfef20000 Start of program headers: 52 (bytes into file) Start of section headers: 49048 (bytes into file) Flags: 0x0 Size of this header: 52 (bytes) Size of program headers: 32 (bytes) Number of program headers: 1 Size of section headers: 40 (bytes) Number of section headers: 10 Section header string table index: 9
Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS fef20000 000060 007690 00 AX 0 0 32 [ 2] .rel.text REL 00000000 00a10c 001e48 08 I 7 1 4 [ 3] .data PROGBITS fef27690 0076f0 000038 00 WA 0 0 4 [ 4] .bss NOBITS fef276c8 007728 0000f8 00 WA 0 0 32 [ 5] .car.data NOBITS fef00000 000000 004e50 00 WA 0 0 1 [ 6] .gnu_debuglink PROGBITS 00000000 007728 000014 00 0 0 4 [ 7] .symtab SYMTAB 00000000 00773c 0015d0 10 8 125 4 [ 8] .strtab STRTAB 00000000 008d0c 001400 00 0 0 1 [ 9] .shstrtab STRTAB 00000000 00bf54 000043 00 0 0 1 Key to Flags: W (write), A (alloc), X (execute), M (merge), S (strings), I (info), L (link order), O (extra OS processing required), G (group), T (TLS), C (compressed), x (unknown), o (OS specific), E (exclude), p (processor specific)
Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x000060 0xfef20000 0xfef20000 0x076c8 0x077c0 RWE 0x20
Section to Segment mapping: Segment Sections... 00 .text .data .bss
with this code
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/36612/1/src/arch/x86/assembly_entry... PS1, Line 36: #if !ENV_STAGE_HAS_DATA_SECTION
- We should provide an indicator that provides the semantics you are desiring so it's clear in the code.
The reason I say the above is that it is possible to get data sections in XIP code but we'd need an entry in this path to copy from spi into CAR where the data section resides. In this patch you just want to enable the data section outright since data (and potentially bss) can just move entirely with the program itself during relocation. I think it's helpful to call out those scenarios explicitly with a macro that makes sense while reading the code.
So if I understand it correctly it should deal with
- moving sections like .bss from car.data in the cbfs stage when not running XIP
- adding a .data section to early X86 stages (except bootblock)
- improve some semantics in rules.h
in different CL?
I think we need clear semantics from the get-go:
1. Does this stage have .bss section? 1a. Did this stage have its .bss cleared prior to running? 2. Does this stage have .data section?
Those need to be answered in both linking as well as runtime. With those richer sets of semantics we can make things easier to implement and follow. And, of course, different behaviors fall out from XIP/non-XIP. The picasso boot flow also has an interesting component: romstage has bss and data, but bss isn't cleared from the loader.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36612 )
Change subject: arch/x86: Link .bss and .data in in-CAR stages ......................................................................
Abandoned
Improved in CB:48156
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)