Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
rules.h: Add ENV_EARLY_RAM
Add a definition for environments where the x86's reset vector is in DRAM and a unique linker file must be used to coordinate regions across stages.
Change-Id: I03703ae37a835de08ad8c905bafa504bdc41e959 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/include/rules.h 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/37486/1
diff --git a/src/include/rules.h b/src/include/rules.h index fa60ede..1947ede 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -255,8 +255,10 @@ #if CONFIG(ARCH_X86) /* Indicates memory layout is determined with arch/x86/car.ld. */ #define ENV_CACHE_AS_RAM (ENV_ROMSTAGE_OR_BEFORE && !CONFIG(RESET_VECTOR_IN_RAM)) +/* Reset vector is in DRAM, and memory layout is determined by its own .ld file. */ +#define ENV_EARLY_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 && !ENV_EARLY_RAM) /* No .bss sections for stage with CAR teardown. */ #define ENV_STAGE_HAS_BSS_SECTION 1 #else
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@259 PS1, Line 259: #define ENV_EARLY_RAM (ENV_ROMSTAGE_OR_BEFORE && CONFIG(RESET_VECTOR_IN_RAM)) Since all stages have mutable memory, we should have ENV_EARLY_RAM = !ENV_CACHE_AS_RAM, it is one or the other. Should we keep this, there should be an accompanying #define for !ARCH_X86.
But I don't see why we would need to add this, are there some if (ENV_EARLY_RAM) cases on your patchtrain?
I don't mind if you rename RESET_VECTOR_IN_RAM, looking at the direction things are going, I believe we can do this all with a single Kconfig that acts as a selector between car.ld and early_ram.ld and then use it elsewhere.
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@261 PS1, Line 261: #define ENV_STAGE_HAS_DATA_SECTION (!ENV_CACHE_AS_RAM && !ENV_EARLY_RAM) I don't see why you would need to restrict this. Aaron has also suggested to add .data to car.ld and I would welcome that change.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37486
to look at the new patch set (#2).
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
rules.h: Add ENV_EARLY_RAM
Add a definition to indicate environments where the x86's reset vector is in DRAM and execution is in any stage relying on a strict memory map.
Change-Id: I03703ae37a835de08ad8c905bafa504bdc41e959 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/include/rules.h 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/37486/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@259 PS1, Line 259: #define ENV_EARLY_RAM (ENV_ROMSTAGE_OR_BEFORE && CONFIG(RESET_VECTOR_IN_RAM)) I read ENV_CAR (and I wrote ENV_EARLY_RAM) to indicate any time when that mode is active, not that they're the opposite of each other. So ENV_CACHE_AS_RAM=1 when CAR is active (bootblock, verstage, romstage) and =0 ramstage, etc. I see them as more analogous than opposite.
there should be an accompanying #define for !ARCH_X86.
Missed that.
...are there some if (ENV_EARLY_RAM) cases on your patchtrain?
See CB:37487. I'd be happy to squash it if it makes it more clear. program.ld is the only place it'll be used AFAIK.
I don't mind if you rename RESET_VECTOR_IN_RAM, looking at the direction things are going
My biggest complaint with the name is it's too generic of a name for a specific implementation. If a competitor made a product that also started executing in DRAM, I don't know that the x86's behavior would be compatible with this implementation.
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@261 PS1, Line 261: #define ENV_STAGE_HAS_DATA_SECTION (!ENV_CACHE_AS_RAM && !ENV_EARLY_RAM)
I don't see why you would need to restrict this. Aaron has also suggested to add .data to car. […]
The elf-to-image creation/compression feature of cbfstool only takes the elf file's PROGBITS. So without more work in that area, all data is effectively initialized to 0 due to zeroing the buffer before compressing. I'm curious how non-ro .data in CAR w/bootblock would work. How would the initialized data get loaded into the data location? BTW, my first question was actually "what's the downside of having no .data in the earliest stages?"
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@259 PS1, Line 259: #define ENV_EARLY_RAM (ENV_ROMSTAGE_OR_BEFORE && CONFIG(RESET_VECTOR_IN_RAM))
I read ENV_CAR (and I wrote ENV_EARLY_RAM) to indicate any time when that mode is active, not that t […]
I left a comment on CB:37487. Anyways, someone else will decide about the +2 for this one, probably Aaron or Julius.
I think the current choice of name is very descriptive for choosing between two linker scripts, one for XIP+CAR another for loading reset vector and program into RAM.
https://review.coreboot.org/c/coreboot/+/37486/1/src/include/rules.h@261 PS1, Line 261: #define ENV_STAGE_HAS_DATA_SECTION (!ENV_CACHE_AS_RAM && !ENV_EARLY_RAM)
The elf-to-image creation/compression feature of cbfstool only takes the elf file's PROGBITS. […]
Hmm.. but .data would be PROGBITS? For XIP romstage, runtime needs to be modified to load .data in to CAR region, but with non-XIP you would get this for free, copied to RAM with the rest of program?
The benefit: less deviation from standard C for developer? The only place right now I think would benefit from this would be DECLARE_SPINLOCK() .
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
Patch Set 4:
obsolete approach?
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37486 )
Change subject: rules.h: Add ENV_EARLY_RAM ......................................................................
Abandoned
Yes, this is an old approach. I missed abandoning it before.
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)