Change in coreboot[master]: rules: Add ability to disable ENV_CACHE_AS_RAM

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34913 )

Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM
......................................................................

rules: Add ability to disable ENV_CACHE_AS_RAM

Change-Id: I3acace92b39093cfb51f71f0dd39582c1e3c8c9a
Signed-off-by: Marshall Dawson 
---
M src/Kconfig
M src/include/rules.h
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/34913/1

diff --git a/src/Kconfig b/src/Kconfig
index f051216..1bcca23 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -159,6 +159,12 @@
      Select this option if the soc implements custom assembly entry code
      for early stages.

+config NO_ENV_CACHE_AS_RAM
+	bool
+	depends on ARCH_X86
+	help
+	  Select this option if the soc doesn't use ENV_CACHE_AS_RAM.
+
 config COMPRESS_BOOTBLOCK
    bool
    depends on HAVE_BOOTBLOCK
diff --git a/src/include/rules.h b/src/include/rules.h
index 10cd715..cda13bb 100644
--- a/src/include/rules.h
+++ b/src/include/rules.h
@@ -284,7 +284,7 @@
 /* x86 specific. Indicates that the current stage is running with cache-as-ram
  * enabled from the beginning of the stage in C code. */
 #if defined(__PRE_RAM__)
-#define ENV_CACHE_AS_RAM CONFIG(ARCH_X86)
+#define ENV_CACHE_AS_RAM (CONFIG(ARCH_X86) && !CONFIG(NO_ENV_CACHE_AS_RAM))
 #else
 #define ENV_CACHE_AS_RAM 0
 #endif

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34913 )

Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM
......................................................................

Patch Set 2:

This should probably be in src/arch/x86/Kconfig and be called X86_NO_CACHE_AS_RAM or something like that, since non-x86 devices also don't use cache-as-RAM but they're not going to select this. On the other hand, there's already CONFIG_CACHE_AS_RAM... why don't you just use that instead?

(Also, honestly, this whole thing looks to me again like hacks upon hacks to support a weird design you probably shouldn't have picked in the first place. I assume you need this because your "romstage in DRAM" still has __PRE_RAM__ defined... but that doesn't make any sense in itself already! __PRE_RAM__ literally means "this is not running in RAM", but your "romstage" *is* running in RAM. All over coreboot people have made assumptions about what this means that you are breaking. I guess you could hack up the Makefile instead to remove the -D__PRE_RAM__ from romstage for your SoC, but then you'd break a ton of other assumptions all over coreboot that romstage always runs with __PRE_RAM__. I think the only way you could really get this to not be a huge mess is by not using the romstage for something it wasn't meant to be, and instead putting your stuff into a custom ramstage entry point, or maybe an expansion of the postcar concept, or even another new stage specific to your SoC.)

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34913 )

Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM
......................................................................

Patch Set 2:

(1 comment)
> Patch Set 2:
>
> This should probably be in src/arch/x86/Kconfig and be called X86_NO_CACHE_AS_RAM or something like that, since non-x86 devices also don't use cache-as-RAM but they're not going to select this. On the other hand, there's already CONFIG_CACHE_AS_RAM... why don't you just use that instead?

CB:34806 intentionally removed it. Per the original use, CACHE_AS_RAM=n would have indicated romstage has to be built with romcc. Not a good idea to completely flip the definition, technically it would have worked.
> (Also, honestly, this whole thing looks to me again like hacks upon hacks to support a weird design you probably shouldn't have picked in the first place. I assume you need this because your "romstage in DRAM" still has __PRE_RAM__ defined... but that doesn't make any sense in itself already! __PRE_RAM__ literally means "this is not running in RAM", but your "romstage" *is* running in RAM. All over coreboot people have made assumptions about what this means that you are breaking. I guess you could hack up the Makefile instead to remove the -D__PRE_RAM__ from romstage for your SoC, but then you'd break a ton of other assumptions all over coreboot that romstage always runs with __PRE_RAM__. I think the only way you could really get this to not be a huge mess is by not using the romstage for something it wasn't meant to be, and instead putting your stuff into a custom ramstage entry point, or maybe an expansion of the postcar concept, or even another new stage specific to your SoC.)

I don't think the situation is that bad with __PRE_RAM__. I will bulk replace most of "#ifndef __PRE_RAM__" with "#if ENV_RAMSTAGE". Sometimes it may need to be "ENV_RAMSTAGE || ENV_SMM". I am hoping to see use of __PRE_RAM__ outside <rules.h> can be totally avoided.

https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h 
File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h@287 
PS2, Line 287: #define ENV_CACHE_AS_RAM (CONFIG(ARCH_X86) && !CONFIG(NO_ENV_CACHE_AS_RAM))
I would have made it look like this.

CONFIG(ARCH_X86) && !CONFIG(RESET_VECTOR_IN_DRAM)

In my approach, this RESET_VECTOR_IN_DRAM would also conditionalise uses of x86/car.ld in x86/memlayout.ld.

Looks like preram_symbols_available() in symbols.h needs a fix too.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34913 )

Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM
......................................................................

Patch Set 2:

(1 comment)

Kyösti, you convinced me on the name.  I've squashed the feature into https://review.coreboot.org/c/coreboot/+/32414 and called it "x86: Introduce RESET_VECTOR_IN_DRAM option".

Will abandon this change.

https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h 
File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/34913/2/src/include/rules.h@287 
PS2, Line 287: #define ENV_CACHE_AS_RAM (CONFIG(ARCH_X86) && !CONFIG(NO_ENV_CACHE_AS_RAM))
> ... this RESET_VECTOR_IN_DRAM would also conditionalise uses of x86/car.ld in x86/memlayout.ld.

I agree, however that causes a new problem -- not insurmountable but I'm still trying minimize changes outside the picasso directory.  We're relying on the fsp2_0 driver that uses the symbols _car_relocatable_data_end and _car_region_end for what we need.
> Looks like preram_symbols_available() in symbols.h needs a fix too.

Hmm, I overlooked that.  However, since I need to keep them in my hybrid romstage for FSP anyway, I think that should stay as-is.  It looks like vboot/common is the only place where that's used currently.  The google implementation of vboot will run on the PSP and not the x86.

[Marshall Dawson (Code Review)]

Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34913 )

Change subject: rules: Add ability to disable ENV_CACHE_AS_RAM
......................................................................

Abandoned

Squashed with https://review.coreboot.org/c/coreboot/+/32414/12 "x86: Introduce RESET_VECTOR_IN_DRAM option"