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