Change in ...coreboot[master]: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33324

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

Change-Id: Icf5e8fa18bea1cdfb85b8a4999d8fccea94d16b9
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/Kconfig
M src/arch/x86/bootblock_simple.c
3 files changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33324/1

diff --git a/src/Kconfig b/src/Kconfig
index d30aa99..aa02001 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -32,8 +32,11 @@
      the coreboot version number, so that you can easily distinguish
      boot logs of different boards from each other.

+config USE_CBFS_PREFIX
+       bool
+
 config CBFS_PREFIX
-	string "CBFS prefix to use"
+	string "CBFS prefix to use" if USE_CBFS_PREFIX
    default "fallback"
    help
      Select the prefix to all files put into the image. It's "fallback"
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index 827c1cb..36f2277 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -247,6 +247,7 @@
    bool "Always load fallback"

config BOOTBLOCK_NORMAL
+	select USE_CBFS_PREFIX
    bool "Switch to normal if CMOS says so"

endchoice
diff --git a/src/arch/x86/bootblock_simple.c b/src/arch/x86/bootblock_simple.c
index fc041c8..fd328a1 100644
--- a/src/arch/x86/bootblock_simple.c
+++ b/src/arch/x86/bootblock_simple.c
@@ -28,9 +28,9 @@
    }

#if CONFIG(VBOOT_SEPARATE_VERSTAGE)
-	const char *target1 = "fallback/verstage";
+	const char *target1 = "verstage";
 #else
-	const char *target1 = "fallback/romstage";
+	const char *target1 = "romstage";
 #endif

unsigned long entry;

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33324

to look at the new patch set (#2).

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

Change-Id: Icf5e8fa18bea1cdfb85b8a4999d8fccea94d16b9
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/Kconfig
M src/arch/x86/bootblock_simple.c
3 files changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33324/2

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33324

to look at the new patch set (#3).

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

Change-Id: Icf5e8fa18bea1cdfb85b8a4999d8fccea94d16b9
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/Kconfig
2 files changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33324/3

[Julius Werner (Code Review)]

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

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 3:

Doesn't this mean that the stages are all going to be called "/ramstage", "/payload", etc. (i.e. with leading slash)? I don't think we want that? If you want to do this, I think you need to first make the slash part of the prefix.

Also, while I do agree that the prefix is ugly (especially on non-x86 boards which can't even do the switching), I'd also caution that this is a biiig change in how things work that's going to effect a lot of people's muscle memory and scripts. There are plenty of hardcoded "fallback/..." in the Chrome OS build system and I bet in other setups as well. If you really want to do this, you should probably at least announce it on the mailing list first and give everyone time to prepare.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33324 )

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 3:
> Patch Set 3:
>
> Doesn't this mean that the stages are all going to be called "/ramstage", "/payload", etc. (i.e. with leading slash)? I don't think we want that? If you want to do this, I think you need to first make the slash part of the prefix.
>
> Also, while I do agree that the prefix is ugly (especially on non-x86 boards which can't even do the switching), I'd also caution that this is a biiig change in how things work that's going to effect a lot of people's muscle memory and scripts. There are plenty of hardcoded "fallback/..." in the Chrome OS build system and I bet in other setups as well. If you really want to do this, you should probably at least announce it on the mailing list first and give everyone time to prepare.

While getting rid of the prefix altogether should be done in the long run, that is not what this patch does.

All it does is removing/hiding the ability to configure the prefix when you're not using the bootblock_normal.c romcc bootblock, which is the only codepath that currently makes use of the prefix (switch between normal/fallback depending on rtc nvram bits).

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33324 )

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 3: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33324 )

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/33324/3/src/Kconfig 
File src/Kconfig:

https://review.coreboot.org/#/c/33324/3/src/Kconfig@35 
PS3, Line 35: config USE_CBFS_PREFIX
The name could be better because it's really just about showing the
prompt, although I have no good idea either. SHOW_CBFS_PREFIX?
CONFIGURE_CBFS_PREFIX?

[Julius Werner (Code Review)]

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

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 3: Code-Review+2

Right, sorry, I misread what this does. Carry on.

[Arthur Heymans (Code Review)]

Hello Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33324

to look at the new patch set (#4).

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

Change-Id: Icf5e8fa18bea1cdfb85b8a4999d8fccea94d16b9
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/Kconfig
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33324/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33324 )

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/33324/3/src/Kconfig 
File src/Kconfig:

https://review.coreboot.org/#/c/33324/3/src/Kconfig@35 
PS3, Line 35: config USE_CBFS_PREFIX
> The name could be better because it's really just about showing the […]

Done

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33324 )

Change subject: arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL
......................................................................

arch/x86/Kconfig: Hide the prefix option on all but BOOTBLOCK_NORMAL

Change-Id: Icf5e8fa18bea1cdfb85b8a4999d8fccea94d16b9
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33324
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/Kconfig
M src/arch/x86/Kconfig
2 files changed, 7 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/Kconfig b/src/Kconfig
index 5d74d67..72d826f 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -32,8 +32,13 @@
      the coreboot version number, so that you can easily distinguish
      boot logs of different boards from each other.

+config CONFIGURABLE_CBFS_PREFIX
+	bool
+	help
+	  Select this to prompt to use to configure the prefix for cbfs files.
+
 config CBFS_PREFIX
-	string "CBFS prefix to use"
+	string "CBFS prefix to use" if CONFIGURABLE_CBFS_PREFIX
    default "fallback"
    help
      Select the prefix to all files put into the image. It's "fallback"
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index 827c1cb..5b1304f 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -247,6 +247,7 @@
    bool "Always load fallback"

config BOOTBLOCK_NORMAL
+	select CONFIGURABLE_CBFS_PREFIX
    bool "Switch to normal if CMOS says so"

endchoice