Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add ENV_BEFORE_RAMSTAGE ......................................................................
Add ENV_BEFORE_RAMSTAGE
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/riscv/include/arch/memlayout.h M src/include/rules.h M src/soc/samsung/exynos5250/alternate_cbfs.c M src/soc/samsung/exynos5420/alternate_cbfs.c 4 files changed, 11 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/1
diff --git a/src/arch/riscv/include/arch/memlayout.h b/src/arch/riscv/include/arch/memlayout.h index 7baab76..ebedb2d 100644 --- a/src/arch/riscv/include/arch/memlayout.h +++ b/src/arch/riscv/include/arch/memlayout.h @@ -20,7 +20,7 @@
#define STACK(addr, size) REGION(stack, addr, size, 4096)
-#if defined(__PRE_RAM__) +#if ENV_BEFORE_RAMSTAGE #define CAR_STACK(addr, size) \ REGION(car_stack, addr, size, 4K) \ ALIAS_REGION(car_stack, stack) diff --git a/src/include/rules.h b/src/include/rules.h index 10cd715..530404d 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -266,6 +266,8 @@ #define ENV_PAYLOAD_LOADER ENV_RAMSTAGE #endif
+#define ENV_BEFORE_RAMSTAGE (ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE || ENV_POSTCAR) + /** * For pre-DRAM stages and post-CAR always build with simple device model, ie. * PCI, PNP and CPU functions operate without use of devicetree. The reason diff --git a/src/soc/samsung/exynos5250/alternate_cbfs.c b/src/soc/samsung/exynos5250/alternate_cbfs.c index e431672..a012837 100644 --- a/src/soc/samsung/exynos5250/alternate_cbfs.c +++ b/src/soc/samsung/exynos5250/alternate_cbfs.c @@ -35,12 +35,12 @@ * should contain all available stages/payloads/etc. It is loaded when this * function is called a second time at the end of the romstage, and copied to * the romstage/ramstage CBFS cache in DRAM. It will reside there for the - * rest of the firmware's lifetime and all subsequent stages (which will not - * have __PRE_RAM__ defined) can just directly reference it there. + * rest of the firmware's lifetime and all subsequent stages can just directly + * reference it there. */ static int usb_cbfs_open(void) { -#ifdef __PRE_RAM__ +#if ENV_BEFORE_RAMSTAGE static int first_run = 1; int (*irom_load_usb)(void) = *irom_load_image_from_usb_ptr;
@@ -75,7 +75,7 @@ */ static int sdmmc_cbfs_open(void) { -#ifdef __PRE_RAM__ +#if ENV_BEFORE_RAMSTAGE /* * In the bootblock, we just copy the small part that fits in the buffer * and hope that it's enough (since the romstage is currently always the diff --git a/src/soc/samsung/exynos5420/alternate_cbfs.c b/src/soc/samsung/exynos5420/alternate_cbfs.c index ba3f9a3..b8f3e04 100644 --- a/src/soc/samsung/exynos5420/alternate_cbfs.c +++ b/src/soc/samsung/exynos5420/alternate_cbfs.c @@ -36,12 +36,12 @@ * should contain all available stages/payloads/etc. It is loaded when this * function is called a second time at the end of the romstage, and copied to * the romstage/ramstage CBFS cache in DRAM. It will reside there for the - * rest of the firmware's lifetime and all subsequent stages (which will not - * have __PRE_RAM__ defined) can just directly reference it there. + * rest of the firmware's lifetime and all subsequent stages can just directly + * reference it there. */ static int usb_cbfs_open(void) { -#ifdef __PRE_RAM__ +#if ENV_BEFORE_RAMSTAGE static int first_run = 1; int (*irom_load_usb)(void) = *irom_load_image_from_usb_ptr;
@@ -79,7 +79,7 @@ */ static int sdmmc_cbfs_open(void) { -#ifdef __PRE_RAM__ +#if ENV_BEFORE_RAMSTAGE /* * In the bootblock, we just copy the small part that fits in the buffer * and hope that it's enough (since the romstage is currently always the
Hello ron minnich, Julius Werner, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#3).
Change subject: Add new ENV_xxx definitions ......................................................................
Add new ENV_xxx definitions
ENV_HAS_DATA_SECTION ENV_HAS_BSS_SECTION
ENV_BEFORE_POSTCAR ENV_BEFORE_RAMSTAGE
MAYBE_BSS
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/riscv/include/arch/memlayout.h M src/cpu/Kconfig M src/device/device_const.c M src/ec/google/chromeec/ec_lpc.c M src/include/rules.h M src/include/stddef.h M src/include/symbols.h M src/lib/lzma.c M src/lib/timestamp.c M src/mainboard/google/stout/chromeos.c M src/security/tpm/tspi/log.c M src/soc/intel/baytrail/northcluster.c M src/soc/intel/braswell/northcluster.c 13 files changed, 42 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/3
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 4:
Copied over what Julius W commented in CB:34921
Patch looks good to me functionally, but is this really something we want to do? Is your goal to remove __PRE_RAM__ completely? I agree that a define that needs to be handled with #ifdef (rather than #if or if()) is ugly, but we could fix that by just creating an ENV_PRE_RAM in <rules.h> instead. I do think that the distinction of "are we in a stage that runs from DRAM" is generally useful -- even if ramstage is currently the only stage which that applies to on non-x86 boards, I wouldn't bet that that will never change in the future. Just to hedge against that possibility, I think it makes more sense to have constants that are named directly after the property you're really interested in.
And if we do intend to keep pre-RAM as a separate distinction, some of these cases should keep that rather than checking for the stage.
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#5).
Change subject: Add new ENV_xxx definitions ......................................................................
Add new ENV_xxx definitions
ENV_HAS_DATA_SECTION ENV_HAS_BSS_SECTION
ENV_BEFORE_POSTCAR ENV_BEFORE_RAMSTAGE
MAYBE_BSS
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/riscv/include/arch/memlayout.h M src/cpu/Kconfig M src/device/device_const.c M src/ec/google/chromeec/ec_lpc.c M src/include/rules.h M src/include/stddef.h M src/include/symbols.h M src/lib/lzma.c M src/lib/timestamp.c M src/mainboard/google/stout/chromeos.c M src/security/tpm/tspi/log.c M src/soc/intel/baytrail/northcluster.c M src/soc/intel/braswell/northcluster.c 13 files changed, 42 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig@19 PS5, Line 19: default n Did this sneak in here? Add some comments on usage and meaning?
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@278 PS5, Line 278: #define ENV_HAS_DATA_SECTION ENV_PROGRAM_IN_RAM I think there are some platforms where .data is used. But my bigger question is how you envision tying this to ARCH_STAGE_HAS_DATA_SECTION ?
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@279 PS5, Line 279: #define ENV_HAS_BSS_SECTION !(ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION)) And ARCH_STAGE_HAS_BSS_SECTION here.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@269 PS5, Line 269: #define ENV_BEFORE_POSTCAR \ If this is supposed to be the new generic __PRE_RAM__, I'd prefer a more generic name like ENV_PRE_RAM, ENV_BEFORE_DRAM or something like that. postcar is very x86-specific.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@278 PS5, Line 278: #define ENV_HAS_DATA_SECTION ENV_PROGRAM_IN_RAM
I think there are some platforms where .data is used. […]
Yes, non-x86 stages always have a .data section and ARCH_STAGE_HAS... should solve this already (although moving those into rules.h may be a good idea to keep all these things in one place).
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@298 PS5, Line 298: #define ENV_CACHE_AS_RAM !ENV_PROGRAM_IN_RAM Do we even need two things that are just exact opposites of another? I think in that case, having only one (and testing for the negation where appropriate) would create less chance of confusion.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@48 PS5, Line 48: #define MAYBE_BSS I think this doesn't really reflect what it means anymore, especially if someone were to stumble across it in code without knowing the macro. The most important thing is that it is used like the 'static' keyword.
So maybe these should be MAYBE_STATIC_BSS and MAYBE_STATIC_NONZERO instead? (Not really happy with those names either but I can't think of anything better.)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS I don't think we should hide this in another macro. Marking local variables static has other effects on behavior than just saving stack space, that needs to be clearly visible to people who write code with this.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h@75 PS5, Line 75: return !CONFIG(ARCH_X86) || ENV_BEFORE_POSTCAR; So... why does the behavior of ENV_CACHE_AS_RAM change, actually? I take that option to mean "the car.ld linker script is in effect", which as I understand isn't true for postcar.
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 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig@19 PS5, Line 19: default n
Did this sneak in here? Add some comments on usage and meaning?
This was anticipation for CB:32414 without pulling that commit into my branch.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@269 PS5, Line 269: #define ENV_BEFORE_POSTCAR \
If this is supposed to be the new generic __PRE_RAM__, I'd prefer a more generic name like ENV_PRE_R […]
Well.. anything PRE_RAM was already wrong for ARMs where we are in SRAM/DRAM starting from bootblock already. And with case of Picasso, ARCH_X86 && RESET_VECTOR_IN_RAM, anything PRE_RAM becomes obscure as well. I think we have ~15 cases of __PRE_RAM__ use that actually need solving, all the rest is just noise (spurious guards).
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@273 PS5, Line 273: (ENV_BEFORE_POSTCAR || ENV_POSTCAR) This can be defined listign PRE_RAM stages explicitly, to avoid ENV_BEFORE_POSTCAR.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@278 PS5, Line 278: #define ENV_HAS_DATA_SECTION ENV_PROGRAM_IN_RAM
Yes, non-x86 stages always have a .data section and ARCH_STAGE_HAS... […]
I'll try to update this, leveraging existing defines from memlayout.h.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@298 PS5, Line 298: #define ENV_CACHE_AS_RAM !ENV_PROGRAM_IN_RAM
Do we even need two things that are just exact opposites of another? I think in that case, having on […]
Not sure, maybe not. All I know is that with __PRE_RAM__, use of !__PRE_RAM__ became obscure once we added __SMM__ and ENV_POSTCAR. I was about suggest to not allow the testing with a negation.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@48 PS5, Line 48: #define MAYBE_BSS
I think this doesn't really reflect what it means anymore, especially if someone were to stumble acr […]
I would do the renaming in separate commit. Either before or after this one.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS
I don't think we should hide this in another macro. […]
Aaron should comment, his suggestion.
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h@75 PS5, Line 75: return !CONFIG(ARCH_X86) || ENV_BEFORE_POSTCAR;
So... […]
Test is _BEFORE_ postcar. This is for the RESET_VECTOR_IN_RAM case from CB:32414, were we would never have ENV_CACHE_AS_RAM==1.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS
Aaron should comment, his suggestion.
Is there ever a case where we want that buffer on the stack? ramstage? and CAR_GLOBAL_MIGRATION platforms?
We either hide it or put all that logic in the compilation unit itself. I don't care either way. MAYBE_STATIC was being utilized before which isn't super explicit either in its semantics.
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#6).
Change subject: Add new ENV_xxx definitions ......................................................................
Add new ENV_xxx definitions
ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__.
ENV_ROMSTAGE_OR_BEFORE is an intermediate define for ARCH_X86 to indicate presence of pre-ram symbols, or the use of car.ld, for the stage. It also acts as a separator for ENV_POSTCAR, should we need it somewhere.
ENV_PROGRAM_IN_RAM is used to determine whether .data section in the stage is allowd.
Define ENV_CACHE_AS_RAM only locally as !ENV_PROGRAM_IN_RAM.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/memlayout.h M src/console/printk.c M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h 6 files changed, 21 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/6
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 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/rules.h@289 PS6, Line 289: #if !ENV_RAMSTAGE So I removed the !ENV_PAYLOAD_LOADER here. One cannot simply move sources from ramstage-y to postcar-y because the device model is different. See CB:32725, I think it is currently just all confusion that is not even build-tested.
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/symbols.h@73 PS6, Line 73: static inline int preram_symbols_available(void) Do we want ARCH_STAGE_HAS_PRERAM_SYMBOLS instead?
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#7).
Change subject: Add new ENV_xxx definitions ......................................................................
Add new ENV_xxx definitions
ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__.
ENV_ROMSTAGE_OR_BEFORE is an intermediate define for ARCH_X86 to indicate presence of pre-ram symbols, or the use of car.ld, for the stage. It also acts as a separator for ENV_POSTCAR, should we need it somewhere.
ENV_PROGRAM_IN_RAM is used to determine whether .data section in the stage is allowd.
Define ENV_CACHE_AS_RAM only locally as !ENV_PROGRAM_IN_RAM.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/memlayout.h M src/console/printk.c M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h 6 files changed, 21 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34939/7/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/7/src/include/rules.h@276 PS7, Line 276: !(CONFIG(ARCH_X86) && ENV_BEFORE_RAMSTAGE && !ENV_POSTCAR && !CONFIG(RESET_VECTOR_IN_RAM)) line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34939/8/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/8/src/include/rules.h@276 PS8, Line 276: !(CONFIG(ARCH_X86) && ENV_BEFORE_RAMSTAGE && !ENV_POSTCAR && !CONFIG(RESET_VECTOR_IN_RAM)) line over 96 characters
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 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS
Is there ever a case where we want that buffer on the stack? ramstage? and CAR_GLOBAL_MIGRATION plat […]
Assuming heap is preferred for ramstage, I would go and put the logic in lzma.c file. So I removed ALLOC_LARGE_BUFFER now.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34939/9/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/9/src/include/rules.h@276 PS9, Line 276: !(CONFIG(ARCH_X86) && ENV_BEFORE_RAMSTAGE && !ENV_POSTCAR && !CONFIG(RESET_VECTOR_IN_RAM)) line over 96 characters
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#10).
Change subject: Add new ENV_xxx definitions ......................................................................
Add new ENV_xxx definitions
ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__.
ENV_ROMSTAGE_OR_BEFORE is an intermediate define for ARCH_X86 to indicate presence of pre-ram symbols, or the use of car.ld, for the stage. It also acts as a separator for ENV_POSTCAR, should we need it somewhere.
ENV_PROGRAM_IN_RAM is used to determine whether .data section in the stage is allowd.
Define ENV_CACHE_AS_RAM only locally as !ENV_PROGRAM_IN_RAM.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/early_variables.h M src/arch/x86/include/arch/memlayout.h M src/console/printk.c M src/cpu/x86/pae/pgtbl.c M src/include/rules.h M src/include/symbols.h 6 files changed, 21 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/10
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 10:
(5 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... seems like an odd reason? Why not change it right away?
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. Why not ENV_PRE_DRAM (or flip to ENV_PROGRAM_IN_DRAM)?
It also means that this will be true for the romstage in DRAM for pistachio. Is that actually what we want? I think in most cases you really want to know whether you're actually running from DRAM (e.g. below you have to explicitly OR in RESET_VECTOR_IN_DRAM because that's not reflected with this macro).
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. If you want this to include SRAM execution, how about flipping it around and calling it ENV_PROGRAM_IN_CAR? (Then again that sounds pretty close to ENV_CACHE_AS_RAM and I'm not sure why you're trying to get rid of that anyway? I think that was a good name...)
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@269 PS5, Line 269: #define ENV_BEFORE_POSTCAR \
Well.. […]
I think for most of us working on Arm, when we say "RAM" we mean DRAM. There's still sometimes a need to check whether we're in an SRAM or DRAM stage, and we used __PRE_RAM__ for that before.
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; I don't think(?) this should be true for pistachio romstage? (Because it's defined in car.ld and I assume they're not using that?) Maybe another reason to have a macro that's actually about whether we're linked into DRAM instead?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 10:
(1 comment)
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;
I don't think(?) this should be true for pistachio romstage? (Because it's defined in car. […]
(Sorry, I mean picasso, not pistachio. Whatever the new AMD thing is.)
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.
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:
I would like to have CB:34980 and CB:34928 submitted before next rebase of this topic branch. We then have majority of spurious __PRE_RAM__ guards removed in master and can discuss the remaining cases where these ENV_xxx need to be evaluated.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add new ENV_xxx definitions ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG@9 PS10, Line 9: ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__. I think it would make more sense that ENV_ROMSTAGE_OR_BEFORE is the new generic replacement for __PRE_RAM__, and ENV_BEFORE_RAMSTAGE can be used where postcar must explicitly be included. Just because no other architectures currently have special stages between romstage and ramstage doesn't mean they never will. (In fact, that used to be possible for a while on all architectures with the combination of CONFIG_VBOOT_STARTS_IN_ROMSTAGE and CONFIG_VBOOT_SEPARATE_VERSTAGE already, until we removed that case.)
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@276 PS10, Line 276: !CONFIG(ARCH_X86)
ENV_CACHE_AS_RAM is poor in the sense it does not give any clue that you cannot have mutable . […]
Well, if you want to know whether you have a .data section you should use ARCH_STAGE_HAS_DATA_SECTION anyway. Maybe the right answer here is that we should only use that and don't need another option next to it. (Maybe we should rename those to ENV_HAVE_DATA_SECTION, etc. as well to make naming a bit more consistent.)
Anyway, having an ENV_PROGRAM_IN_RAM that includes SRAM will be very confusing to people working on those systems, let's please not do that.
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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG@9 PS10, Line 9: ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__.
I think it would make more sense that ENV_ROMSTAGE_OR_BEFORE is the new generic replacement for __PR […]
I am fine with either. Let's wait until we can review (pending) followup work to see how much we really need to use this.
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@276 PS10, Line 276: !CONFIG(ARCH_X86)
Well, if you want to know whether you have a . […]
As this patchset demonstrates, ENV_CACHE_AS_RAM seems to have no use outside x86 <arch/early_variables.h> and we could wipe it out with next release.
Looking into non-x86 arch, I see three cases of __PRE_RAM__ left, CB:34892. The people workin on systems with SRAM never needed to make the distinction between SRAM/DRAM? I'll name it _IN_DRAM now, let's see what the followups utilising these end up looking like before making the final calls.
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#11).
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h>
ENV_ROMSTAGE_OR_BEFORE is a direct replacement for testing defined(__PRE_RAM__) as a true statement instead of with the help of the preprocessor.
Note that for x86, due to existence of ENV_POSTCAR and ENV_SMM, ENV_ROMSTAGE_OR_BEFORE and ENV_RAMSTAGE are not the inverse of each other.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/printk.c M src/include/rules.h 2 files changed, 8 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34939/11/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/11/src/include/rules.h@274 PS11, Line 274: #define ENV_STAGE_HAS_DATA_SECTION !ENV_ROMSTAGE_OR_BEFORE please, no space before tabs
https://review.coreboot.org/c/coreboot/+/34939/11/src/include/rules.h@278 PS11, Line 278: #define ENV_STAGE_HAS_DATA_SECTION 1 please, no space before tabs
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#12).
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h>
ENV_ROMSTAGE_OR_BEFORE is a direct replacement for testing defined(__PRE_RAM__) as a true statement instead of with the help of the preprocessor.
Note that for x86, due to existence of ENV_POSTCAR and ENV_SMM, ENV_ROMSTAGE_OR_BEFORE and ENV_RAMSTAGE are not the inverse of each other.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/printk.c M src/include/rules.h 2 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/12
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34939/10//COMMIT_MSG@9 PS10, Line 9: ENV_BEFORE_RAMSTAGE is the generic replacement for __PRE_RAM__.
I am fine with either. […]
Done
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. */
How many times do we want to rewrite this just because a longer name might hit 96 line lengths until […]
CB:35016
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@276 PS10, Line 276: !CONFIG(ARCH_X86)
As this patchset demonstrates, ENV_CACHE_AS_RAM seems to have no use outside x86 <arch/early_variabl […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 12: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 12:
(10 comments)
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 \
My impression is we rarely need to know if we are executing from DRAM or XIP with cache-as-ram. […]
Ack
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@269 PS5, Line 269: #define ENV_BEFORE_POSTCAR \
I think for most of us working on Arm, when we say "RAM" we mean DRAM. […]
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@273 PS5, Line 273: (ENV_BEFORE_POSTCAR || ENV_POSTCAR)
This can be defined listign PRE_RAM stages explicitly, to avoid ENV_BEFORE_POSTCAR.
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@278 PS5, Line 278: #define ENV_HAS_DATA_SECTION ENV_PROGRAM_IN_RAM
I'll try to update this, leveraging existing defines from memlayout.h.
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@279 PS5, Line 279: #define ENV_HAS_BSS_SECTION !(ENV_ROMSTAGE && CONFIG(CAR_GLOBAL_MIGRATION))
And ARCH_STAGE_HAS_BSS_SECTION here.
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/rules.h@298 PS5, Line 298: #define ENV_CACHE_AS_RAM !ENV_PROGRAM_IN_RAM
Not sure, maybe not. […]
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@48 PS5, Line 48: #define MAYBE_BSS
I would do the renaming in separate commit. Either before or after this one.
Done
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/stddef.h@52 PS5, Line 52: #define ALLOC_LARGE_BUFFER MAYBE_BSS
Assuming heap is preferred for ramstage, I would go and put the logic in lzma.c file. […]
Ack
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;
Well the current iteration of Picasso is using car.ld. […]
Ack
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/5/src/include/symbols.h@75 PS5, Line 75: return !CONFIG(ARCH_X86) || ENV_BEFORE_POSTCAR;
Test is _BEFORE_ postcar. […]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/34939/5/src/cpu/Kconfig@19 PS5, Line 19: default n
This was anticipation for CB:32414 without pulling that commit into my branch.
Ack
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/rules.h@289 PS6, Line 289: #if !ENV_RAMSTAGE
So I removed the !ENV_PAYLOAD_LOADER here. […]
Ack
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/c/coreboot/+/34939/6/src/include/symbols.h@73 PS6, Line 73: static inline int preram_symbols_available(void)
Do we want ARCH_STAGE_HAS_PRERAM_SYMBOLS instead?
Ack
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Martin Roth, Furquan Shaikh, Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34939
to look at the new patch set (#13).
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h>
ENV_ROMSTAGE_OR_BEFORE is a direct replacement for testing defined(__PRE_RAM__) as a true statement instead of with the help of the preprocessor.
Note that for x86, due to existence of ENV_POSTCAR and ENV_SMM, ENV_ROMSTAGE_OR_BEFORE and ENV_RAMSTAGE are not the inverse of each other.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/printk.c M src/include/rules.h 2 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34939/13
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Patch Set 14: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34939 )
Change subject: Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h> ......................................................................
Add definition for ENV_ROMSTAGE_OR_BEFORE to <rules.h>
ENV_ROMSTAGE_OR_BEFORE is a direct replacement for testing defined(__PRE_RAM__) as a true statement instead of with the help of the preprocessor.
Note that for x86, due to existence of ENV_POSTCAR and ENV_SMM, ENV_ROMSTAGE_OR_BEFORE and ENV_RAMSTAGE are not the inverse of each other.
Change-Id: Ibd2292f922ccb9e79d10ca9bc35797048d174287 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34939 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/console/printk.c M src/include/rules.h 2 files changed, 7 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/console/printk.c b/src/console/printk.c index 8606bbb..1165226 100644 --- a/src/console/printk.c +++ b/src/console/printk.c @@ -49,8 +49,7 @@ { int i, log_this;
- if (CONFIG(SQUELCH_EARLY_SMP) && ENV_CACHE_AS_RAM && - !boot_cpu()) + if (CONFIG(SQUELCH_EARLY_SMP) && ENV_ROMSTAGE_OR_BEFORE && !boot_cpu()) return 0;
log_this = console_log_level(msg_level); diff --git a/src/include/rules.h b/src/include/rules.h index ed14722..8a6945b 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -266,9 +266,12 @@ #define ENV_PAYLOAD_LOADER ENV_RAMSTAGE #endif
+#define ENV_ROMSTAGE_OR_BEFORE \ + (ENV_DECOMPRESSOR || ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE) + #if CONFIG(ARCH_X86) -/* Indicates memory layout is determined by arch/x86/car.ld. */ -#define ENV_CACHE_AS_RAM (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_VERSTAGE) +/* Indicates memory layout is determined with arch/x86/car.ld. */ +#define ENV_CACHE_AS_RAM ENV_ROMSTAGE_OR_BEFORE /* No .data sections with execute-in-place from ROM. */ #define ENV_STAGE_HAS_DATA_SECTION !ENV_CACHE_AS_RAM /* No .bss sections with execute-in-place from ROM. */ @@ -294,7 +297,7 @@ * be built with simple device model. */
-#if (defined(__PRE_RAM__) || ENV_SMM || !ENV_PAYLOAD_LOADER) +#if !ENV_RAMSTAGE #define __SIMPLE_DEVICE__ #endif
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)