Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40582
to review the following change.
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE
When CONFIG_SEPARATE_VERSTAGE=n, all verstage code gets linked into the appropriate calling stage (bootblock or romstage). This means that ENV_VERSTAGE is actually 0, and instead ENV_BOOTBLOCK or ENV_ROMSTAGE are 1. This keeps tripping up people who are just trying to write a simple "are we in verstage (i.e. wherever the vboot init logic runs)" check, e.g. for TPM init functions which may run in "verstage" or ramstage depending on whether vboot is enabled. Those checks will not work as intended for CONFIG_SEPARATE_VERSTAGE=n.
This patch renames ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE to try to clarify that this macro can really only be used to check whether code is running in a *separate* verstage, and clue people in that they may need to cover the linked-in verstage case as well.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I2ff3a3c3513b3db44b3cff3d93398330cd3632ea --- M src/arch/x86/assembly_entry.S M src/arch/x86/memlayout.ld M src/commonlib/storage/sdhci.c M src/drivers/i2c/tpm/cr50.c M src/drivers/spi/tpm/tpm.c M src/drivers/usb/ehci_debug.c M src/include/cbmem.h M src/include/console/cbmem_console.h M src/include/console/console.h M src/include/console/uart.h M src/include/console/usb.h M src/include/memlayout.h M src/include/rules.h M src/lib/cbfs.c M src/mainboard/google/deltaur/chromeos.c M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/misc.h M src/soc/mediatek/mt8173/flash_controller.c 19 files changed, 43 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/40582/1
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index d48d28f..f36e7da 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -28,7 +28,7 @@ shrl $2, %ecx rep stosl
-#if ((ENV_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ +#if ((ENV_SEPARATE_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ || (ENV_ROMSTAGE && CONFIG(ROMSTAGE_DEBUG_SPINLOOP)))
/* Wait for a JTAG debugger to break in and set EBX non-zero */ diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 05efac6..5e1ef24 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -32,7 +32,7 @@ ROMSTAGE(CONFIG_ROMSTAGE_ADDR, 1M)
#include EARLY_MEMLAYOUT -#elif ENV_VERSTAGE +#elif ENV_SEPARATE_VERSTAGE /* The 1M size is not allocated. It's just for basic size checking. * Link at 32MiB address and rely on cbfstool to relocate to XIP. */ VERSTAGE(CONFIG_VERSTAGE_ADDR, 1M) diff --git a/src/commonlib/storage/sdhci.c b/src/commonlib/storage/sdhci.c index fd9fc63..246b9b9 100644 --- a/src/commonlib/storage/sdhci.c +++ b/src/commonlib/storage/sdhci.c @@ -27,7 +27,7 @@ #include <commonlib/stdlib.h>
#define DMA_AVAILABLE ((CONFIG(SDHCI_ADMA_IN_BOOTBLOCK) && ENV_BOOTBLOCK) \ - || (CONFIG(SDHCI_ADMA_IN_VERSTAGE) && ENV_VERSTAGE) \ + || (CONFIG(SDHCI_ADMA_IN_VERSTAGE) && ENV_SEPARATE_VERSTAGE) \ || (CONFIG(SDHCI_ADMA_IN_ROMSTAGE) && ENV_ROMSTAGE) \ || ENV_POSTCAR || ENV_RAMSTAGE)
diff --git a/src/drivers/i2c/tpm/cr50.c b/src/drivers/i2c/tpm/cr50.c index 34873dc..3d9ca59 100644 --- a/src/drivers/i2c/tpm/cr50.c +++ b/src/drivers/i2c/tpm/cr50.c @@ -502,7 +502,7 @@ if (cr50_i2c_probe(chip, &did_vid)) return -1;
- if (ENV_VERSTAGE || ENV_BOOTBLOCK) + if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) if (process_reset(chip)) return -1;
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 62d1bba..8f93e2a 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -454,7 +454,7 @@
printk(BIOS_INFO, " done!\n");
- if (ENV_VERSTAGE || ENV_BOOTBLOCK) + if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) /* * Claim locality 0, do it only during the first * initialization after reset. diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 094cd56..c26d1db 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -55,7 +55,7 @@ { if (glob_dbg_info_p == NULL) { struct ehci_debug_info *info; - if (ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE) { + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE || ENV_ROMSTAGE) { /* The message likely does not show if we hit this. */ if (sizeof(*info) > _car_ehci_dbg_info_size) die("BUG: Increase ehci_dbg_info reserve in CAR"); diff --git a/src/include/cbmem.h b/src/include/cbmem.h index 77fff07..dcffbfe 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -161,7 +161,7 @@ if (ENV_BOOTBLOCK) return 0;
- if (ENV_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) + if (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) return 0;
return 1; diff --git a/src/include/console/cbmem_console.h b/src/include/console/cbmem_console.h index a291db8..2996f7c 100644 --- a/src/include/console/cbmem_console.h +++ b/src/include/console/cbmem_console.h @@ -9,8 +9,8 @@ void cbmemc_tx_byte(unsigned char data);
#define __CBMEM_CONSOLE_ENABLE__ (CONFIG(CONSOLE_CBMEM) && \ - (ENV_RAMSTAGE || ENV_VERSTAGE || ENV_POSTCAR || ENV_ROMSTAGE || \ - (ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE)))) + (ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE || ENV_POSTCAR || \ + ENV_ROMSTAGE || (ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE))))
#if __CBMEM_CONSOLE_ENABLE__ static inline void __cbmemc_init(void) { cbmemc_init(); } diff --git a/src/include/console/console.h b/src/include/console/console.h index fdc48da..be06c66 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -31,9 +31,9 @@
#define __CONSOLE_ENABLE__ \ ((ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE)) || \ - (ENV_POSTCAR && CONFIG(POSTCAR_CONSOLE)) || \ - ENV_VERSTAGE || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_LIBAGESA || \ - (ENV_SMM && CONFIG(DEBUG_SMI))) + (ENV_POSTCAR && CONFIG(POSTCAR_CONSOLE)) || \ + ENV_SEPARATE_VERSTAGE || ENV_ROMSTAGE || ENV_RAMSTAGE || \ + ENV_LIBAGESA || (ENV_SMM && CONFIG(DEBUG_SMI)))
#if __CONSOLE_ENABLE__ asmlinkage void console_init(void); diff --git a/src/include/console/uart.h b/src/include/console/uart.h index 1bd6ef0..4385237 100644 --- a/src/include/console/uart.h +++ b/src/include/console/uart.h @@ -51,8 +51,8 @@ void oxford_remap(unsigned int new_base);
#define __CONSOLE_SERIAL_ENABLE__ (CONFIG(CONSOLE_SERIAL) && \ - (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_VERSTAGE || \ - ENV_POSTCAR || (ENV_SMM && CONFIG(DEBUG_SMI)))) + (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE + || ENV_POSTCAR || (ENV_SMM && CONFIG(DEBUG_SMI))))
#if __CONSOLE_SERIAL_ENABLE__ static inline void __uart_init(void) diff --git a/src/include/console/usb.h b/src/include/console/usb.h index b7bc7f4..e67f125 100644 --- a/src/include/console/usb.h +++ b/src/include/console/usb.h @@ -18,7 +18,7 @@ ((ENV_BOOTBLOCK && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ (ENV_ROMSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ (ENV_POSTCAR && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ - (ENV_VERSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ + (ENV_SEPARATE_VERSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ ENV_RAMSTAGE))
#define USB_PIPE_FOR_CONSOLE 0 diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 0cd465b..bef3637 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -152,7 +152,7 @@ REGION(tpm_tcpa_log, addr, size, 16) \ _ = ASSERT(size >= 2K, "tpm tcpa log buffer must be at least 2K!");
-#if ENV_VERSTAGE +#if ENV_SEPARATE_VERSTAGE #define VERSTAGE(addr, sz) \ SYMBOL(verstage, addr) \ _everstage = _verstage + sz; \ diff --git a/src/include/rules.h b/src/include/rules.h index 92603db..612f131 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -14,7 +14,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -26,7 +26,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -38,7 +38,7 @@ #define ENV_ROMSTAGE 1 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -50,19 +50,26 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 1 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 #define ENV_STRING "smm"
+/* + * NOTE: "verstage" code may either run as a separate stage or linked into the + * bootblock/romstage, depending on the setting of CONFIG_SEPARATE_VERSTAGE. The + * ENV_SEPARATE_VERSTAGE macro will only return true for "verstage" code when + * CONFIG_SEPARATE_VERSTAGE=y, otherwise that code will have ENV_BOOTBLOCK or + * ENV_ROMSTAGE set (depending on the CONFIG_VBOOT_STARTS_IN_... options). + */ #elif defined(__VERSTAGE__) #define ENV_DECOMPRESSOR 0 #define ENV_BOOTBLOCK 0 #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 1 +#define ENV_SEPARATE_VERSTAGE 1 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -74,7 +81,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 1 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -86,7 +93,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 1 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -98,7 +105,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 1 #define ENV_LIBAGESA 0 @@ -110,7 +117,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 1 @@ -126,7 +133,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -239,7 +246,7 @@
#define ENV_ROMSTAGE_OR_BEFORE \ (ENV_DECOMPRESSOR || ENV_BOOTBLOCK || ENV_ROMSTAGE || \ - (ENV_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))) + (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)))
#if CONFIG(ARCH_X86) /* Indicates memory layout is determined with arch/x86/car.ld. */ diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 5e85bb9..a61bc63 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -106,7 +106,7 @@ return in_size;
case CBFS_COMPRESS_LZ4: - if ((ENV_BOOTBLOCK || ENV_VERSTAGE) && + if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) && !CONFIG(COMPRESS_PRERAM_STAGES)) return 0;
@@ -125,7 +125,7 @@
case CBFS_COMPRESS_LZMA: /* We assume here romstage and postcar are never compressed. */ - if (ENV_BOOTBLOCK || ENV_VERSTAGE) + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) return 0; if (ENV_ROMSTAGE && CONFIG(POSTCAR_STAGE)) return 0; @@ -236,8 +236,8 @@
/* Hacky way to not load programs over read only media. The stages * that would hit this path initialize themselves. */ - if ((ENV_BOOTBLOCK || ENV_VERSTAGE) && !CONFIG(NO_XIP_EARLY_STAGES) && - CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) { + if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) && + !CONFIG(NO_XIP_EARLY_STAGES) && CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) { void *mapping = rdev_mmap(fh, foffset, fsize); rdev_munmap(fh, mapping); if (mapping == load) diff --git a/src/mainboard/google/deltaur/chromeos.c b/src/mainboard/google/deltaur/chromeos.c index 5a0c481..2665f46 100644 --- a/src/mainboard/google/deltaur/chromeos.c +++ b/src/mainboard/google/deltaur/chromeos.c @@ -89,7 +89,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/mainboard/google/drallion/chromeos.c b/src/mainboard/google/drallion/chromeos.c index ee3509d..eff43d0 100644 --- a/src/mainboard/google/drallion/chromeos.c +++ b/src/mainboard/google/drallion/chromeos.c @@ -84,7 +84,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index e64bb73..cd59fa9 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -82,7 +82,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index fd422b2..d1e60bb 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -47,7 +47,7 @@ static inline int verification_should_run(void) { if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_VERSTAGE; + return ENV_SEPARATE_VERSTAGE; else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) return ENV_ROMSTAGE; else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) diff --git a/src/soc/mediatek/mt8173/flash_controller.c b/src/soc/mediatek/mt8173/flash_controller.c index 9aa1a67..6b222c9 100644 --- a/src/soc/mediatek/mt8173/flash_controller.c +++ b/src/soc/mediatek/mt8173/flash_controller.c @@ -157,7 +157,7 @@ done += next; }
- if (ENV_BOOTBLOCK || ENV_VERSTAGE) { + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) { dma_buf = (uintptr_t)_dma_coherent; dma_buf_len = REGION_SIZE(dma_coherent); } else {
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40582 )
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40582/1/src/include/console/uart.h File src/include/console/uart.h:
https://review.coreboot.org/c/coreboot/+/40582/1/src/include/console/uart.h@... PS1, Line 54: (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE missing continuation \
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40582 )
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40582/1/src/include/console/uart.h File src/include/console/uart.h:
https://review.coreboot.org/c/coreboot/+/40582/1/src/include/console/uart.h@... PS1, Line 54: (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE
missing continuation \
Whoops, done. (I don't know why I never get emails when Jenkins sets a -1 anymore, makes it really hard to keep track of things...)
Hello build bot (Jenkins), Christian Walter, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40582
to look at the new patch set (#2).
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE
When CONFIG_SEPARATE_VERSTAGE=n, all verstage code gets linked into the appropriate calling stage (bootblock or romstage). This means that ENV_VERSTAGE is actually 0, and instead ENV_BOOTBLOCK or ENV_ROMSTAGE are 1. This keeps tripping up people who are just trying to write a simple "are we in verstage (i.e. wherever the vboot init logic runs)" check, e.g. for TPM init functions which may run in "verstage" or ramstage depending on whether vboot is enabled. Those checks will not work as intended for CONFIG_SEPARATE_VERSTAGE=n.
This patch renames ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE to try to clarify that this macro can really only be used to check whether code is running in a *separate* verstage, and clue people in that they may need to cover the linked-in verstage case as well.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I2ff3a3c3513b3db44b3cff3d93398330cd3632ea --- M src/arch/x86/assembly_entry.S M src/arch/x86/memlayout.ld M src/commonlib/storage/sdhci.c M src/drivers/i2c/tpm/cr50.c M src/drivers/spi/tpm/tpm.c M src/drivers/usb/ehci_debug.c M src/include/cbmem.h M src/include/console/cbmem_console.h M src/include/console/console.h M src/include/console/uart.h M src/include/console/usb.h M src/include/memlayout.h M src/include/rules.h M src/lib/cbfs.c M src/mainboard/google/deltaur/chromeos.c M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/misc.h M src/soc/mediatek/mt8173/flash_controller.c 19 files changed, 43 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/40582/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40582 )
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
Patch Set 2: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40582 )
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE
When CONFIG_SEPARATE_VERSTAGE=n, all verstage code gets linked into the appropriate calling stage (bootblock or romstage). This means that ENV_VERSTAGE is actually 0, and instead ENV_BOOTBLOCK or ENV_ROMSTAGE are 1. This keeps tripping up people who are just trying to write a simple "are we in verstage (i.e. wherever the vboot init logic runs)" check, e.g. for TPM init functions which may run in "verstage" or ramstage depending on whether vboot is enabled. Those checks will not work as intended for CONFIG_SEPARATE_VERSTAGE=n.
This patch renames ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE to try to clarify that this macro can really only be used to check whether code is running in a *separate* verstage, and clue people in that they may need to cover the linked-in verstage case as well.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I2ff3a3c3513b3db44b3cff3d93398330cd3632ea Reviewed-on: https://review.coreboot.org/c/coreboot/+/40582 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/arch/x86/assembly_entry.S M src/arch/x86/memlayout.ld M src/commonlib/storage/sdhci.c M src/drivers/i2c/tpm/cr50.c M src/drivers/spi/tpm/tpm.c M src/drivers/usb/ehci_debug.c M src/include/cbmem.h M src/include/console/cbmem_console.h M src/include/console/console.h M src/include/console/uart.h M src/include/console/usb.h M src/include/memlayout.h M src/include/rules.h M src/lib/cbfs.c M src/mainboard/google/deltaur/chromeos.c M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/misc.h M src/soc/mediatek/mt8173/flash_controller.c 19 files changed, 43 insertions(+), 36 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/assembly_entry.S b/src/arch/x86/assembly_entry.S index d48d28f..f36e7da 100644 --- a/src/arch/x86/assembly_entry.S +++ b/src/arch/x86/assembly_entry.S @@ -28,7 +28,7 @@ shrl $2, %ecx rep stosl
-#if ((ENV_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ +#if ((ENV_SEPARATE_VERSTAGE && CONFIG(VERSTAGE_DEBUG_SPINLOOP)) \ || (ENV_ROMSTAGE && CONFIG(ROMSTAGE_DEBUG_SPINLOOP)))
/* Wait for a JTAG debugger to break in and set EBX non-zero */ diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 05efac6..5e1ef24 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -32,7 +32,7 @@ ROMSTAGE(CONFIG_ROMSTAGE_ADDR, 1M)
#include EARLY_MEMLAYOUT -#elif ENV_VERSTAGE +#elif ENV_SEPARATE_VERSTAGE /* The 1M size is not allocated. It's just for basic size checking. * Link at 32MiB address and rely on cbfstool to relocate to XIP. */ VERSTAGE(CONFIG_VERSTAGE_ADDR, 1M) diff --git a/src/commonlib/storage/sdhci.c b/src/commonlib/storage/sdhci.c index fd9fc63..246b9b9 100644 --- a/src/commonlib/storage/sdhci.c +++ b/src/commonlib/storage/sdhci.c @@ -27,7 +27,7 @@ #include <commonlib/stdlib.h>
#define DMA_AVAILABLE ((CONFIG(SDHCI_ADMA_IN_BOOTBLOCK) && ENV_BOOTBLOCK) \ - || (CONFIG(SDHCI_ADMA_IN_VERSTAGE) && ENV_VERSTAGE) \ + || (CONFIG(SDHCI_ADMA_IN_VERSTAGE) && ENV_SEPARATE_VERSTAGE) \ || (CONFIG(SDHCI_ADMA_IN_ROMSTAGE) && ENV_ROMSTAGE) \ || ENV_POSTCAR || ENV_RAMSTAGE)
diff --git a/src/drivers/i2c/tpm/cr50.c b/src/drivers/i2c/tpm/cr50.c index 34873dc..3d9ca59 100644 --- a/src/drivers/i2c/tpm/cr50.c +++ b/src/drivers/i2c/tpm/cr50.c @@ -502,7 +502,7 @@ if (cr50_i2c_probe(chip, &did_vid)) return -1;
- if (ENV_VERSTAGE || ENV_BOOTBLOCK) + if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) if (process_reset(chip)) return -1;
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 62d1bba..8f93e2a 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -454,7 +454,7 @@
printk(BIOS_INFO, " done!\n");
- if (ENV_VERSTAGE || ENV_BOOTBLOCK) + if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) /* * Claim locality 0, do it only during the first * initialization after reset. diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 094cd56..c26d1db 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -55,7 +55,7 @@ { if (glob_dbg_info_p == NULL) { struct ehci_debug_info *info; - if (ENV_BOOTBLOCK || ENV_VERSTAGE || ENV_ROMSTAGE) { + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE || ENV_ROMSTAGE) { /* The message likely does not show if we hit this. */ if (sizeof(*info) > _car_ehci_dbg_info_size) die("BUG: Increase ehci_dbg_info reserve in CAR"); diff --git a/src/include/cbmem.h b/src/include/cbmem.h index 77fff07..dcffbfe 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -161,7 +161,7 @@ if (ENV_BOOTBLOCK) return 0;
- if (ENV_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) + if (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) return 0;
return 1; diff --git a/src/include/console/cbmem_console.h b/src/include/console/cbmem_console.h index a291db8..2996f7c 100644 --- a/src/include/console/cbmem_console.h +++ b/src/include/console/cbmem_console.h @@ -9,8 +9,8 @@ void cbmemc_tx_byte(unsigned char data);
#define __CBMEM_CONSOLE_ENABLE__ (CONFIG(CONSOLE_CBMEM) && \ - (ENV_RAMSTAGE || ENV_VERSTAGE || ENV_POSTCAR || ENV_ROMSTAGE || \ - (ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE)))) + (ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE || ENV_POSTCAR || \ + ENV_ROMSTAGE || (ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE))))
#if __CBMEM_CONSOLE_ENABLE__ static inline void __cbmemc_init(void) { cbmemc_init(); } diff --git a/src/include/console/console.h b/src/include/console/console.h index fdc48da..be06c66 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -31,9 +31,9 @@
#define __CONSOLE_ENABLE__ \ ((ENV_BOOTBLOCK && CONFIG(BOOTBLOCK_CONSOLE)) || \ - (ENV_POSTCAR && CONFIG(POSTCAR_CONSOLE)) || \ - ENV_VERSTAGE || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_LIBAGESA || \ - (ENV_SMM && CONFIG(DEBUG_SMI))) + (ENV_POSTCAR && CONFIG(POSTCAR_CONSOLE)) || \ + ENV_SEPARATE_VERSTAGE || ENV_ROMSTAGE || ENV_RAMSTAGE || \ + ENV_LIBAGESA || (ENV_SMM && CONFIG(DEBUG_SMI)))
#if __CONSOLE_ENABLE__ asmlinkage void console_init(void); diff --git a/src/include/console/uart.h b/src/include/console/uart.h index 1bd6ef0..d423d9d 100644 --- a/src/include/console/uart.h +++ b/src/include/console/uart.h @@ -51,8 +51,8 @@ void oxford_remap(unsigned int new_base);
#define __CONSOLE_SERIAL_ENABLE__ (CONFIG(CONSOLE_SERIAL) && \ - (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_VERSTAGE || \ - ENV_POSTCAR || (ENV_SMM && CONFIG(DEBUG_SMI)))) + (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_SEPARATE_VERSTAGE \ + || ENV_POSTCAR || (ENV_SMM && CONFIG(DEBUG_SMI))))
#if __CONSOLE_SERIAL_ENABLE__ static inline void __uart_init(void) diff --git a/src/include/console/usb.h b/src/include/console/usb.h index b7bc7f4..e67f125 100644 --- a/src/include/console/usb.h +++ b/src/include/console/usb.h @@ -18,7 +18,7 @@ ((ENV_BOOTBLOCK && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ (ENV_ROMSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ (ENV_POSTCAR && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ - (ENV_VERSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ + (ENV_SEPARATE_VERSTAGE && CONFIG(USBDEBUG_IN_PRE_RAM)) || \ ENV_RAMSTAGE))
#define USB_PIPE_FOR_CONSOLE 0 diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 0cd465b..bef3637 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -152,7 +152,7 @@ REGION(tpm_tcpa_log, addr, size, 16) \ _ = ASSERT(size >= 2K, "tpm tcpa log buffer must be at least 2K!");
-#if ENV_VERSTAGE +#if ENV_SEPARATE_VERSTAGE #define VERSTAGE(addr, sz) \ SYMBOL(verstage, addr) \ _everstage = _verstage + sz; \ diff --git a/src/include/rules.h b/src/include/rules.h index 92603db..612f131 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -14,7 +14,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -26,7 +26,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -38,7 +38,7 @@ #define ENV_ROMSTAGE 1 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -50,19 +50,26 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 1 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 #define ENV_STRING "smm"
+/* + * NOTE: "verstage" code may either run as a separate stage or linked into the + * bootblock/romstage, depending on the setting of CONFIG_SEPARATE_VERSTAGE. The + * ENV_SEPARATE_VERSTAGE macro will only return true for "verstage" code when + * CONFIG_SEPARATE_VERSTAGE=y, otherwise that code will have ENV_BOOTBLOCK or + * ENV_ROMSTAGE set (depending on the CONFIG_VBOOT_STARTS_IN_... options). + */ #elif defined(__VERSTAGE__) #define ENV_DECOMPRESSOR 0 #define ENV_BOOTBLOCK 0 #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 1 +#define ENV_SEPARATE_VERSTAGE 1 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -74,7 +81,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 1 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -86,7 +93,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 1 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -98,7 +105,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 1 #define ENV_LIBAGESA 0 @@ -110,7 +117,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 1 @@ -126,7 +133,7 @@ #define ENV_ROMSTAGE 0 #define ENV_RAMSTAGE 0 #define ENV_SMM 0 -#define ENV_VERSTAGE 0 +#define ENV_SEPARATE_VERSTAGE 0 #define ENV_RMODULE 0 #define ENV_POSTCAR 0 #define ENV_LIBAGESA 0 @@ -239,7 +246,7 @@
#define ENV_ROMSTAGE_OR_BEFORE \ (ENV_DECOMPRESSOR || ENV_BOOTBLOCK || ENV_ROMSTAGE || \ - (ENV_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))) + (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)))
#if CONFIG(ARCH_X86) /* Indicates memory layout is determined with arch/x86/car.ld. */ diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 5e85bb9..a61bc63 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -106,7 +106,7 @@ return in_size;
case CBFS_COMPRESS_LZ4: - if ((ENV_BOOTBLOCK || ENV_VERSTAGE) && + if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) && !CONFIG(COMPRESS_PRERAM_STAGES)) return 0;
@@ -125,7 +125,7 @@
case CBFS_COMPRESS_LZMA: /* We assume here romstage and postcar are never compressed. */ - if (ENV_BOOTBLOCK || ENV_VERSTAGE) + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) return 0; if (ENV_ROMSTAGE && CONFIG(POSTCAR_STAGE)) return 0; @@ -236,8 +236,8 @@
/* Hacky way to not load programs over read only media. The stages * that would hit this path initialize themselves. */ - if ((ENV_BOOTBLOCK || ENV_VERSTAGE) && !CONFIG(NO_XIP_EARLY_STAGES) && - CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) { + if ((ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) && + !CONFIG(NO_XIP_EARLY_STAGES) && CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) { void *mapping = rdev_mmap(fh, foffset, fsize); rdev_munmap(fh, mapping); if (mapping == load) diff --git a/src/mainboard/google/deltaur/chromeos.c b/src/mainboard/google/deltaur/chromeos.c index 5a0c481..2665f46 100644 --- a/src/mainboard/google/deltaur/chromeos.c +++ b/src/mainboard/google/deltaur/chromeos.c @@ -89,7 +89,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/mainboard/google/drallion/chromeos.c b/src/mainboard/google/drallion/chromeos.c index ee3509d..eff43d0 100644 --- a/src/mainboard/google/drallion/chromeos.c +++ b/src/mainboard/google/drallion/chromeos.c @@ -84,7 +84,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index e64bb73..cd59fa9 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -82,7 +82,7 @@ * The TPM recovery request is passed between stages through vboot data * or cbmem depending on stage. */ - if (ENV_VERSTAGE && + if (ENV_SEPARATE_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && cr50_state) state = REC_MODE_REQUESTED; diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index fd422b2..d1e60bb 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -47,7 +47,7 @@ static inline int verification_should_run(void) { if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_VERSTAGE; + return ENV_SEPARATE_VERSTAGE; else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) return ENV_ROMSTAGE; else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) diff --git a/src/soc/mediatek/mt8173/flash_controller.c b/src/soc/mediatek/mt8173/flash_controller.c index 9aa1a67..6b222c9 100644 --- a/src/soc/mediatek/mt8173/flash_controller.c +++ b/src/soc/mediatek/mt8173/flash_controller.c @@ -157,7 +157,7 @@ done += next; }
- if (ENV_BOOTBLOCK || ENV_VERSTAGE) { + if (ENV_BOOTBLOCK || ENV_SEPARATE_VERSTAGE) { dma_buf = (uintptr_t)_dma_coherent; dma_buf_len = REGION_SIZE(dma_coherent); } else {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40582 )
Change subject: rules.h: Rename ENV_VERSTAGE to ENV_SEPARATE_VERSTAGE ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2547 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2546 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2545 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2544
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Julius Werner (Code Review)