Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34939/10/src/arch/x86/include/arch/... File src/arch/x86/include/arch/early_variables.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/arch/x86/include/arch/... PS10, Line 23: /* Just defined locally to avoid rewriting this file. */
That... […]
How many times do we want to rewrite this just because a longer name might hit 96 line lengths until we agree whether it is !ENV_PROGRAM_IN_RAM or something else here. Besides, this file is on kill-list for next release.
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h@269 PS10, Line 269: #define ENV_ROMSTAGE_OR_BEFORE \
I guess this works for all architectures, but I still think it sounds very clunky. […]
My impression is we rarely need to know if we are executing from DRAM or XIP with cache-as-ram. We are covering .data and .bss existence via ARCH_STAGE_HAS_xxx_SECTION with this iteration of the patchset.
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/rules.h@276 PS10, Line 276: !CONFIG(ARCH_X86)
When I read "in RAM", I'm thinking DRAM. […]
ENV_CACHE_AS_RAM is poor in the sense it does not give any clue that you cannot have mutable .data section. I think ENV_PROGRAM_IN_RAM is better in this perspective, as .data is part of a loaded program.
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/10/src/include/symbols.h@75 PS10, Line 75: return !CONFIG(ARCH_X86) || ENV_ROMSTAGE_OR_BEFORE;
(Sorry, I mean picasso, not pistachio. Whatever the new AMD thing is. […]
Well the current iteration of Picasso is using car.ld. Whether it was CAR or RAM, stages before ramstage can have set of pre-ram symbols using fixed locations in the linker scripts.