Change in coreboot[master]: bootblock: Support normal/fallback mechanism again

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

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

bootblock: Support normal/fallback mechanism again

Change-Id: I7395e62f6682f4ef123da10ac125127a57711ec6
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/Kconfig
M src/lib/prog_loaders.c
2 files changed, 35 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/37760/1

diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index e27aec2..e3816a7 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -248,7 +248,6 @@
 choice
    prompt "Bootblock behaviour"
    default BOOTBLOCK_SIMPLE
-	depends on ROMCC_BOOTBLOCK

config BOOTBLOCK_SIMPLE
    bool "Always load fallback"
@@ -261,6 +260,7 @@

config BOOTBLOCK_SOURCE
    string
+	depends on ROMCC_BOOTBLOCK
    default "bootblock_simple.c" if BOOTBLOCK_SIMPLE
    default "bootblock_normal.c" if BOOTBLOCK_NORMAL

diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 5787496..36fe1ce 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -53,6 +53,39 @@
    return 0;
 }

+#include <pc80/mc146818rtc.h>
+#include <string.h>
+
+static const char *get_fallback(const char *stagelist)
+{
+	while (*stagelist)
+		stagelist++;
+	return ++stagelist;
+}
+
+static int legacy_romstage_selector(struct prog *romstage)
+{
+	static const char *default_filenames = "normal/romstage\0fallback/romstage";
+	const char *boot_candidate;
+	size_t stages_len;
+
+	if (!(CONFIG(BOOTBLOCK_NORMAL)))
+		return prog_locate(romstage);
+
+	boot_candidate = cbfs_boot_map_with_leak("coreboot-stages", CBFS_TYPE_RAW, &stages_len);
+	if (!boot_candidate)
+		boot_candidate = default_filenames;
+
+	if (do_normal_boot()) {
+		romstage->name = boot_candidate;
+		if (!prog_locate(romstage))
+			return 0;
+	}
+
+	romstage->name = get_fallback(boot_candidate);
+	return prog_locate(romstage);
+}
+
 void run_romstage(void)
 {
    struct prog romstage =
@@ -60,7 +93,7 @@

vboot_run_logic();

-	if (prog_locate(&romstage))
+	if (legacy_romstage_selector(&romstage))
    	goto fail;

timestamp_add_now(TS_START_COPYROM);

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

Hello build bot (Jenkins),

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

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

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

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

bootblock: Support normal/fallback mechanism again

Change-Id: I7395e62f6682f4ef123da10ac125127a57711ec6
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/Kconfig
M src/arch/x86/Makefile.inc
A src/arch/x86/bootblock_normal.c
M src/include/program_loading.h
M src/lib/prog_loaders.c
5 files changed, 57 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/37760/2

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
PS2, Line 15: pc80/mc146818rtc.h
why please ?

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

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

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
PS2, Line 15: pc80/mc146818rtc.h
> why please ?

for do_normal_boot()

And yes, there is more work to be done here, see <fallback.h>.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 2:

(1 comment)

Thx

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/2/src/arch/x86/bootblock_norm... 
PS2, Line 15: pc80/mc146818rtc.h
> for do_normal_boot() […]

Done

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 2: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

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

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
PS5, Line 33: 	boot_candidate = cbfs_boot_map_with_leak("coreboot-stages", CBFS_TYPE_RAW, &stages_len);
I wonder how much this was ever used. Should we keep this or only allow
the simple normal/fallback?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc 
File src/arch/x86/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc@1... 
PS5, Line 115: bootblock-$(CONFIG_BOOTBLOCK_NORMAL) += bootblock_normal.c
What is the difference between 'normal' and 'simple'? It seems to me this new file should be named simple since the Kconfig for normal indicates a cmos flag. Did the names get swapped or is the Kconfig commentary incorrect?

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

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

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc 
File src/arch/x86/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc@1... 
PS5, Line 115: bootblock-$(CONFIG_BOOTBLOCK_NORMAL) += bootblock_normal.c
> What is the difference between 'normal' and 'simple'? It seems to me this new file should be named s […]

The simple one always boots fallback. This is how they were named with romcc bootblock too.

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
PS5, Line 33: 	boot_candidate = cbfs_boot_map_with_leak("coreboot-stages", CBFS_TYPE_RAW, &stages_len);
> I wonder how much this was ever used. Should we keep this or only allow […]

I'd be willing to drop this.

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
PS5, Line 37: 	if (do_normal_boot()) {
This is where cmos flag comes into play.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc 
File src/arch/x86/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/Makefile.inc@1... 
PS5, Line 115: bootblock-$(CONFIG_BOOTBLOCK_NORMAL) += bootblock_normal.c
> The simple one always boots fallback. This is how they were named with romcc bootblock too.

Thanks. I still find it confusing in the naming, but it's consistent. :)

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
File src/arch/x86/bootblock_normal.c:

https://review.coreboot.org/c/coreboot/+/37760/5/src/arch/x86/bootblock_norm... 
PS5, Line 37: 	if (do_normal_boot()) {
> This is where cmos flag comes into play.

ok. Thanks. I must be confused.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37760 )

Change subject: bootblock: Support normal/fallback mechanism again
......................................................................

bootblock: Support normal/fallback mechanism again

Change-Id: I7395e62f6682f4ef123da10ac125127a57711ec6
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37760
Reviewed-by: Nico Huber 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M src/arch/x86/Kconfig
M src/arch/x86/Makefile.inc
A src/arch/x86/bootblock_normal.c
M src/include/program_loading.h
M src/lib/prog_loaders.c
5 files changed, 57 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve
  HAOUAS Elyes: Looks good to me, but someone else must approve

diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig
index f4c0dc9..212c9f9 100644
--- a/src/arch/x86/Kconfig
+++ b/src/arch/x86/Kconfig
@@ -232,11 +232,10 @@
      Add a spin (JMP .) in assembly_entry.S during early romstage to wait
      for a JTAG debugger to break into the execution sequence.

-# Selecting a cbfs prefix from the bootblock is only implemented with romcc
 choice
    prompt "Bootblock behaviour"
    default BOOTBLOCK_SIMPLE
-	depends on ROMCC_BOOTBLOCK
+	depends on !VBOOT

config BOOTBLOCK_SIMPLE
    bool "Always load fallback"
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index 3b13efc..7e150ff 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -112,7 +112,7 @@
 bootblock-$(CONFIG_IDT_IN_EVERY_STAGE) += idt.S
 bootblock-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c
 bootblock-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c
-
+bootblock-$(CONFIG_BOOTBLOCK_NORMAL) += bootblock_normal.c
 bootblock-y += id.S
 $(call src-to-obj,bootblock,$(dir)/id.S): $(obj)/build.h

diff --git a/src/arch/x86/bootblock_normal.c b/src/arch/x86/bootblock_normal.c
new file mode 100644
index 0000000..8001ed0
--- /dev/null
+++ b/src/arch/x86/bootblock_normal.c
@@ -0,0 +1,45 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <cbfs.h>
+#include <pc80/mc146818rtc.h>
+#include <program_loading.h>
+#include <stddef.h>
+#include <string.h>
+
+static const char *get_fallback(const char *stagelist)
+{
+	while (*stagelist)
+		stagelist++;
+	return ++stagelist;
+}
+
+int legacy_romstage_selector(struct prog *romstage)
+{
+	static const char *default_filenames = "normal/romstage\0fallback/romstage";
+	const char *boot_candidate;
+	size_t stages_len;
+
+	boot_candidate = cbfs_boot_map_with_leak("coreboot-stages", CBFS_TYPE_RAW, &stages_len);
+	if (!boot_candidate)
+		boot_candidate = default_filenames;
+
+	if (do_normal_boot()) {
+		romstage->name = boot_candidate;
+		if (!prog_locate(romstage))
+			return 0;
+	}
+
+	romstage->name = get_fallback(boot_candidate);
+	return prog_locate(romstage);
+}
diff --git a/src/include/program_loading.h b/src/include/program_loading.h
index 1b71fad..320ff3c 100644
--- a/src/include/program_loading.h
+++ b/src/include/program_loading.h
@@ -179,6 +179,9 @@
 /* Run romstage from bootblock. */
 void run_romstage(void);

+/* Runtime selector for CBFS_PREFIX of romstage. */
+int legacy_romstage_selector(struct prog *romstage);
+
 /************************
  *   RAMSTAGE LOADING   *
  ************************/
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 978ec16..0319325 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -60,8 +60,13 @@

vboot_run_logic();

-	if (prog_locate(&romstage))
-		goto fail;
+	if (CONFIG(ARCH_X86) && CONFIG(BOOTBLOCK_NORMAL)) {
+		if (legacy_romstage_selector(&romstage))
+			goto fail;
+	} else {
+		if (prog_locate(&romstage))
+			goto fail;
+	}

timestamp_add_now(TS_START_COPYROM);