Change in coreboot[master]: [RFC]arch/x86: Only allow normal and fallback prefix

[Arthur Heymans (Code Review)]

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

Change subject: [RFC]arch/x86: Only allow normal and fallback prefix
......................................................................

[RFC]arch/x86: Only allow normal and fallback prefix

Remove the functionality to use other prefixes which requires a
coreboot-stages cbfs file. Specifying the stage prefix in Kconfig and
making a properly formatted coreboot-stages cbfs file is errorprone,
so get rid of the functionality assuming it's mostly unused.

Change-Id: I04222120bd1241c3b0996afa27dcc35ac42fbbc8
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/bootblock_normal.c
2 files changed, 20 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/35822/1

diff --git a/src/Kconfig b/src/Kconfig
index 8fcb3ae..baf406f 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -37,12 +37,26 @@
    help
      Select this to prompt to use to configure the prefix for cbfs files.

+choice
+	prompt "CBFS prefix to use"
+	depends on CONFIGURABLE_CBFS_PREFIX
+	default CBFS_PREFIX_FALLBACK
+
+config CBFS_PREFIX_FALLBACK
+	bool "fallback"
+
+config CBFS_PREFIX_NORMAL
+	bool "normal"
+
+endchoice
+
 config CBFS_PREFIX
-	string "CBFS prefix to use" if CONFIGURABLE_CBFS_PREFIX
-	default "fallback"
+	string
+	default "fallback" if !CONFIGURABLE_CBFS_PREFIX || CBFS_PREFIX_FALLBACK
+	default "normal" if CBFS_PREFIX_NORMAL
    help
      Select the prefix to all files put into the image. It's "fallback"
-	  by default, "normal" is a common alternative.
+	  by default, "normal" is the alternative.

choice
    prompt "Compiler to use"
diff --git a/src/arch/x86/bootblock_normal.c b/src/arch/x86/bootblock_normal.c
index 905ecb2..9cc99f7 100644
--- a/src/arch/x86/bootblock_normal.c
+++ b/src/arch/x86/bootblock_normal.c
@@ -26,7 +26,7 @@
 static void main(unsigned long bist)
 {
    u8 boot_mode;
-	const char *default_filenames =
+	const char *romstage_filenames =
    	"normal/romstage\0fallback/romstage";

if (boot_cpu()) {
@@ -44,20 +44,15 @@
    	boot_mode = boot_use_normal(cmos_read(RTC_BOOT_BYTE));
    }

-	char *normal_candidate = (char *)walkcbfs("coreboot-stages");
-
-	if (!normal_candidate)
-		normal_candidate = default_filenames;
-
    unsigned long entry;

if (boot_mode) {
-		entry = findstage(normal_candidate);
+		entry = findstage(romstage_filenames);
    	if (entry)
    		call(entry, bist);
    }

-	entry = findstage(get_fallback(normal_candidate));
+	entry = findstage(get_fallback(romstage_filenames));
    if (entry)
    	call(entry, bist);

[Arthur Heymans (Code Review)]

Hello Kyösti Mälkki, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: [RFC]arch/x86: Only allow normal and fallback prefix
......................................................................

[RFC]arch/x86: Only allow normal and fallback prefix

Remove the functionality to use other prefixes which requires a
coreboot-stages cbfs file. Specifying the stage prefix in Kconfig and
making a properly formatted coreboot-stages cbfs file is errorprone,
so get rid of the functionality assuming it's mostly unused.

Change-Id: I04222120bd1241c3b0996afa27dcc35ac42fbbc8
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
M src/arch/x86/bootblock_normal.c
2 files changed, 19 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/35822/2

[Nico Huber (Code Review)]

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

Change subject: [RFC]arch/x86: Only allow normal and fallback prefix
......................................................................

Patch Set 2:

(1 comment)

I wonder if it would be hard to maintain the current Kconfig
rules for romcc bootblocks and just restrict them for C-env
ones?

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig 
File src/Kconfig:

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig@50 
PS2, Line 50: 
Could we just add a third choice here that depends on !C_ENV
and would enable the prompt below?

[Arthur Heymans (Code Review)]

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

Change subject: [RFC]arch/x86: Only allow normal and fallback prefix
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig 
File src/Kconfig:

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig@50 
PS2, Line 50:
> Could we just add a third choice here that depends on !C_ENV […]

Sure, I don't think it's a good idea to have such options but then at least no-one will be hurt by this change.

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Arthur Heymans (Code Review)]

Hello Kyösti Mälkki, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

arch/x86: Add a choice for selecting normal/fallback cbfs prefix

Setting the cbfs prefix is prone to error. Therefore add a Kconfig
choice for 2 common values, fallback and normal, while still keeping
the ability to specify an arbitrary value.

Change-Id: I04222120bd1241c3b0996afa27dcc35ac42fbbc8
Signed-off-by: Arthur Heymans 
---
M src/Kconfig
1 file changed, 19 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/35822/3

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Nico Huber (Code Review)]

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

Patch Set 3: Code-Review+2

Breaks compatibility with existing .config files, but
generally it seems for the greater good.

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

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

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

Patch Set 3:

CB:438 (!)

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Arthur Heymans (Code Review)]

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

Patch Set 3:
> Patch Set 3:
>
> CB:438 (!)

I don't think the argument about needing 'Kconfig' surgery in external tree's is a good argument as you need way more than just Kconfig surgery to make it work with more than 2 prefixes. With 2 prefixes all you can do is a cosmetic renaming. And even if those external trees have that, the option for a user selectable Kconfig is still there.

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Nico Huber (Code Review)]

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

Patch Set 3:
> > CB:438 (!)
>
> I don't think the argument about needing 'Kconfig' surgery in external tree's is a good argument as you need way more than just Kconfig surgery to make it work with more than 2 prefixes. With 2 prefixes all you can do is a cosmetic renaming. And even if those external trees have that, the option for a user selectable Kconfig is still there.

Ack, compared to CB:438, the option to enter a custom prefix
is kept. We could still add the rule that one can only change
the bootblock behaviour with "fallback" selected. Or maybe
make it depend on the update-existing-image option. All
bootblock-affecting options are void in the update case.

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Nico Huber (Code Review)]

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

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig 
File src/Kconfig:

https://review.coreboot.org/c/coreboot/+/35822/2/src/Kconfig@50 
PS2, Line 50:
> Sure, I don't think it's a good idea to have such options but then at least no-one will be hurt by t […]

Done

Change in coreboot[master]: arch/x86: Add a choice for selecting normal/fallback cbfs prefix

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35822 )

Change subject: arch/x86: Add a choice for selecting normal/fallback cbfs prefix
......................................................................

arch/x86: Add a choice for selecting normal/fallback cbfs prefix

Setting the cbfs prefix is prone to error. Therefore add a Kconfig
choice for 2 common values, fallback and normal, while still keeping
the ability to specify an arbitrary value.

Change-Id: I04222120bd1241c3b0996afa27dcc35ac42fbbc8
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35822
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/Kconfig
1 file changed, 19 insertions(+), 2 deletions(-)

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

diff --git a/src/Kconfig b/src/Kconfig
index 8fcb3ae..4c71f28 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -37,9 +37,26 @@
    help
      Select this to prompt to use to configure the prefix for cbfs files.

+choice
+	prompt "CBFS prefix to use"
+	depends on CONFIGURABLE_CBFS_PREFIX
+	default CBFS_PREFIX_FALLBACK
+
+config CBFS_PREFIX_FALLBACK
+	bool "fallback"
+
+config CBFS_PREFIX_NORMAL
+	bool "normal"
+
+config CBFS_PREFIX_DIY
+	bool "Define your own cbfs prefix"
+
+endchoice
+
 config CBFS_PREFIX
-	string "CBFS prefix to use" if CONFIGURABLE_CBFS_PREFIX
-	default "fallback"
+	string "CBFS prefix to use" if CBFS_PREFIX_DIY
+	default "fallback" if !CONFIGURABLE_CBFS_PREFIX || CBFS_PREFIX_FALLBACK
+	default "normal" if CBFS_PREFIX_NORMAL
    help
      Select the prefix to all files put into the image. It's "fallback"
      by default, "normal" is a common alternative.