Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
coreboot-id: Add coreboot id based on FMAP entries
This change replaces the coreboot id added to bootblock/decompressor stages with a ID region in FMAP that can be used across platforms.
Following changes are made as part of this: 1. Adds new Kconfig options for COREBOOT_ID_FMAP_NAME and COREBOOT_ID_SIZE which provide the details of the fmap region name used for storing coreboot ID and the size of this region. Chrome OS does not use any of the coreboot build details filled in ID region. So, they are skipped for all Chrome OS boards. 2. Adds ##COREBOOT_ID_ENTRY## to default fmd files that can be used as a marker to insert the fmap entries for coreboot ID. 3. Replaces ##COREBOOT_ID_ENTRY## marker in the default fmd files using Makefile and the newly added FMAP_CURRENT_BASE. 4. Adds coreboot build details (version, vendor, part number) to this newly added FMAP region using cbfstool add-id command. 5. Removes id.ld and id.S from all architectures using it. 6. Adds COREBOOT_ID region to all non-Chrome OS fmd files.
Because of this change, following difference would be observed: 1. Non-Chrome OS boards: coreboot build details will move from near the reset vector in bootblock/decompressor to a separate FMAP region. coreboot does not rely on these details at runtime. So, there should not be any impact. I am unsure if any tools used this at runtime. If yes, then those tools will have to be updated to use FMAP instead to find the location of ID.
2. Chrome OS boards: No coreboot details added to ROM image.
TEST=Verified that build details are correctly added to the ID region in FMAP.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ibe1aaf36bec00b4cffc46a3640a5a0d1e4bb8067 --- M Documentation/Intel/SoC/soc.html M Makefile.inc M src/Kconfig M src/arch/arm/Makefile.inc D src/arch/arm/id.S M src/arch/arm64/Makefile.inc D src/arch/arm64/id.S M src/arch/x86/Makefile.inc D src/arch/x86/id.S D src/arch/x86/id.ld M src/arch/x86/memlayout.ld M src/mainboard/51nb/x210/board.fmd M src/mainboard/cavium/cn8100_sff_evb/board.fmd M src/mainboard/emulation/qemu-i440fx/vboot-rwa-16M.fmd M src/mainboard/emulation/qemu-q35/vboot-rwa-16M.fmd M src/mainboard/emulation/qemu-q35/vboot-rwab-16M.fmd M src/mainboard/facebook/monolith/vboot-ro.fmd M src/mainboard/facebook/monolith/vboot-rw.fmd M src/mainboard/intel/cedarisland_crb/board.fmd M src/mainboard/intel/galileo/vboot.fmd M src/mainboard/intel/leafhill/leafhill.16384.fmd M src/mainboard/intel/leafhill/leafhill.8192.fmd M src/mainboard/intel/minnow3/minnow3.fmd M src/mainboard/lenovo/t400/vboot-rwa.fmd M src/mainboard/lenovo/t410/vboot-rwa.fmd M src/mainboard/lenovo/t420/vboot-rwa.fmd M src/mainboard/lenovo/t420s/vboot-rwa.fmd M src/mainboard/lenovo/t430/vboot-rwab.fmd M src/mainboard/lenovo/t430s/vboot-rwab.fmd M src/mainboard/lenovo/t440p/vboot-rwab.fmd M src/mainboard/lenovo/t520/vboot-rwa.fmd M src/mainboard/lenovo/t530/vboot-rwab.fmd M src/mainboard/lenovo/x131e/vboot-rwab.fmd M src/mainboard/lenovo/x1_carbon_gen1/vboot-rwab.fmd M src/mainboard/lenovo/x200/vboot-rwa.fmd M src/mainboard/lenovo/x201/vboot-rwa.fmd M src/mainboard/lenovo/x220/vboot-rwa.fmd M src/mainboard/lenovo/x230/vboot-rwab.fmd M src/mainboard/ocp/tiogapass/board.fmd M src/mainboard/opencellular/elgon/board.fmd M src/mainboard/opencellular/elgon/vboot.fmd M src/mainboard/siemens/mc_apl1/mc_apl1.fmd M src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd M src/mainboard/supermicro/x11-lga1151-series/vboot-ro-rwab.fmd M src/mainboard/up/squared/upsquared.fmd M src/mainboard/up/squared/vboot-ro.fmd M src/mainboard/up/squared/vboot-roa.fmd M src/mainboard/up/squared/vboot-roab.fmd M util/cbfstool/default-x86.fmd M util/cbfstool/default.fmd 50 files changed, 113 insertions(+), 133 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/40377/1
diff --git a/Documentation/Intel/SoC/soc.html b/Documentation/Intel/SoC/soc.html index 6b1bb30..9e4d62e 100644 --- a/Documentation/Intel/SoC/soc.html +++ b/Documentation/Intel/SoC/soc.html @@ -199,9 +199,6 @@ </li> </ul> </li> - <li><a target="_blank" href="https://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/arch/x86/id.S">src/arch/x86/id.S</a> - added by <a target="_blank" href="https://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/arch/x86/Makefile.inc;hb=HEAD#l110">src/arch/x86/Makefile.inc</a> - </li> <li><a target="_blank" href="https://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/cpu/intel/fit/fit.S">src/cpu/intel/fit/fit.S</a> added by <a target="_blank" href="https://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/cpu/intel/fit/Makefile.inc;hb=HEAD">src/cpu/intel/fit/Makefile.inc</a> </li> diff --git a/Makefile.inc b/Makefile.inc index ac430f7..19f018c 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -926,6 +926,15 @@ FMAP_SMMSTORE_ENTRY := endif
+ifneq ($(CONFIG_COREBOOT_ID_SIZE),0) +FMAP_COREBOOT_ID_BASE := $(FMAP_CURRENT_BASE) +FMAP_COREBOOT_ID_SIZE := $(CONFIG_COREBOOT_ID_SIZE) +FMAP_COREBOOT_ID_ENTRY := COREBOOT_ID@$(FMAP_COREBOOT_ID_BASE) $(FMAP_COREBOOT_ID_SIZE) +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_COREBOOT_ID_BASE) $(FMAP_COREBOOT_ID_SIZE)) +else +FMAP_COREBOOT_ID_ENTRY := +endif + # # X86 FMAP region # @@ -983,6 +992,15 @@ FMAP_MRC_CACHE_ENTRY := endif
+ifneq ($(CONFIG_COREBOOT_ID_SIZE),0) +FMAP_COREBOOT_ID_BASE := $(FMAP_CURRENT_BASE) +FMAP_COREBOOT_ID_SIZE := $(CONFIG_COREBOOT_ID_SIZE) +FMAP_COREBOOT_ID_ENTRY := COREBOOT_ID@$(FMAP_COREBOOT_ID_BASE) $(FMAP_COREBOOT_ID_SIZE) +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_COREBOOT_ID_BASE) $(FMAP_COREBOOT_ID_SIZE)) +else +FMAP_COREBOOT_ID_ENTRY := +endif + # # NON-X86 COREBOOT default cbfs FMAP region # @@ -1002,6 +1020,7 @@ -e "s,##CONSOLE_ENTRY##,$(FMAP_CONSOLE_ENTRY)," \ -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \ -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \ + -e "s,##COREBOOT_ID_ENTRY##,$(FMAP_COREBOOT_ID_ENTRY)," \ -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \ -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \ $(DEFAULT_FLASHMAP) > $@.tmp @@ -1028,6 +1047,11 @@ ifneq ($(CONFIG_UPDATE_IMAGE),y) $(obj)/coreboot.pre: $(objcbfs)/bootblock.bin $$(prebuilt-files) $(CBFSTOOL) $(IFITTOOL) $$(cpu_ucode_cbfs_file) $(obj)/fmap.fmap $(obj)/fmap.desc $(CBFSTOOL) $@.tmp create -M $(obj)/fmap.fmap -r $(shell cat $(obj)/fmap.desc) +ifneq ($(CONFIG_COREBOOT_ID_FMAP_NAME),"") + $(CBFSTOOL) $@.tmp add-id -r $(CONFIG_COREBOOT_ID_FMAP_NAME) --id-version $(KERNELVERSION) \ + --id-vendor $(CONFIG_MAINBOARD_VENDOR) \ + --id-part-num $(CONFIG_MAINBOARD_PART_NUMBER) +endif ifeq ($(CONFIG_ARCH_X86),y) $(CBFSTOOL) $@.tmp add \ -f $(objcbfs)/bootblock.bin \ diff --git a/src/Kconfig b/src/Kconfig index da21af1..378f302 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -422,6 +422,22 @@ binaries. This symbol should only be used to generate a default FMAP and is unused when a non-default fmd file is provided via CONFIG_FMDFILE.
+config COREBOOT_ID_FMAP_NAME + string "Name of FMAP region storing coreboot build details" + default "" if CHROMEOS + default "COREBOOT_ID" + help + This is the name of the FMAP region where coreboot build details like + version, vendor name and part number are stored by the build system. + +config COREBOOT_ID_SIZE + hex "Size of FMAP region storing coreboot build details" + default 0 if CHROMEOS + default 0x40 + help + This is the size of the FMAP region where coreboot build details like + version, vendor name and part number are stored by the build system. + endmenu
# load site-local kconfig to allow user specific defaults and overrides diff --git a/src/arch/arm/Makefile.inc b/src/arch/arm/Makefile.inc index a8abfaf..998ef8e 100644 --- a/src/arch/arm/Makefile.inc +++ b/src/arch/arm/Makefile.inc @@ -31,11 +31,6 @@
ifeq ($(CONFIG_ARCH_BOOTBLOCK_ARM),y)
-decompressor-y += id.S -bootblock-y += id.S -$(call src-to-obj,decompressor,$(dir)/id.S): $(obj)/build.h -$(call src-to-obj,bootblock,$(dir)/id.S): $(obj)/build.h - decompressor-y += boot.c bootblock-y += boot.c decompressor-y += div0.c diff --git a/src/arch/arm/id.S b/src/arch/arm/id.S deleted file mode 100644 index 3cdd013..0000000 --- a/src/arch/arm/id.S +++ /dev/null @@ -1,23 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* This file is part of the coreboot project. */ - -#include <build.h> - - .section ".id", "a", %progbits - - .globl __id_start -__id_start: -ver: - .asciz COREBOOT_VERSION -vendor: - .asciz CONFIG_MAINBOARD_VENDOR -part: - .asciz CONFIG_MAINBOARD_PART_NUMBER -.long __id_end - ver /* Reverse offset to the vendor id */ -.long __id_end - vendor /* Reverse offset to the vendor id */ -.long __id_end - part /* Reverse offset to the part number */ -.long CONFIG_ROM_SIZE /* Size of this romimage */ - .globl __id_end - -__id_end: -.previous diff --git a/src/arch/arm64/Makefile.inc b/src/arch/arm64/Makefile.inc index c3d1fe5..3e4b28d 100644 --- a/src/arch/arm64/Makefile.inc +++ b/src/arch/arm64/Makefile.inc @@ -31,10 +31,6 @@ bootblock-y += div0.c decompressor-y += eabi_compat.c bootblock-y += eabi_compat.c -decompressor-y += id.S -bootblock-y += id.S -$(call src-to-obj,decompressor,$(dir)/id.S): $(obj)/build.h -$(call src-to-obj,bootblock,$(dir)/id.S): $(obj)/build.h
bootblock-$(CONFIG_ARM64_USE_ARCH_TIMER) += arch_timer.c bootblock-y += transition.c transition_asm.S diff --git a/src/arch/arm64/id.S b/src/arch/arm64/id.S deleted file mode 100644 index 3cdd013..0000000 --- a/src/arch/arm64/id.S +++ /dev/null @@ -1,23 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* This file is part of the coreboot project. */ - -#include <build.h> - - .section ".id", "a", %progbits - - .globl __id_start -__id_start: -ver: - .asciz COREBOOT_VERSION -vendor: - .asciz CONFIG_MAINBOARD_VENDOR -part: - .asciz CONFIG_MAINBOARD_PART_NUMBER -.long __id_end - ver /* Reverse offset to the vendor id */ -.long __id_end - vendor /* Reverse offset to the vendor id */ -.long __id_end - part /* Reverse offset to the part number */ -.long CONFIG_ROM_SIZE /* Size of this romimage */ - .globl __id_end - -__id_end: -.previous diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index b28ef78..d22e203 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -111,8 +111,6 @@ 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
bootblock-y += bootblock_crt0.S
diff --git a/src/arch/x86/id.S b/src/arch/x86/id.S deleted file mode 100644 index dd447e6..0000000 --- a/src/arch/x86/id.S +++ /dev/null @@ -1,29 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* This file is part of the coreboot project. */ - -#include <build.h> - - .section ".id", "a", @progbits - - .globl __id_start -__id_start: -ver: - .asciz COREBOOT_VERSION -vendor: - .asciz CONFIG_MAINBOARD_VENDOR -part: - .asciz CONFIG_MAINBOARD_PART_NUMBER -.long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the - *vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - vendor /* Reverse offset to the - * vendor id - */ -.long __id_end + CONFIG_ID_SECTION_OFFSET - part /* Reverse offset to the - * part number - */ -.long CONFIG_ROM_SIZE /* Size of this romimage */ - .globl __id_end - -__id_end: -.previous diff --git a/src/arch/x86/id.ld b/src/arch/x86/id.ld deleted file mode 100644 index 31d5738..0000000 --- a/src/arch/x86/id.ld +++ /dev/null @@ -1,9 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* This file is part of the coreboot project. */ - -SECTIONS { - . = (0xffffffff - CONFIG_ID_SECTION_OFFSET) - (__id_end - __id_start) + 1; - .id (.): { - KEEP(*(.id)) - } -} diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld index 05efac6..ed1dbb2 100644 --- a/src/arch/x86/memlayout.ld +++ b/src/arch/x86/memlayout.ld @@ -53,7 +53,6 @@ /* Bootblock specific scripts which provide more SECTION directives. */ #include <cpu/x86/16bit/entry16.ld> #include <cpu/x86/16bit/reset16.ld> -#include <arch/x86/id.ld> #if CONFIG(CPU_INTEL_FIRMWARE_INTERFACE_TABLE) #include <cpu/intel/fit/fit.ld> #endif diff --git a/src/mainboard/51nb/x210/board.fmd b/src/mainboard/51nb/x210/board.fmd index 1955a05..4e506fa 100644 --- a/src/mainboard/51nb/x210/board.fmd +++ b/src/mainboard/51nb/x210/board.fmd @@ -8,7 +8,8 @@ EC@0x0 0x10000 RW_MRC_CACHE@0x10000 0x10000 SMMSTORE@0x20000 0x40000 - CONSOLE@0x60000 0x20000 + CONSOLE@0x60000 0x1ffc0 + COREBOOT_ID@0x7ffc0 0x40 FMAP@0x80000 0x200 COREBOOT(CBFS) } diff --git a/src/mainboard/cavium/cn8100_sff_evb/board.fmd b/src/mainboard/cavium/cn8100_sff_evb/board.fmd index 34f3161..7ac0d9f 100644 --- a/src/mainboard/cavium/cn8100_sff_evb/board.fmd +++ b/src/mainboard/cavium/cn8100_sff_evb/board.fmd @@ -4,6 +4,7 @@ # 0 - 0x10000 is free for firmware usage. # bootblock starts at 0x20000 FMAP@0x0 0x1000 + COREBOOT_ID@0x1000 0x40 # bootblock includes trusted/non-trusted CLIB, CSIB, # and BL1FWs packaged in # src/soc/cavium/common/Makefile.inc. diff --git a/src/mainboard/emulation/qemu-i440fx/vboot-rwa-16M.fmd b/src/mainboard/emulation/qemu-i440fx/vboot-rwa-16M.fmd index 0d2c9da..dd06a23 100644 --- a/src/mainboard/emulation/qemu-i440fx/vboot-rwa-16M.fmd +++ b/src/mainboard/emulation/qemu-i440fx/vboot-rwa-16M.fmd @@ -10,7 +10,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/emulation/qemu-q35/vboot-rwa-16M.fmd b/src/mainboard/emulation/qemu-q35/vboot-rwa-16M.fmd index 0d2c9da..dd06a23 100644 --- a/src/mainboard/emulation/qemu-q35/vboot-rwa-16M.fmd +++ b/src/mainboard/emulation/qemu-q35/vboot-rwa-16M.fmd @@ -10,7 +10,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/emulation/qemu-q35/vboot-rwab-16M.fmd b/src/mainboard/emulation/qemu-q35/vboot-rwab-16M.fmd index fcbfa95..94899f0 100644 --- a/src/mainboard/emulation/qemu-q35/vboot-rwab-16M.fmd +++ b/src/mainboard/emulation/qemu-q35/vboot-rwab-16M.fmd @@ -19,7 +19,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/facebook/monolith/vboot-ro.fmd b/src/mainboard/facebook/monolith/vboot-ro.fmd index 569971a..3efe768 100644 --- a/src/mainboard/facebook/monolith/vboot-ro.fmd +++ b/src/mainboard/facebook/monolith/vboot-ro.fmd @@ -17,7 +17,8 @@ RO_SECTION@0x1000 0x8DF000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_FRID_PAD@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_FRID_PAD@0x880 0x780 GBB@0x1000 0x4000 COREBOOT(CBFS)@0x5000 0x8DA000 } diff --git a/src/mainboard/facebook/monolith/vboot-rw.fmd b/src/mainboard/facebook/monolith/vboot-rw.fmd index dc2dadf..d8636ca 100644 --- a/src/mainboard/facebook/monolith/vboot-rw.fmd +++ b/src/mainboard/facebook/monolith/vboot-rw.fmd @@ -22,7 +22,8 @@ RO_SECTION@0x1000 0x7F000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_FRID_PAD@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_FRID_PAD@0x880 0x780 GBB@0x1000 0x4000 COREBOOT(CBFS)@0x5000 0x07A000 } diff --git a/src/mainboard/intel/cedarisland_crb/board.fmd b/src/mainboard/intel/cedarisland_crb/board.fmd index 2002f6e..e63fda3 100644 --- a/src/mainboard/intel/cedarisland_crb/board.fmd +++ b/src/mainboard/intel/cedarisland_crb/board.fmd @@ -5,7 +5,8 @@ SI_ME@0x3000 0x2fd5000 SI_PT@0x2fd8000 0x10000 } - FMAP@0x03000000 0x800 + FMAP@0x03000000 0x700 + COREBOOT_ID@0x3000700 0x100 RW_MRC_CACHE@0x3000800 0x10000 COREBOOT(CBFS)@0x3010800 } diff --git a/src/mainboard/intel/galileo/vboot.fmd b/src/mainboard/intel/galileo/vboot.fmd index 4d349bd..e7f3d1c 100644 --- a/src/mainboard/intel/galileo/vboot.fmd +++ b/src/mainboard/intel/galileo/vboot.fmd @@ -44,7 +44,8 @@ RO_SECTION@0x10000 0x1f0000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_FRID_PAD@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_FRID_PAD@0x880 0x780 GBB@0x1000 0x7f000 COREBOOT(CBFS)@0x80000 0x170000 } diff --git a/src/mainboard/intel/leafhill/leafhill.16384.fmd b/src/mainboard/intel/leafhill/leafhill.16384.fmd index a91ba9a..3ae4d81 100644 --- a/src/mainboard/intel/leafhill/leafhill.16384.fmd +++ b/src/mainboard/intel/leafhill/leafhill.16384.fmd @@ -1,7 +1,8 @@ FLASH 16M { SI_DESC@0x0 0x1000 IFWI@0x1000 0x2ff000 - FMAP@0x300000 0x800 + FMAP@0x300000 0x700 + COREBOOT_ID@0x300700 0x100 COREBOOT(CBFS)@0x300800 0xc1d800 UNIFIED_MRC_CACHE@0xf1e000 0x21000 { RECOVERY_MRC_CACHE@0x0 0x10000 diff --git a/src/mainboard/intel/leafhill/leafhill.8192.fmd b/src/mainboard/intel/leafhill/leafhill.8192.fmd index 3f4d21b..f754d82 100644 --- a/src/mainboard/intel/leafhill/leafhill.8192.fmd +++ b/src/mainboard/intel/leafhill/leafhill.8192.fmd @@ -1,7 +1,8 @@ FLASH 8M { SI_DESC@0x0 0x1000 IFWI@0x1000 0x300000 - FMAP@0x321000 0x800 + FMAP@0x321000 0x700 + COREBOOT_ID@0x321700 0x100 COREBOOT(CBFS)@0x321800 0x300000 UNIFIED_MRC_CACHE@0x621800 0x21000 { RECOVERY_MRC_CACHE@0x0 0x10000 diff --git a/src/mainboard/intel/minnow3/minnow3.fmd b/src/mainboard/intel/minnow3/minnow3.fmd index d51b5ee..b3ee60a 100644 --- a/src/mainboard/intel/minnow3/minnow3.fmd +++ b/src/mainboard/intel/minnow3/minnow3.fmd @@ -1,7 +1,8 @@ FLASH 16M { SI_DESC@0x0 0x1000 IFWI@0x1000 0x300000 - FMAP@0x301000 0x800 + FMAP@0x301000 0x700 + COREBOOT_ID@0x301700 0x100 COREBOOT(CBFS)@0x301800 0x3dc800 UNIFIED_MRC_CACHE@0x6de000 0x21000 { RECOVERY_MRC_CACHE@0x0 0x10000 diff --git a/src/mainboard/lenovo/t400/vboot-rwa.fmd b/src/mainboard/lenovo/t400/vboot-rwa.fmd index 4af3fcd..7fff352 100644 --- a/src/mainboard/lenovo/t400/vboot-rwa.fmd +++ b/src/mainboard/lenovo/t400/vboot-rwa.fmd @@ -17,7 +17,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/t410/vboot-rwa.fmd b/src/mainboard/lenovo/t410/vboot-rwa.fmd index 8f50d33..6aa8bf4 100644 --- a/src/mainboard/lenovo/t410/vboot-rwa.fmd +++ b/src/mainboard/lenovo/t410/vboot-rwa.fmd @@ -22,7 +22,8 @@ RO_SECTION 0x11e000 { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 GBB 0x1e000 COREBOOT(CBFS) } diff --git a/src/mainboard/lenovo/t420/vboot-rwa.fmd b/src/mainboard/lenovo/t420/vboot-rwa.fmd index 8a4cd3b..ac157c6 100644 --- a/src/mainboard/lenovo/t420/vboot-rwa.fmd +++ b/src/mainboard/lenovo/t420/vboot-rwa.fmd @@ -20,7 +20,8 @@ WP_RO@0x1e1000 0x11f000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_PADDING@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_PADDING@0x880 0x780 RO_VPD(PRESERVE)@0x1000 0x1000 GBB@0x2000 0x1e000 COREBOOT(CBFS)@0x20000 0xff000 diff --git a/src/mainboard/lenovo/t420s/vboot-rwa.fmd b/src/mainboard/lenovo/t420s/vboot-rwa.fmd index 8a4cd3b..ac157c6 100644 --- a/src/mainboard/lenovo/t420s/vboot-rwa.fmd +++ b/src/mainboard/lenovo/t420s/vboot-rwa.fmd @@ -20,7 +20,8 @@ WP_RO@0x1e1000 0x11f000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_PADDING@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_PADDING@0x880 0x780 RO_VPD(PRESERVE)@0x1000 0x1000 GBB@0x2000 0x1e000 COREBOOT(CBFS)@0x20000 0xff000 diff --git a/src/mainboard/lenovo/t430/vboot-rwab.fmd b/src/mainboard/lenovo/t430/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/t430/vboot-rwab.fmd +++ b/src/mainboard/lenovo/t430/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/t430s/vboot-rwab.fmd b/src/mainboard/lenovo/t430s/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/t430s/vboot-rwab.fmd +++ b/src/mainboard/lenovo/t430s/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/t440p/vboot-rwab.fmd b/src/mainboard/lenovo/t440p/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/t440p/vboot-rwab.fmd +++ b/src/mainboard/lenovo/t440p/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/t520/vboot-rwa.fmd b/src/mainboard/lenovo/t520/vboot-rwa.fmd index 8a4cd3b..ac157c6 100644 --- a/src/mainboard/lenovo/t520/vboot-rwa.fmd +++ b/src/mainboard/lenovo/t520/vboot-rwa.fmd @@ -20,7 +20,8 @@ WP_RO@0x1e1000 0x11f000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_PADDING@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_PADDING@0x880 0x780 RO_VPD(PRESERVE)@0x1000 0x1000 GBB@0x2000 0x1e000 COREBOOT(CBFS)@0x20000 0xff000 diff --git a/src/mainboard/lenovo/t530/vboot-rwab.fmd b/src/mainboard/lenovo/t530/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/t530/vboot-rwab.fmd +++ b/src/mainboard/lenovo/t530/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/x131e/vboot-rwab.fmd b/src/mainboard/lenovo/x131e/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/x131e/vboot-rwab.fmd +++ b/src/mainboard/lenovo/x131e/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/x1_carbon_gen1/vboot-rwab.fmd b/src/mainboard/lenovo/x1_carbon_gen1/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/x1_carbon_gen1/vboot-rwab.fmd +++ b/src/mainboard/lenovo/x1_carbon_gen1/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/x200/vboot-rwa.fmd b/src/mainboard/lenovo/x200/vboot-rwa.fmd index 4af3fcd..7fff352 100644 --- a/src/mainboard/lenovo/x200/vboot-rwa.fmd +++ b/src/mainboard/lenovo/x200/vboot-rwa.fmd @@ -17,7 +17,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/lenovo/x201/vboot-rwa.fmd b/src/mainboard/lenovo/x201/vboot-rwa.fmd index 0d1aa5d..78aaeb1 100644 --- a/src/mainboard/lenovo/x201/vboot-rwa.fmd +++ b/src/mainboard/lenovo/x201/vboot-rwa.fmd @@ -21,7 +21,8 @@ RO_SECTION { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 GBB 0x1e000 COREBOOT(CBFS) } diff --git a/src/mainboard/lenovo/x220/vboot-rwa.fmd b/src/mainboard/lenovo/x220/vboot-rwa.fmd index 8a4cd3b..ac157c6 100644 --- a/src/mainboard/lenovo/x220/vboot-rwa.fmd +++ b/src/mainboard/lenovo/x220/vboot-rwa.fmd @@ -20,7 +20,8 @@ WP_RO@0x1e1000 0x11f000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_PADDING@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_PADDING@0x880 0x780 RO_VPD(PRESERVE)@0x1000 0x1000 GBB@0x2000 0x1e000 COREBOOT(CBFS)@0x20000 0xff000 diff --git a/src/mainboard/lenovo/x230/vboot-rwab.fmd b/src/mainboard/lenovo/x230/vboot-rwab.fmd index 1747c0e..df60275 100644 --- a/src/mainboard/lenovo/x230/vboot-rwab.fmd +++ b/src/mainboard/lenovo/x230/vboot-rwab.fmd @@ -25,7 +25,8 @@ WP_RO { FMAP 0x800 RO_FRID 0x40 - RO_PADDING 0x7c0 + COREBOOT_ID 0x40 + RO_PADDING 0x780 RO_VPD(PRESERVE) 0x1000 GBB 0x1e000 COREBOOT(CBFS) diff --git a/src/mainboard/ocp/tiogapass/board.fmd b/src/mainboard/ocp/tiogapass/board.fmd index 1e3fda7..53f848b 100644 --- a/src/mainboard/ocp/tiogapass/board.fmd +++ b/src/mainboard/ocp/tiogapass/board.fmd @@ -5,7 +5,8 @@ PLATFORM_DATA@0xa26000 0x10000 } SI_BIOS@0x1000000 0x1000000 { - FMAP@0x0 0x800 + FMAP@0x0 0x700 + COREBOOT_ID@0x700 0x100 COREBOOT(CBFS)@0x800 0xfff800 } } diff --git a/src/mainboard/opencellular/elgon/board.fmd b/src/mainboard/opencellular/elgon/board.fmd index 784f3b0..e5ea8cb 100644 --- a/src/mainboard/opencellular/elgon/board.fmd +++ b/src/mainboard/opencellular/elgon/board.fmd @@ -4,6 +4,7 @@ # 0 - 0x10000 is free for firmware usage. # bootblock starts at 0x20000 FMAP@0x0 0x1000 + COREBOOT_ID@0x1000 0x40 # bootblock includes trusted/non-trusted CLIB, CSIB, # and BL1FWs packaged in # src/soc/cavium/common/Makefile.inc. diff --git a/src/mainboard/opencellular/elgon/vboot.fmd b/src/mainboard/opencellular/elgon/vboot.fmd index 0d6af2d..909e8de 100644 --- a/src/mainboard/opencellular/elgon/vboot.fmd +++ b/src/mainboard/opencellular/elgon/vboot.fmd @@ -6,6 +6,7 @@ # bootblock starts at 0x20000 FMAP@0x0 0x1000 RO_FRID@0x1000 0x100 + COREBOOT_ID@0x1100 0x40 # bootblock includes trusted/non-trusted CLIB, CSIB, # and BL1FWs packaged in # src/soc/cavium/common/Makefile.inc. diff --git a/src/mainboard/siemens/mc_apl1/mc_apl1.fmd b/src/mainboard/siemens/mc_apl1/mc_apl1.fmd index 36772d6..fdeb65c 100644 --- a/src/mainboard/siemens/mc_apl1/mc_apl1.fmd +++ b/src/mainboard/siemens/mc_apl1/mc_apl1.fmd @@ -1,7 +1,8 @@ FLASH 16M { SI_DESC@0x0 0x1000 IFWI 0x2ff000 - FMAP 0x800 + FMAP 0x700 + COREBOOT_ID 0x100 COREBOOT(CBFS) 0xb9d800 UNIFIED_MRC_CACHE 0x21000 { RECOVERY_MRC_CACHE 0x10000 diff --git a/src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd b/src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd index 8af2e9a..d165d71 100644 --- a/src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd +++ b/src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd @@ -6,7 +6,8 @@ RO_SECTION 0xb8f000 { FMAP 0x800 RO_FRID 0x40 - RO_FRID_PAD 0x7c0 + COREBOOT_ID 0x40 + RO_FRID_PAD 0x780 COREBOOT(CBFS) 0xb4e000 GBB 0x40000 } diff --git a/src/mainboard/supermicro/x11-lga1151-series/vboot-ro-rwab.fmd b/src/mainboard/supermicro/x11-lga1151-series/vboot-ro-rwab.fmd index a295680..aa38f37 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/vboot-ro-rwab.fmd +++ b/src/mainboard/supermicro/x11-lga1151-series/vboot-ro-rwab.fmd @@ -27,7 +27,8 @@ RO_SECTION@0x4000 0x41d000 { FMAP@0x0 0x800 RO_FRID@0x800 0x40 - RO_FRID_PAD@0x840 0x7c0 + COREBOOT_ID@0x840 0x40 + RO_FRID_PAD@0x880 0x780 GBB@0x1000 0xef000 COREBOOT(CBFS)@0xf0000 0x32d000 } diff --git a/src/mainboard/up/squared/upsquared.fmd b/src/mainboard/up/squared/upsquared.fmd index 891138e..86c7cea 100644 --- a/src/mainboard/up/squared/upsquared.fmd +++ b/src/mainboard/up/squared/upsquared.fmd @@ -9,7 +9,8 @@ RW_MRC_CACHE@0x10000 0x10000 RW_VAR_MRC_CACHE@0x20000 0x1000 } - CONSOLE@0x22000 0x20000 + CONSOLE@0x22000 0x1ffc0 + COREBOOT_ID@0x41fc0 0x40 COREBOOT(CBFS)@0x42000 0xb7d000 BIOS_UNUSABLE@0xbbf000 0x40000 } diff --git a/src/mainboard/up/squared/vboot-ro.fmd b/src/mainboard/up/squared/vboot-ro.fmd index 72f92fe..8c71f53 100644 --- a/src/mainboard/up/squared/vboot-ro.fmd +++ b/src/mainboard/up/squared/vboot-ro.fmd @@ -7,7 +7,8 @@ RO_VPD(PRESERVE)@0x2ff800 0x4000 RO_SECTION@0x303800 0xb8d800 { RO_FRID@0x0 0x40 - RO_FRID_PAD@0x40 0x7c0 + COREBOOT_ID@0x40 0x40 + RO_FRID_PAD@0x80 0x780 GBB@0x800 0x40000 COREBOOT(CBFS)@0x40800 0xb4d000 } diff --git a/src/mainboard/up/squared/vboot-roa.fmd b/src/mainboard/up/squared/vboot-roa.fmd index 330ce03..118cde9 100644 --- a/src/mainboard/up/squared/vboot-roa.fmd +++ b/src/mainboard/up/squared/vboot-roa.fmd @@ -7,7 +7,8 @@ RO_VPD(PRESERVE)@0x2ff800 0x4000 RO_SECTION@0x303800 0x1fe800 { RO_FRID@0x0 0x40 - RO_FRID_PAD@0x40 0x7c0 + COREBOOT_ID@0x40 0x40 + RO_FRID_PAD@0x80 0x780 GBB@0x800 0x40000 COREBOOT(CBFS)@0x40800 0x1be000 } diff --git a/src/mainboard/up/squared/vboot-roab.fmd b/src/mainboard/up/squared/vboot-roab.fmd index 4ddcbc0..512355d 100644 --- a/src/mainboard/up/squared/vboot-roab.fmd +++ b/src/mainboard/up/squared/vboot-roab.fmd @@ -7,7 +7,8 @@ RO_VPD(PRESERVE)@0x2ff800 0x4000 RO_SECTION@0x303800 0x1ff800 { RO_FRID@0x0 0x40 - RO_FRID_PAD@0x40 0x7c0 + COREBOOT_ID@0x40 0x40 + RO_FRID_PAD@0x80 0x780 GBB@0x800 0x40000 COREBOOT(CBFS)@0x40800 0x1bf000 } diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index f0143e9..ac958b4 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -12,6 +12,7 @@ ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## ##SMMSTORE_ENTRY## + ##COREBOOT_ID_ENTRY## FMAP@##FMAP_BASE## ##FMAP_SIZE## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## } diff --git a/util/cbfstool/default.fmd b/util/cbfstool/default.fmd index 8f4819e..7c8fc2c 100644 --- a/util/cbfstool/default.fmd +++ b/util/cbfstool/default.fmd @@ -15,6 +15,7 @@ FMAP@##FMAP_BASE## ##FMAP_SIZE## ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## + ##COREBOOT_ID_ENTRY## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## } }
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 1:
This needs support in flashrom's show_id() function (in its layout.c). Adding quasisec@ for visibility.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780 I have built this patchset for mc_apl5 which uses a vboot scheme. Here is a hexdump of the related image portion: ***** 00304800 53 69 65 6d 65 6e 73 5f 4d 43 20 41 50 4c 35 2e |Siemens_MC APL5.| 00304810 34 2e 31 31 2d 32 31 39 35 2d 67 63 64 37 32 64 |4.11-2195-gcd72d| 00304820 31 37 63 38 30 00 00 00 00 00 00 00 00 00 00 00 |17c80...........| 00304830 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| 00304840 34 2e 31 31 2d 32 31 39 35 2d 67 63 64 37 32 64 |4.11-2195-gcd72d| 00304850 31 37 63 38 30 00 53 69 65 6d 65 6e 73 00 4d 43 |17c80.Siemens.MC| 00304860 20 41 50 4c 35 00 ff ff ff ff ff ff ff ff ff ff | APL5...........| ***** Now we do have nearly the same information twice, sitting next to each other in the image. The first part comes from the region RO_FRID while the second one is the new stuff added by your patch. Is this realy needed or should we skip it for VBOOT enabled boards like chromebooks do?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
I have built this patchset for mc_apl5 which uses a vboot scheme. […]
You are right. With this change, there is not much value in adding the ID for vboot-enabled boards since it is just adding redundant info. I can skip it for vboot enabled boards in general.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/lenovo/t530/v... File src/mainboard/lenovo/t530/vboot-rwab.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/lenovo/t530/v... PS2, Line 28: COREBOOT_ID 0x40 some use 0x40, other 0x100 as size for COREBOOT_ID. why?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S File src/arch/x86/id.S:
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S@a16 PS2, Line 16: .long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the were these values ever directly used?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S File src/arch/x86/id.S:
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S@a16 PS2, Line 16: .long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the
were these values ever directly used?
flashrom's show_id() function (in flashrom's layout.c) uses these fields for a sanity check that coreboot-to-coreboot updates (with these fields intact) remain compatible, I think.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S File src/arch/x86/id.S:
https://review.coreboot.org/c/coreboot/+/40377/2/src/arch/x86/id.S@a16 PS2, Line 16: .long __id_end + CONFIG_ID_SECTION_OFFSET - ver /* Reverse offset to the
flashrom's show_id() function (in flashrom's layout. […]
Thanks, Patrick. I read your first comment and somehow forgot (and didn't look).
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/lenovo/t530/v... File src/mainboard/lenovo/t530/vboot-rwab.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/lenovo/t530/v... PS2, Line 28: COREBOOT_ID 0x40
some use 0x40, other 0x100 as size for COREBOOT_ID. […]
0x40 is all that is needed. On most platforms, there was padding present where I could borrow from. In other cases, I borrowed from a huge FMAP region, but to keep size of previous and next regions aligned similarly, I borrowed a little extra. I can rework to make all 0x40.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
Sorry, I really don't like this. :/ If we can't agree on where to put it can we at least agree to remove it entirely, maybe?
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
You are right. […]
Well, I'm still really not a fan of making a separate FMAP section for this (trying to merge discussion from CB:40376). And now you're saying some boards aren't even going to have it? What good is an ID when you can't rely on it being there? And in return if someone tried to build without vboot for a Chrome OS FMAP they're gonna get weird errors.
If we think this information isn't useful, we might as well drop it completely. I'm pretty sure nobody is using it right now. (The show_id() function in Chromium flashrom is dead code, and it upstream flashrom I can't find it at all. Also, it would've never worked on non-x86 boards in the current state. I don't know what flashrom would need this info for anyway.)
If we want to keep this in the image as separate FYI information (like we already do with the 'config' and 'revision' files), I think that's fine, but then the right place to put it is CBFS. FMAP sections are clunky and high touch and we shouldn't create more of them just for something we don't really need anyway. There's a reason we don't just put every file into a separate FMAP section.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
FMAP sections are clunky and high touch and we shouldn't create more of them just for something we don't really need anyway. There's a reason we don't just put every file into a separate FMAP section.
Maybe we can discuss on mailing list, but I'm curious to hear more about your 'clunky and high touch' assessment.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
Maybe we can discuss on mailing list, but I'm curious to hear more about your 'clunky and high touch' assessment.
I mean that you have to always manually put this entry in every FMAP you write. It's one more piece of boilerplate to carry around. In fact I bet the main reason Furquan is excluding CONFIG_CHROMEOS from this is to avoid having to update a hundred FMAPs of old Chrome OS boards, which leads us to the weird inconsistency where some configurations will have this ID and some won't (which I think is a bad idea... if we want to have this ID, it should be consistent everywhere, otherwise it's pretty useless). I think that's somewhat proving my point that putting things in FMAP is high-touch and we shouldn't add more stuff there without good reason.
I think with two layout mechanisms we should have clear reasons what goes in which one and shouldn't be putting things here or there arbitrarily. CBFS is generally more efficient and convenient, but makes precise placement more difficult. So basically stuff that needs to be placed in a specific spot or aligned to an erase block should be an FMAP section, everything else should be in CBFS (and yes, that means some of our legacy FMAP regions like GBB or FRID should would probably also be better placed in CBFS, and I'm considering to move them when I have to touch all that stuff anyway). This ID block has no placement requirements. It is functionally the same thing as the 'config' and 'revision' files we already place in CBFS.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
In fact I bet the main reason Furquan is excluding CONFIG_CHROMEOS from this is to avoid having to update a hundred FMAPs of old Chrome OS boards, which leads us to the weird inconsistency where some configurations will have this ID and some won't (which I think is a bad idea... if we want to have this ID, it should be consistent everywhere, otherwise it's pretty useless).
Actually, no. The reason I dropped it from Chrome OS boards is because I am confident that we don't use it and we have RO_FRID which already stores similar information. I can add it back if you want all boards to be consistent. But, I don't think it really adds more value to Chrome OS boards where we already have RO_FRID.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
Actually, no. The reason I dropped it from Chrome OS boards is because I am confident that we don't use it and we have RO_FRID which already stores similar information. I can add it back if you want all boards to be consistent.
Okay, but... don't you still think that adding it to 100 different FMAP files (and having to subsequently fit it into every future FMAP we'll write) would be a hassle that would be nice to avoid?
But, I don't think it really adds more value to Chrome OS boards where we already have RO_FRID.
Note that they look more similar here than they actually are, since for a developer build the default sets the vboot version to the coreboot version (including Git SHA). For actual production Chromebook firmware this gets overridden by our official firmware version (from the branch builder) and the coreboot Git SHA is lost.
Then again, like I said I doubt it adds much value to anyone at all right now so I would also be okay with removing it entirely unless non-Chrome people say they like to have it. But if we do keep it as the official coreboot version info independent from vboot, I think that should apply to all configurations.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
Actually, no. The reason I dropped it from Chrome OS boards is because I am confident that we don't use it and we have RO_FRID which already stores similar information. I can add it back if you want all boards to be consistent.
Okay, but... don't you still think that adding it to 100 different FMAP files (and having to subsequently fit it into every future FMAP we'll write) would be a hassle that would be nice to avoid?
Yes, our tooling in this area is not great at all which leads to things being cumbersome. This is obviously my opinion, but I'd like to see us drop C as our tool language and Make doing as many complex things as it does in coreboot. We mad that decision 3 years ago or so about not having better tools in image composition, and I think we keep having issues because of it. Anyway, my $.02. :)
FWIW, I don't think the id thing adds much value, but it's not something *I* use. I haven't seen a lot of complaints from others on this patchset so maybe we just nuke the whole thing?
Thanks for your elaboration, Julius, on the 'clunky and high touch' opinion.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
Actually, no. […]
I share the opinion with Julius that if it really is a pure ID, than all boards should have it in the same manner. And from my point of view for such an ID there is no real need to have any address constraints so there is no real need to place it into a FMAP region, CBFS would serve the needs just fine. What I am not sure about is if we really should drop it completely. I use the git hash and mainboard name it contains every now and then to check an image I have read back from a flash. I still can find this information in console strings in the image but this is something that may change in the future and is a bit unhandy. So maybe we can agree on a ID format which is consistent for all boars with the same content all across the tree? And for me it would be fine to place it into the CBFS. The amount of flash space we need is little and should not lead to flash size issues. With this approach one could simply use cbfstool to get the content of this ID on an image if needed.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
Sorry for being late here. Some information from a non-vboot user:
We have our update process in the payload, it checks the id of an image to flash. It's expected at the end of CBFS as it was there for more than a decade (on x86). If we remove it there, we might have to revert this downstream to stay compatible with our current production firmware, but only if we decided to jump to a newer branch for an existing product (unlikely).
Nevertheless, I don't think we should remove this without a more public discussion. There may be more coreboot ecosystem using it than we know.
I haven't looked into the implementation here, but the idea of an FMAP region seems odd: The id section used to contain a version, don't we have multiple versions in one image in case of vboot? Wouldn't that need an FMAP region per RO/RW slot to be consistent?
If we want to unify things, I would propose to put it at the end of each CBFS. So far, every CBFS seems to comprise one version of coreboot. Why at the end? well, it's already there on x86 (non- reset-vector-in-ram case), and I don't expect it to collide in other cases.
Another thought, if we want to get rid of this legacy, x86 location, but keep the id, how about adding a simple file to CBFS as standard from now on, and keeping the legacy id in the bootblock for a few releases until users adapted?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
Looking at a current CBFS, we already have a `revision` file... which is not very machine readable, though. And what is still missing is the board/vendor id, but that really shouldn't differ between multiple CBFS's inside a single image.
From an academic viewpoint, I would probably put the version into the CBFS header and board/vendor into the FMAP header ;) Not sure how feasible that is.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
Another thought, if we want to get rid of this legacy, x86 location, but keep the id, how about adding a simple file to CBFS as standard from now on, and keeping the legacy id in the bootblock for a few releases until users adapted?
That's also my preferred option. (I would be fine with keeping the old .id in parallel for a bit longer too, but I don't know if Furquan had any specific pressure to get rid of it right now.)
From an academic viewpoint, I would probably put the version into the CBFS header and board/vendor into the FMAP header ;) Not sure how feasible that is.
If you actually mean header, I don't think there's any space for those kind of things in there, and I don't think we'd want to mess with these well-established structures.
Looking at a current CBFS, we already have a `revision` file... which is not very machine readable, though. And what is still missing is the board/vendor id, but that really shouldn't differ between multiple CBFS's inside a single image.
Yeah, it would probably make sense to either merge this into the revision file or supersede that file with whatever new format that contains all that info we'd make up.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... File src/mainboard/siemens/mc_apl1/mc_apl_vboot.fmd:
https://review.coreboot.org/c/coreboot/+/40377/2/src/mainboard/siemens/mc_ap... PS2, Line 9: COREBOOT_ID 0x40 : RO_FRID_PAD 0x780
The reason I chose FMAP is because flashrom (which I think is a commonly used tool) supports FMAP already. So, extending flashrom to support reading ID is not really a lot of change.
Like I said above, I think the flashrom code for doing this is dead (unused) anyway? So I don't think we need to worry about that immediately, and for potential future uses in other tools I'm not sure the "more likely to know FMAP than CBFS" assumption necessarily holds. If we really want something universally usable, I think we should just put a unique magic number in front of it that tools can search for, and then it's fine to put it (uncompressed) into CBFS too.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Patch Set 2:
Patch Set 2:
Another thought, if we want to get rid of this legacy, x86 location, but keep the id, how about adding a simple file to CBFS as standard from now on, and keeping the legacy id in the bootblock for a few releases until users adapted?
That's also my preferred option. (I would be fine with keeping the old .id in parallel for a bit longer too, but I don't know if Furquan had any specific pressure to get rid of it right now.)
No, I think we can keep it for a couple of releases and eventually drop the .id support.
Furquan Shaikh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40377 )
Change subject: coreboot-id: Add coreboot id based on FMAP entries ......................................................................
Abandoned
Haven't done anything in 1+ year
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Werner Zeh (Code Review)