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.