Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to review the following change.
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added CBMEM cache. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 20 files changed, 131 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/1
diff --git a/Makefile.inc b/Makefile.inc index e8a2d52..cd6b23d 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -226,7 +226,7 @@ # Add handler to copy linker scripts define generic-objs_ld_template_gen de$(EMPTY)fine $(1)-objs_ld_template -$$(call src-to-obj,$1,$$(1).ld): $$(1).ld $(obj)/config.h +$$(call src-to-obj,$1,$$(1).ld): $$(1).ld $(obj)/config.h $(obj)/fmap_config.h @printf " CP $$$$(subst $$$$(obj)/,,$$$$(@))\n" $$(CC_$(1)) -MMD $$(CPPFLAGS_$(1)) $$($(1)-ld-ccopts) $(PREPROCESS_ONLY) -include $(obj)/config.h -MT $$$$@ -o $$$$@ $$$$< en$(EMPTY)def diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index 502e774..a95cda0 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -209,6 +209,12 @@ hex default 0x80
+config FMAP_CACHE_SIZE + hex + default 0x800 + help + Size of the pre-RAM FMAP cache. Set to 0 to disable. + # 64KiB default bootblock size when employing C_ENVIRONMENT_BOOTBLOCK. config C_ENV_BOOTBLOCK_SIZE hex diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 6ccbd8c..7ea270a 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -33,6 +33,7 @@ #if CONFIG(VBOOT_MEASURED_BOOT) VBOOT2_TPM_LOG(., 2K) #endif + FMAP_CACHE(., CONFIG_FMAP_CACHE_SIZE) /* Stack for CAR stages. Since it persists across all stages that * use CAR it can be reused. The chipset/SoC is expected to provide * the stack size. */ diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 0100e27..56dfb6a 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -21,6 +21,8 @@ #include <arch/memlayout.h> #include <vb2_constants.h>
+#include "fmap_config.h" + /* Macros that the architecture can override. */ #ifndef ARCH_POINTER_ALIGN_SIZE #define ARCH_POINTER_ALIGN_SIZE 8 @@ -30,7 +32,8 @@ #define ARCH_CACHELINE_ALIGN_SIZE 64 #endif
-#define STR(x) #x +#define STR(x) XSTR(x) +#define XSTR(x) #x
#define ALIGN_COUNTER(align) \ . = ALIGN(align); @@ -73,6 +76,11 @@ ALIAS_REGION(cbfs_cache, preram_cbfs_cache) \ ALIAS_REGION(cbfs_cache, postram_cbfs_cache)
+#define FMAP_CACHE(addr, sz) \ + REGION(fmap_cache, addr, sz, 4) \ + _ = ASSERT(sz == 0 || sz >= FMAP_SIZE, \ + STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); + #if ENV_ROMSTAGE_OR_BEFORE #define PRERAM_CBFS_CACHE(addr, size) \ REGION(preram_cbfs_cache, addr, size, 4) \ diff --git a/src/include/symbols.h b/src/include/symbols.h index 56df8d5..eec4701 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -34,6 +34,7 @@ DECLARE_REGION(preram_cbfs_cache) DECLARE_REGION(postram_cbfs_cache) DECLARE_REGION(cbfs_cache) +DECLARE_REGION(fmap_cache) DECLARE_REGION(payload)
/* "program" always refers to the current execution unit. */ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index e5678ff..02050ae 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -208,13 +208,6 @@ $(call src-to-obj,verstage,$(dir)/version.c) : $(obj)/build.h $(call src-to-obj,postcar,$(dir)/version.c) : $(obj)/build.h
-$(call src-to-obj,bootblock,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,romstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,ramstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,smm,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,verstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,postcar,$(dir)/cbfs.c) : $(obj)/fmap_config.h - $(call src-to-obj,bootblock,$(dir)/fmap.c) : $(obj)/fmap_config.h $(call src-to-obj,romstage,$(dir)/fmap.c) : $(obj)/fmap_config.h $(call src-to-obj,ramstage,$(dir)/fmap.c) : $(obj)/fmap_config.h diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 1e8a93f..8b0f34c 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -26,7 +26,6 @@ #include <symbols.h> #include <timestamp.h> #include <fmap.h> -#include "fmap_config.h" #include <security/vboot/vboot_crtm.h>
#define ERROR(x...) printk(BIOS_ERR, "CBFS: " x) @@ -294,7 +293,10 @@ if (bdev == NULL) return -1;
- size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE; + struct region fmap_region; + if (fmap_locate_area("COREBOOT", &fmap_region)) + return -1; + size_t fmap_top = fmap_region.offset + fmap_region.size;
/* Find location of header using signed 32-bit offset from * end of CBFS region. */ diff --git a/src/lib/fmap.c b/src/lib/fmap.c index a427102..9c19773 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -15,12 +15,13 @@
#include <arch/early_variables.h> #include <boot_device.h> +#include <cbmem.h> #include <console/console.h> #include <fmap.h> #include <commonlib/fmap_serialized.h> #include <stddef.h> #include <string.h> -#include <cbmem.h> +#include <symbols.h>
#include "fmap_config.h"
@@ -31,27 +32,87 @@ static int fmap_print_once CAR_GLOBAL; static struct mem_region_device fmap_cache CAR_GLOBAL;
+#define print_once(...) if (!car_get_var(fmap_print_once)) printk(__VA_ARGS__) + +DECLARE_OPTIONAL_REGION(fmap_cache); + uint64_t get_fmap_flash_offset(void) { return FMAP_OFFSET; }
+static int check_signature(struct fmap *fmap) +{ + return memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature)); +} + +static void report(struct fmap *fmap) +{ + print_once(BIOS_DEBUG, "FMAP: Found "%s" version %d.%d at %#x.\n", + fmap->name, fmap->ver_major, fmap->ver_minor, FMAP_OFFSET); + print_once(BIOS_DEBUG, "FMAP: base = %#llx size = %#x #areas = %d\n", + (long long)fmap->base, fmap->size, fmap->nareas); + car_set_var(fmap_print_once, 1); +} + +static void setup_preram_cache(struct mem_region_device *cache_mrdev) +{ + if (!ENV_ROMSTAGE_OR_BEFORE) { + print_once(BIOS_WARNING, + "WARNING: Post-RAM FMAP access too early for cache!\n"); + return; + } + + if (REGION_SIZE(fmap_cache) == 0) { + print_once(BIOS_WARNING, + "WARNING: Running without pre-RAM FMAP_CACHE!\n"); + return; + } + + struct fmap *fmap = (struct fmap *)_fmap_cache; + if (!ENV_BOOTBLOCK) { + /* NOTE: This assumes that for all platforms running this code, + the bootblock is the first stage and the bootblock will make + at least one FMAP access (usually from finding CBFS). */ + if (!check_signature(fmap)) + goto register_cache; + + printk(BIOS_ERR, "ERROR: FMAP cache corrupted?!\n"); + } + + /* In case we fail below, make sure the cache is invalid. */ + memset(fmap->signature, 0, sizeof(fmap->signature)); + + boot_device_init(); + const struct region_device *boot_rdev = boot_device_ro(); + if (!boot_rdev) + return; + + /* memlayout statically guarantees that the FMAP_CACHE is big enough. */ + if (rdev_readat(boot_rdev, fmap, FMAP_OFFSET, FMAP_SIZE) != FMAP_SIZE) + return; + if (check_signature(fmap)) + return; + report(fmap); + +register_cache: + mem_region_device_ro_init(cache_mrdev, fmap, FMAP_SIZE); +} + static int find_fmap_directory(struct region_device *fmrd) { const struct region_device *boot; struct fmap *fmap; - size_t fmap_size; + struct mem_region_device *cache; size_t offset = FMAP_OFFSET;
- if (cbmem_possibly_online() && !ENV_SMM) { - /* Try FMAP cache first */ - const struct mem_region_device *cache; - - cache = car_get_var_ptr(&fmap_cache); - if (region_device_sz(&cache->rdev)) - return rdev_chain(fmrd, &cache->rdev, 0, - region_device_sz(&cache->rdev)); - } + /* Try FMAP cache first */ + cache = car_get_var_ptr(&fmap_cache); + if (!region_device_sz(&cache->rdev)) + setup_preram_cache(cache); + if (region_device_sz(&cache->rdev)) + return rdev_chain(fmrd, &cache->rdev, 0, + region_device_sz(&cache->rdev));
boot_device_init(); boot = boot_device_ro(); @@ -59,32 +120,22 @@ if (boot == NULL) return -1;
- fmap_size = sizeof(struct fmap); - - fmap = rdev_mmap(boot, offset, fmap_size); + fmap = rdev_mmap(boot, offset, sizeof(struct fmap));
if (fmap == NULL) return -1;
- if (memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature))) { + if (check_signature(fmap)) { printk(BIOS_DEBUG, "No FMAP found at %zx offset.\n", offset); rdev_munmap(boot, fmap); return -1; }
- if (!car_get_var(fmap_print_once)) { - printk(BIOS_DEBUG, "FMAP: Found "%s" version %d.%d at %zx.\n", - fmap->name, fmap->ver_major, fmap->ver_minor, offset); - printk(BIOS_DEBUG, "FMAP: base = %llx size = %x #areas = %d\n", - (long long)fmap->base, fmap->size, fmap->nareas); - car_set_var(fmap_print_once, 1); - } - - fmap_size += fmap->nareas * sizeof(struct fmap_area); + report(fmap);
rdev_munmap(boot, fmap);
- return rdev_chain(fmrd, boot, offset, fmap_size); + return rdev_chain(fmrd, boot, offset, FMAP_SIZE); }
int fmap_locate_area_as_rdev(const char *name, struct region_device *area) @@ -213,7 +264,7 @@ return rdev_writeat(&rdev, buffer, 0, size); }
-static void fmap_register_cache(int unused) +static void fmap_register_cbmem_cache(int unused) { const struct cbmem_entry *e; struct mem_region_device *mdev; @@ -233,7 +284,7 @@ * The main reason to copy the FMAP into CBMEM is to make it available to the * OS on every architecture. As side effect use the CBMEM copy as cache. */ -static void fmap_setup_cache(int unused) +static void fmap_setup_cbmem_cache(int unused) { struct region_device fmrd;
@@ -256,9 +307,9 @@ }
/* Finally advertise the cache for the current stage */ - fmap_register_cache(unused); + fmap_register_cbmem_cache(unused); }
-ROMSTAGE_CBMEM_INIT_HOOK(fmap_setup_cache) -RAMSTAGE_CBMEM_INIT_HOOK(fmap_register_cache) -POSTCAR_CBMEM_INIT_HOOK(fmap_register_cache) +ROMSTAGE_CBMEM_INIT_HOOK(fmap_setup_cbmem_cache) +RAMSTAGE_CBMEM_INIT_HOOK(fmap_register_cbmem_cache) +POSTCAR_CBMEM_INIT_HOOK(fmap_register_cbmem_cache) diff --git a/src/soc/mediatek/mt8173/include/soc/memlayout.ld b/src/soc/mediatek/mt8173/include/soc/memlayout.ld index adda86e..2358c39 100644 --- a/src/soc/mediatek/mt8173/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8173/include/soc/memlayout.ld @@ -40,7 +40,8 @@ SRAM_START(0x00100000) VBOOT2_WORK(0x00100000, 12K) VBOOT2_TPM_LOG(0x00103000, 2K) - PRERAM_CBMEM_CONSOLE(0x00103800, 14K) + FMAP_CACHE(0x00103800, 2K) + PRERAM_CBMEM_CONSOLE(0x00104000, 12K) WATCHDOG_TOMBSTONE(0x00107000, 4) PRERAM_CBFS_CACHE(0x00107004, 16K - 4) TIMESTAMP(0x0010B000, 4K) diff --git a/src/soc/mediatek/mt8183/include/soc/memlayout.ld b/src/soc/mediatek/mt8183/include/soc/memlayout.ld index 82e404f..a8f464a 100644 --- a/src/soc/mediatek/mt8183/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8183/include/soc/memlayout.ld @@ -34,7 +34,8 @@ VBOOT2_TPM_LOG(0x00103000, 2K) PRERAM_CBMEM_CONSOLE(0x00103800, 14K) WATCHDOG_TOMBSTONE(0x00107000, 4) - PRERAM_CBFS_CACHE(0x00107004, 48K - 4) + PRERAM_CBFS_CACHE(0x00107004, 46K - 4) + FMAP_CACHE(0x00112800, 2K) TIMESTAMP(0x00113000, 4K) STACK(0x00114000, 16K) TTB(0x00118000, 28K) diff --git a/src/soc/nvidia/tegra124/include/soc/memlayout.ld b/src/soc/nvidia/tegra124/include/soc/memlayout.ld index 7e2f9ec..7e2cc7a 100644 --- a/src/soc/nvidia/tegra124/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra124/include/soc/memlayout.ld @@ -28,7 +28,8 @@ SRAM_START(0x40000000) TTB(0x40000000, 16K + 32) PRERAM_CBMEM_CONSOLE(0x40004020, 6K - 32) - PRERAM_CBFS_CACHE(0x40005800, 16K) + FMAP_CACHE(0x40005800, 2K) + PRERAM_CBFS_CACHE(0x40006000, 14K) VBOOT2_WORK(0x40009800, 12K) VBOOT2_TPM_LOG(0x4000D800, 2K) STACK(0x4000E000, 8K) diff --git a/src/soc/nvidia/tegra210/include/soc/memlayout.ld b/src/soc/nvidia/tegra210/include/soc/memlayout.ld index 6d74ab9..3388bd1 100644 --- a/src/soc/nvidia/tegra210/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra210/include/soc/memlayout.ld @@ -29,7 +29,8 @@ { SRAM_START(0x40000000) PRERAM_CBMEM_CONSOLE(0x40000000, 2K) - PRERAM_CBFS_CACHE(0x40000800, 30K) + FMAP_CACHE(0x40000800, 2K) + PRERAM_CBFS_CACHE(0x40001000, 28K) VBOOT2_WORK(0x40008000, 12K) VBOOT2_TPM_LOG(0x4000B000, 2K) #if ENV_ARM64 diff --git a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld index f1a7bc5..6ff1018 100644 --- a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld @@ -34,7 +34,8 @@ /* This includes bootblock image, can be reused after bootblock starts */ /* UBER_SBL(0x0A0C0000, 48K) */
- PRERAM_CBFS_CACHE(0x0A0C0000, 93K) + PRERAM_CBFS_CACHE(0x0A0C0000, 92K) + FMAP_CACHE(0x0A0EF800, 2K)
TTB(0x0A0F0000, 16K) TTB_SUBTABLES(0x0A0F4000, 4K) diff --git a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld index 25db175..595d939 100644 --- a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld @@ -38,7 +38,8 @@ QCA_SHARED_RAM(2A03F000, 4K) */ STACK(0x2A040000, 16K) - PRERAM_CBFS_CACHE(0x2A044000, 93K) + PRERAM_CBFS_CACHE(0x2A044000, 91K) + FMAP_CACHE(0x2A05B000, 2K) TTB_SUBTABLES(0x2A05B800, 2K) TTB(0x2A05C000, 16K) SRAM_END(0x2A060000) diff --git a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld index 68642d6..dd013b5 100644 --- a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld +++ b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld @@ -39,7 +39,8 @@ TIMESTAMP(0x8C4F000, 1K) PRERAM_CBMEM_CONSOLE(0x8C4F400, 32K) PRERAM_CBFS_CACHE(0x8C57400, 70K) - REGION(bsram_unused, 0x8C68C00, 0xA2400, 0x100) + FMAP_CACHE(0x8C68C00, 2K) + REGION(bsram_unused, 0x8C69400, 0xA1C00, 0x100) BSRAM_END(0x8D80000)
DRAM_START(0x80000000) diff --git a/src/soc/qualcomm/sc7180/include/soc/memlayout.ld b/src/soc/qualcomm/sc7180/include/soc/memlayout.ld index b2ee3b2..3f43419 100644 --- a/src/soc/qualcomm/sc7180/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sc7180/include/soc/memlayout.ld @@ -43,6 +43,7 @@ REGION(ddr_training, 0x14850000, 8K, 4K) REGION(qclib_serial_log, 0x14852000, 4K, 4K) REGION(ddr_information, 0x14853000, 1K, 1K) + FMAP_CACHE(0x14853400, 2K) REGION(dcb, 0x14870000, 16K, 4K) REGION(pmic, 0x14874000, 44K, 4K) REGION(limits_cfg, 0x1487F000, 4K, 4K) diff --git a/src/soc/rockchip/rk3399/include/soc/memlayout.ld b/src/soc/rockchip/rk3399/include/soc/memlayout.ld index 293057a..4e46e2d 100644 --- a/src/soc/rockchip/rk3399/include/soc/memlayout.ld +++ b/src/soc/rockchip/rk3399/include/soc/memlayout.ld @@ -33,7 +33,8 @@ #if ENV_RAMSTAGE REGION(bl31_sram, 0xFF8C0000, 64K, 1) #else - PRERAM_CBFS_CACHE(0xFF8C0000, 7K) + PRERAM_CBFS_CACHE(0xFF8C0000, 5K) + FMAP_CACHE(0xFF8C1400, 2K) TIMESTAMP(0xFF8C1C00, 1K) /* 0xFF8C2004 is the entry point address the masked ROM will jump to. */ OVERLAP_DECOMPRESSOR_VERSTAGE_ROMSTAGE(0xFF8C2004, 88K - 4) diff --git a/src/soc/samsung/exynos5250/include/soc/memlayout.ld b/src/soc/samsung/exynos5250/include/soc/memlayout.ld index 0bd319e..7e052f0 100644 --- a/src/soc/samsung/exynos5250/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5250/include/soc/memlayout.ld @@ -31,7 +31,8 @@ ROMSTAGE(0x2030000, 128K) /* 32K hole */ TTB(0x2058000, 16K) - PRERAM_CBFS_CACHE(0x205C000, 78K) + PRERAM_CBFS_CACHE(0x205C000, 76K) + FMAP_CACHE(0x206F000, 2K) VBOOT2_TPM_LOG(0x206F800, 2K) VBOOT2_WORK(0x2070000, 12K) STACK(0x2074000, 16K) diff --git a/src/soc/samsung/exynos5420/include/soc/memlayout.ld b/src/soc/samsung/exynos5420/include/soc/memlayout.ld index bc5d066..ff781d2 100644 --- a/src/soc/samsung/exynos5420/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5420/include/soc/memlayout.ld @@ -32,7 +32,8 @@ ROMSTAGE(0x2030000, 128K) /* 32K hole */ TTB(0x2058000, 16K) - PRERAM_CBFS_CACHE(0x205C000, 76K) + PRERAM_CBFS_CACHE(0x205C000, 74K) + FMAP_CACHE(0x206E800, 2K) STACK(0x206F000, 16K) /* 1K hole for weird kernel-shared CPU/SMP state structure that doesn't * seem to be implemented right now? */ diff --git a/util/cbfstool/fmaptool.c b/util/cbfstool/fmaptool.c index faf6508..1c01ec0 100644 --- a/util/cbfstool/fmaptool.c +++ b/util/cbfstool/fmaptool.c @@ -24,6 +24,7 @@ #define STDIN_FILENAME_SENTINEL "-"
#define HEADER_FMAP_OFFSET "FMAP_OFFSET" +#define HEADER_FMAP_SIZE "FMAP_SIZE"
enum fmaptool_return { FMAPTOOL_EXIT_SUCCESS = 0, @@ -83,7 +84,8 @@ }
static bool write_header(const char *out_fname, - const struct flashmap_descriptor *root) + const struct flashmap_descriptor *root, + const int fmap_size) { assert(out_fname);
@@ -100,21 +102,8 @@
fputs("#ifndef FMAPTOOL_GENERATED_HEADER_H_\n", header); fputs("#define FMAPTOOL_GENERATED_HEADER_H_\n\n", header); - fprintf(header, "#define %s %#x\n\n", HEADER_FMAP_OFFSET, fmap_offset); - - /* also add defines for each CBFS-carrying fmap region: base and size */ - cbfs_section_iterator_t cbfs_it = cbfs_sections_iterator(); - while (cbfs_it) { - const struct flashmap_descriptor *item = - cbfs_sections_iterator_deref(cbfs_it); - assert(item->offset_known && item->size_known); - unsigned abs_base = fmd_calc_absolute_offset(root, item->name); - fprintf(header, "#define ___FMAP__%s_BASE 0x%x\n", - item->name, abs_base); - fprintf(header, "#define ___FMAP__%s_SIZE 0x%x\n", - item->name, item->size); - cbfs_sections_iterator_advance(&cbfs_it); - } + fprintf(header, "#define %s %#x\n", HEADER_FMAP_OFFSET, fmap_offset); + fprintf(header, "#define %s %#x\n\n", HEADER_FMAP_SIZE, fmap_size);
fputs("#endif\n", header);
@@ -245,7 +234,7 @@ fmap_destroy(flashmap);
if (args.header_filename && - !write_header(args.header_filename, descriptor)) { + !write_header(args.header_filename, descriptor, size)) { full_fmd_cleanup(&descriptor); return FMAPTOOL_EXIT_FAILED_WRITING_HEADER; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36657/1/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/1/src/include/memlayout.h@79 PS1, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/1/src/include/memlayout.h@79 PS1, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/1/src/include/memlayout.h@82 PS1, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/1/src/include/memlayout.h@82 PS1, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/fmap.c@35 PS1, Line 35: #define print_once(...) if (!car_get_var(fmap_print_once)) printk(__VA_ARGS__) Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/cbfs.c@301 PS1, Line 301: /* Find location of header using signed 32-bit offset from Does anyone know why we're still doing all this crap at all? Can I just take the offset and size from the FMAP directly instead?
Hello Aaron Durbin, Patrick Rudolph, Joel Kitching, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to look at the new patch set (#2).
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added CBMEM cache. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 20 files changed, 133 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36657/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/2/src/include/memlayout.h@79 PS2, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/2/src/include/memlayout.h@79 PS2, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/2/src/include/memlayout.h@82 PS2, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/2/src/include/memlayout.h@82 PS2, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/2/src/lib/fmap.c@36 PS2, Line 36: if (!car_get_var(fmap_print_once)) printk(__VA_ARGS__); \ trailing statements should be on next line
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins), Joel Kitching, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to look at the new patch set (#3).
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added CBMEM cache. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/Kconfig M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 20 files changed, 134 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@82 PS3, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@82 PS3, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3:
(4 comments)
I wonder, is it necessary to manually set the size of the CAR/SRAM FMAP cache via a config option? Or could we just set it to the exact size of the known FMAP and just have the config as a on/off? (And if there isn't enough room, it'll still know at link time and fail.)
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@35 PS3, Line 35: #define STR(x) XSTR(x) : #define XSTR(x) #x I don't doubt that this is necessary... but what does it accomplish?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@35 PS3, Line 35: print_once Not sure I like this name... it sounds like the function should take care of setting fmap_print_once itself. On the other hand, I can't think of anything better at the moment.
Will this end up printing once per stage?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@305 PS3, Line 305: const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s); So is this re-reading FMAP when CBMEM comes up, rather than copying it from CAR/SRAM if it is available?
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... File src/soc/mediatek/mt8173/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... PS3, Line 44: 12K Should probably note this reduction in the commit message.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3: Code-Review+1
(3 comments)
I'm starting to wonder if it's not getting easier to define a section for persistent preram objects that you use objcopy to make sure they are the same between early stages or just use the romstage one for all early stages.
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: FMAP_CACHE(addr, sz) Any need to specify the size and not use the one specified by fmap_config.h to match it exactly? If it grows too large would the linker not warn about that too?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@64 PS3, Line 64: print_once(BIOS_WARNING, : "WARNING: Post-RAM FMAP access too early for cache!\n"); I guess it could be helpful to add a comment that this code is reached before cbmem_init?
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@70 PS3, Line 70: BIOS_WARNING BIOS_NOTICE since running without pre-RAM cache is also possible?
Hello Aaron Durbin, Arthur Heymans, Patrick Rudolph, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to look at the new patch set (#4).
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added CBMEM cache. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Add FMAP_CACHEs to all platforms that can afford it (other than the RISC-V things where I have no idea how they work), trying to take the space from things that look like they were oversized anyway (pre-RAM consoles and CBFS caches).
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 19 files changed, 134 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36657/4/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/4/src/include/memlayout.h@79 PS4, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/4/src/include/memlayout.h@79 PS4, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/4/src/include/memlayout.h@82 PS4, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/4/src/include/memlayout.h@82 PS4, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3:
(8 comments)
I wonder, is it necessary to manually set the size of the CAR/SRAM FMAP cache via a config option? Or could we just set it to the exact size of the known FMAP and just have the config as a on/off? (And if there isn't enough room, it'll still know at link time and fail.)
Okay, did that for x86. For Arm, see inline comment for why I'd like to stick to explicit sizes.
I'm starting to wonder if it's not getting easier to define a section for persistent preram objects that you use objcopy to make sure they are the same between early stages or just use the romstage one for all early stages.
Not really sure I get the objcopy part. I agree that we're accumulating quite a lot of these pre-RAM cache areas and it might be nice to combine them somehow... maybe we could just have a big PRERAM_CACHE() section and then FMAP, timestamps, console, CBFS and others will automatically grab their required amount of space from that. One tricky issue would be to make sure one cannot starve the others, though... e.g. you don't want to fail to boot because you added so many timestamps you can't fit a critical file into CBFS cache anymore.
Anyway, one patch at a time.
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld@36 PS3, Line 36: FMAP_CACHE(., CONFIG_FMAP_CACHE_SIZE) Decided to change this to just use FMAP_SIZE. I could add a boolean Kconfig to disable it, but I doubt any of our x86 platforms are so tight on CAR that that would be necessary?
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@35 PS3, Line 35: #define STR(x) XSTR(x) : #define XSTR(x) #x
I don't doubt that this is necessary... […]
It makes it so the 'FMAP_SIZE' macro in the error message below will actually be expanded. It's a common trick (in fact we actually have another copy of STR() in src/arch/riscv/include/bits.h which already does this), I just forgot to add it the first time I wrote this file.
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: FMAP_CACHE(addr, sz)
Any need to specify the size and not use the one specified by fmap_config. […]
This is sort of a philosophical choice... I prefer memlayout to always explicitly contain address and size for every section so that the file can serve as a reference as well as a definition. It's true that we could automatically determine it in this case (and the linker would catch overflows), but you don't really gain anything from that either -- you still have to decide how far behind this you place the next section. I think defining the available space explicitly rather than implicitly through the next member is more clear and more consistent with the rest of memlayout.
(I could however change the x86 version to just use FMAP_SIZE if you want. It would make more sense there because the addresses are dynamic anyway.)
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@35 PS3, Line 35: print_once
Not sure I like this name... it sounds like the function should take care of setting fmap_print_once itself. On the other hand, I can't think of anything better at the moment.
I mean, it does *effectively* print once because all this code will be walked through exactly once before report() is called to disable it. I don't really see a better name either (print_first() or something like that is less clear, I think).
Yes, it will print once per stage (but if you use the cache, all lines using this only execute in the bootblock anyway).
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@64 PS3, Line 64: print_once(BIOS_WARNING, : "WARNING: Post-RAM FMAP access too early for cache!\n");
I guess it could be helpful to add a comment that this code is reached before cbmem_init?
Yeah, that was what I was trying to express without making the message too long. Will add a comment as well.
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@70 PS3, Line 70: BIOS_WARNING
BIOS_NOTICE since running without pre-RAM cache is also possible?
Okay. I just want to make sure it doesn't become common and too easy to overlook (really, the only reason we need to support that is for that damn RK3288 platform with 100K SRAM).
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@305 PS3, Line 305: const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s);
So is this re-reading FMAP when CBMEM comes up, rather than copying it from CAR/SRAM if it is availa […]
No, in the cached case fmrd is chained from fmap_cache here, which is the pre-RAM memory buffer. So even though this is "reading from an rdev", it's really just a memcpy() from CAR/SRAM into CBMEM.
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... File src/soc/mediatek/mt8173/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... PS3, Line 44: 12K
Should probably note this reduction in the commit message.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36657/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36657/4//COMMIT_MSG@10 PS4, Line 10: CBMEM CBMEM cache for FMAP?
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/cbfs.c@299 PS4, Line 299: fmap_region.offset + fmap_region.size; Could you please use region_offset(&fmap_region) and region_sz(&fmap_region). We could introduce a region_end() helper to yield the calculation as well.
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/fmap.c@47 PS4, Line 47: static int check_signature(struct fmap *fmap) const struct
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/fmap.c@52 PS4, Line 52: static void report(struct fmap *fmap) const struct
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/fmap.c@68 PS4, Line 68: "WARNING: Post-RAM FMAP access too early for cache!\n"); nit: function is called setup_preram_cache() but clearly we can get here post ram. It seems a little confusing.
https://review.coreboot.org/c/coreboot/+/36657/4/src/lib/fmap.c@123 PS4, Line 123: region_device_sz(&cache->rdev)); fyi: https://review.coreboot.org/c/coreboot/+/36683
Hello Aaron Durbin, Arthur Heymans, Patrick Rudolph, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to look at the new patch set (#5).
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added post-RAM FMAP cache in CBMEM. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Add FMAP_CACHEs to all platforms that can afford it (other than the RISC-V things where I have no idea how they work), trying to take the space from things that look like they were oversized anyway (pre-RAM consoles and CBFS caches).
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 19 files changed, 134 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 5: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld@37 PS5, Line 37: . = ALIGN(32); Where does this come from?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld@37 PS5, Line 37: . = ALIGN(32);
Where does this come from?
The FMAP size isn't necessarily a multiple of 32, so if I don't put this here the location counter may be misaligned afterwards. This can cause all sorts of issues down below. I just noticed the memlayout alignment assertions for the PRERAM_CBMEM_CONSOLE and TIMESTAMP sections failing, but I could imagine that a misaligned car_stack is probably also not that great.
Ideally, every section in here would take care of aligning itself (like it's done for PDPT below). But right now most of them don't and it seems that there's rather some assumption that the location counter will always maintain a somewhat sane alignment, so putting this in here seems like the safest solution for now. If someone later wants to refactor the whole file and figure out the precise alignments every individual section needs, we could take it out again.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld@37 PS5, Line 37: . = ALIGN(32);
The FMAP size isn't necessarily a multiple of 32, so if I don't put this here the location counter may be misaligned afterwards. This can cause all sorts of issues down below. I just noticed the memlayout alignment assertions for the PRERAM_CBMEM_CONSOLE and TIMESTAMP sections failing, but I could imagine that a misaligned car_stack is probably also not that great.
Ideally, every section in here would take care of aligning itself (like it's done for PDPT below). But right now most of them don't and it seems that there's rather some assumption that the location counter will always maintain a somewhat sane alignment, so putting this in here seems like the safest solution for now. If someone later wants to refactor the whole file and figure out the precise alignments every individual section needs, we could take it out again.
_car_stack_start is unlikely to have alignment requirements but _car_stack_end does (well the code aligns stack anyway...). Maybe move it a little lower (in the file, address) where we care little about alignment? e.g. below (in the file) _car_ehci_dbg
This file is due for a cleanup using the region macros.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/5/src/arch/x86/car.ld@37 PS5, Line 37: . = ALIGN(32);
The FMAP size isn't necessarily a multiple of 32, so if I don't put this here the location counter […]
Sure, I can put it down with TIMESTAMP()
Hello Aaron Durbin, Arthur Heymans, Patrick Rudolph, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36657
to look at the new patch set (#6).
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added post-RAM FMAP cache in CBMEM. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Add FMAP_CACHEs to all platforms that can afford it (other than the RISC-V things where I have no idea how they work), trying to take the space from things that look like they were oversized anyway (pre-RAM consoles and CBFS caches).
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org --- M Makefile.inc M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 19 files changed, 132 insertions(+), 68 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/36657/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36657/6/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/6/src/include/memlayout.h@79 PS6, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/6/src/include/memlayout.h@79 PS6, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/6/src/include/memlayout.h@82 PS6, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/6/src/include/memlayout.h@82 PS6, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 6: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 6: Code-Review+2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 6: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36657/7/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/7/src/include/memlayout.h@79 PS7, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/7/src/include/memlayout.h@79 PS7, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/7/src/include/memlayout.h@82 PS7, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/7/src/include/memlayout.h@82 PS7, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36657/8/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/8/src/include/memlayout.h@79 PS8, Line 79: #define FMAP_CACHE(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/c/coreboot/+/36657/8/src/include/memlayout.h@79 PS8, Line 79: #define FMAP_CACHE(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/c/coreboot/+/36657/8/src/include/memlayout.h@82 PS8, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/36657/8/src/include/memlayout.h@82 PS8, Line 82: STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); space prohibited after that '!' (ctx:VxW)
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/8/src/soc/qualcomm/ipq40xx/in... File src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/8/src/soc/qualcomm/ipq40xx/in... PS8, Line 37: 92K Should this be 91K?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/8/src/soc/qualcomm/ipq40xx/in... File src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/8/src/soc/qualcomm/ipq40xx/in... PS8, Line 37: 92K
Should this be 91K?
No, there was already 1K of free space here. memlayout would complain if there was an overlap, so since Jenkins gave his +1 we should be good here.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld@36 PS3, Line 36: FMAP_CACHE(., CONFIG_FMAP_CACHE_SIZE)
Decided to change this to just use FMAP_SIZE. […]
Ack
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@35 PS3, Line 35: #define STR(x) XSTR(x) : #define XSTR(x) #x
It makes it so the 'FMAP_SIZE' macro in the error message below will actually be expanded. […]
Ack
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: FMAP_CACHE(addr, sz)
This is sort of a philosophical choice... […]
Ack
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36657/1/src/lib/cbfs.c@301 PS1, Line 301: /* Find location of header using signed 32-bit offset from
Does anyone know why we're still doing all this crap at all? Can I just take the offset and size fro […]
Done
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@35 PS3, Line 35: print_once
Not sure I like this name... […]
Ack
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@64 PS3, Line 64: print_once(BIOS_WARNING, : "WARNING: Post-RAM FMAP access too early for cache!\n");
Yeah, that was what I was trying to express without making the message too long. […]
Done
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@305 PS3, Line 305: const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s);
No, in the cached case fmrd is chained from fmap_cache here, which is the pre-RAM memory buffer. […]
Ack
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
lib/fmap: Add optional pre-RAM cache
This patch adds an optional pre-RAM cache for the FMAP which most platforms should be able to use, complementing the recently added post-RAM FMAP cache in CBMEM. vboot systems currently read the FMAP about half a dozen times from flash in verstage, which will all be coalesced into a single read with this patch. It will also help future vboot improvements since when FMAP reads become "free" vboot doesn't need to keep track of so much information separately.
In order to make sure we have a single, well-defined point where the new cache is first initialized, eliminate the build-time hardcoding of the CBFS section offsets, so that all CBFS accesses explicitly read the FMAP.
Add FMAP_CACHEs to all platforms that can afford it (other than the RISC-V things where I have no idea how they work), trying to take the space from things that look like they were oversized anyway (pre-RAM consoles and CBFS caches).
Change-Id: I2820436776ef620bdc4481b5cd4b6957764248ea Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36657 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Joel Kitching kitching@google.com --- M Makefile.inc M src/arch/x86/car.ld M src/include/memlayout.h M src/include/symbols.h M src/lib/Makefile.inc M src/lib/cbfs.c M src/lib/fmap.c M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sc7180/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld M src/soc/samsung/exynos5420/include/soc/memlayout.ld M util/cbfstool/fmaptool.c 19 files changed, 132 insertions(+), 68 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Arthur Heymans: Looks good to me, approved Joel Kitching: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index 8ecc0ef..8de54a0 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -226,7 +226,7 @@ # Add handler to copy linker scripts define generic-objs_ld_template_gen de$(EMPTY)fine $(1)-objs_ld_template -$$(call src-to-obj,$1,$$(1).ld): $$(1).ld $(obj)/config.h +$$(call src-to-obj,$1,$$(1).ld): $$(1).ld $(obj)/config.h $(obj)/fmap_config.h @printf " CP $$$$(subst $$$$(obj)/,,$$$$(@))\n" $$(CC_$(1)) -MMD $$(CPPFLAGS_$(1)) $$($(1)-ld-ccopts) $(PREPROCESS_ONLY) -include $(obj)/config.h -MT $$$$@ -o $$$$@ $$$$< en$(EMPTY)def diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 74fc74b..972cb52 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -58,6 +58,7 @@ #endif
TIMESTAMP(., 0x200) + FMAP_CACHE(., FMAP_SIZE)
_car_ehci_dbg_info = .; /* Reserve sizeof(struct ehci_dbg_info). */ diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 0100e27..56dfb6a 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -21,6 +21,8 @@ #include <arch/memlayout.h> #include <vb2_constants.h>
+#include "fmap_config.h" + /* Macros that the architecture can override. */ #ifndef ARCH_POINTER_ALIGN_SIZE #define ARCH_POINTER_ALIGN_SIZE 8 @@ -30,7 +32,8 @@ #define ARCH_CACHELINE_ALIGN_SIZE 64 #endif
-#define STR(x) #x +#define STR(x) XSTR(x) +#define XSTR(x) #x
#define ALIGN_COUNTER(align) \ . = ALIGN(align); @@ -73,6 +76,11 @@ ALIAS_REGION(cbfs_cache, preram_cbfs_cache) \ ALIAS_REGION(cbfs_cache, postram_cbfs_cache)
+#define FMAP_CACHE(addr, sz) \ + REGION(fmap_cache, addr, sz, 4) \ + _ = ASSERT(sz == 0 || sz >= FMAP_SIZE, \ + STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE))); + #if ENV_ROMSTAGE_OR_BEFORE #define PRERAM_CBFS_CACHE(addr, size) \ REGION(preram_cbfs_cache, addr, size, 4) \ diff --git a/src/include/symbols.h b/src/include/symbols.h index 56df8d5..eec4701 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -34,6 +34,7 @@ DECLARE_REGION(preram_cbfs_cache) DECLARE_REGION(postram_cbfs_cache) DECLARE_REGION(cbfs_cache) +DECLARE_REGION(fmap_cache) DECLARE_REGION(payload)
/* "program" always refers to the current execution unit. */ diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 3b7d57c..dc0c46d 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -208,13 +208,6 @@ $(call src-to-obj,verstage,$(dir)/version.c) : $(obj)/build.h $(call src-to-obj,postcar,$(dir)/version.c) : $(obj)/build.h
-$(call src-to-obj,bootblock,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,romstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,ramstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,smm,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,verstage,$(dir)/cbfs.c) : $(obj)/fmap_config.h -$(call src-to-obj,postcar,$(dir)/cbfs.c) : $(obj)/fmap_config.h - $(call src-to-obj,bootblock,$(dir)/fmap.c) : $(obj)/fmap_config.h $(call src-to-obj,romstage,$(dir)/fmap.c) : $(obj)/fmap_config.h $(call src-to-obj,ramstage,$(dir)/fmap.c) : $(obj)/fmap_config.h diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 1601f20..27a1592 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -26,7 +26,6 @@ #include <symbols.h> #include <timestamp.h> #include <fmap.h> -#include "fmap_config.h" #include <security/vboot/vboot_crtm.h>
#define ERROR(x...) printk(BIOS_ERR, "CBFS: " x) @@ -310,7 +309,10 @@ if (bdev == NULL) return -1;
- size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE; + struct region fmap_region; + if (fmap_locate_area("COREBOOT", &fmap_region)) + return -1; + size_t fmap_top = region_end(&fmap_region);
/* Find location of header using signed 32-bit offset from * end of CBFS region. */ diff --git a/src/lib/fmap.c b/src/lib/fmap.c index f3ff071..4b4179c 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -15,12 +15,13 @@
#include <arch/early_variables.h> #include <boot_device.h> +#include <cbmem.h> #include <console/console.h> #include <fmap.h> #include <commonlib/fmap_serialized.h> #include <stddef.h> #include <string.h> -#include <cbmem.h> +#include <symbols.h>
#include "fmap_config.h"
@@ -31,26 +32,94 @@ static int fmap_print_once CAR_GLOBAL; static struct mem_region_device fmap_cache CAR_GLOBAL;
+#define print_once(...) do { \ + if (!car_get_var(fmap_print_once)) \ + printk(__VA_ARGS__); \ + } while (0) + +DECLARE_OPTIONAL_REGION(fmap_cache); + uint64_t get_fmap_flash_offset(void) { return FMAP_OFFSET; }
+static int check_signature(const struct fmap *fmap) +{ + return memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature)); +} + +static void report(const struct fmap *fmap) +{ + print_once(BIOS_DEBUG, "FMAP: Found "%s" version %d.%d at %#x.\n", + fmap->name, fmap->ver_major, fmap->ver_minor, FMAP_OFFSET); + print_once(BIOS_DEBUG, "FMAP: base = %#llx size = %#x #areas = %d\n", + (long long)fmap->base, fmap->size, fmap->nareas); + car_set_var(fmap_print_once, 1); +} + +static void setup_preram_cache(struct mem_region_device *cache_mrdev) +{ + if (!ENV_ROMSTAGE_OR_BEFORE) { + /* We get here if ramstage makes an FMAP access before calling + cbmem_initialize(). We should avoid letting it come to that, + so print a warning. */ + print_once(BIOS_WARNING, + "WARNING: Post-RAM FMAP access too early for cache!\n"); + return; + } + + if (REGION_SIZE(fmap_cache) == 0) { + /* If you see this you really want to add an FMAP_CACHE to your + memlayout, unless you absolutely can't affort the 2K. */ + print_once(BIOS_NOTICE, + "NOTE: Running without FMAP_CACHE, should add it!\n"); + return; + } + + struct fmap *fmap = (struct fmap *)_fmap_cache; + if (!ENV_BOOTBLOCK) { + /* NOTE: This assumes that for all platforms running this code, + the bootblock is the first stage and the bootblock will make + at least one FMAP access (usually from finding CBFS). */ + if (!check_signature(fmap)) + goto register_cache; + + printk(BIOS_ERR, "ERROR: FMAP cache corrupted?!\n"); + } + + /* In case we fail below, make sure the cache is invalid. */ + memset(fmap->signature, 0, sizeof(fmap->signature)); + + boot_device_init(); + const struct region_device *boot_rdev = boot_device_ro(); + if (!boot_rdev) + return; + + /* memlayout statically guarantees that the FMAP_CACHE is big enough. */ + if (rdev_readat(boot_rdev, fmap, FMAP_OFFSET, FMAP_SIZE) != FMAP_SIZE) + return; + if (check_signature(fmap)) + return; + report(fmap); + +register_cache: + mem_region_device_ro_init(cache_mrdev, fmap, FMAP_SIZE); +} + static int find_fmap_directory(struct region_device *fmrd) { const struct region_device *boot; struct fmap *fmap; - size_t fmap_size; + struct mem_region_device *cache; size_t offset = FMAP_OFFSET;
- if (cbmem_possibly_online() && !ENV_SMM) { - /* Try FMAP cache first */ - const struct mem_region_device *cache; - - cache = car_get_var_ptr(&fmap_cache); - if (region_device_sz(&cache->rdev)) - return rdev_chain_full(fmrd, &cache->rdev); - } + /* Try FMAP cache first */ + cache = car_get_var_ptr(&fmap_cache); + if (!region_device_sz(&cache->rdev)) + setup_preram_cache(cache); + if (region_device_sz(&cache->rdev)) + return rdev_chain_full(fmrd, &cache->rdev);
boot_device_init(); boot = boot_device_ro(); @@ -58,32 +127,22 @@ if (boot == NULL) return -1;
- fmap_size = sizeof(struct fmap); - - fmap = rdev_mmap(boot, offset, fmap_size); + fmap = rdev_mmap(boot, offset, sizeof(struct fmap));
if (fmap == NULL) return -1;
- if (memcmp(fmap->signature, FMAP_SIGNATURE, sizeof(fmap->signature))) { + if (check_signature(fmap)) { printk(BIOS_DEBUG, "No FMAP found at %zx offset.\n", offset); rdev_munmap(boot, fmap); return -1; }
- if (!car_get_var(fmap_print_once)) { - printk(BIOS_DEBUG, "FMAP: Found "%s" version %d.%d at %zx.\n", - fmap->name, fmap->ver_major, fmap->ver_minor, offset); - printk(BIOS_DEBUG, "FMAP: base = %llx size = %x #areas = %d\n", - (long long)fmap->base, fmap->size, fmap->nareas); - car_set_var(fmap_print_once, 1); - } - - fmap_size += fmap->nareas * sizeof(struct fmap_area); + report(fmap);
rdev_munmap(boot, fmap);
- return rdev_chain(fmrd, boot, offset, fmap_size); + return rdev_chain(fmrd, boot, offset, FMAP_SIZE); }
int fmap_locate_area_as_rdev(const char *name, struct region_device *area) @@ -212,7 +271,7 @@ return rdev_writeat(&rdev, buffer, 0, size); }
-static void fmap_register_cache(int unused) +static void fmap_register_cbmem_cache(int unused) { const struct cbmem_entry *e; struct mem_region_device *mdev; @@ -232,7 +291,7 @@ * The main reason to copy the FMAP into CBMEM is to make it available to the * OS on every architecture. As side effect use the CBMEM copy as cache. */ -static void fmap_setup_cache(int unused) +static void fmap_setup_cbmem_cache(int unused) { struct region_device fmrd;
@@ -255,9 +314,9 @@ }
/* Finally advertise the cache for the current stage */ - fmap_register_cache(unused); + fmap_register_cbmem_cache(unused); }
-ROMSTAGE_CBMEM_INIT_HOOK(fmap_setup_cache) -RAMSTAGE_CBMEM_INIT_HOOK(fmap_register_cache) -POSTCAR_CBMEM_INIT_HOOK(fmap_register_cache) +ROMSTAGE_CBMEM_INIT_HOOK(fmap_setup_cbmem_cache) +RAMSTAGE_CBMEM_INIT_HOOK(fmap_register_cbmem_cache) +POSTCAR_CBMEM_INIT_HOOK(fmap_register_cbmem_cache) diff --git a/src/soc/mediatek/mt8173/include/soc/memlayout.ld b/src/soc/mediatek/mt8173/include/soc/memlayout.ld index adda86e..2358c39 100644 --- a/src/soc/mediatek/mt8173/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8173/include/soc/memlayout.ld @@ -40,7 +40,8 @@ SRAM_START(0x00100000) VBOOT2_WORK(0x00100000, 12K) VBOOT2_TPM_LOG(0x00103000, 2K) - PRERAM_CBMEM_CONSOLE(0x00103800, 14K) + FMAP_CACHE(0x00103800, 2K) + PRERAM_CBMEM_CONSOLE(0x00104000, 12K) WATCHDOG_TOMBSTONE(0x00107000, 4) PRERAM_CBFS_CACHE(0x00107004, 16K - 4) TIMESTAMP(0x0010B000, 4K) diff --git a/src/soc/mediatek/mt8183/include/soc/memlayout.ld b/src/soc/mediatek/mt8183/include/soc/memlayout.ld index 82e404f..a8f464a 100644 --- a/src/soc/mediatek/mt8183/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8183/include/soc/memlayout.ld @@ -34,7 +34,8 @@ VBOOT2_TPM_LOG(0x00103000, 2K) PRERAM_CBMEM_CONSOLE(0x00103800, 14K) WATCHDOG_TOMBSTONE(0x00107000, 4) - PRERAM_CBFS_CACHE(0x00107004, 48K - 4) + PRERAM_CBFS_CACHE(0x00107004, 46K - 4) + FMAP_CACHE(0x00112800, 2K) TIMESTAMP(0x00113000, 4K) STACK(0x00114000, 16K) TTB(0x00118000, 28K) diff --git a/src/soc/nvidia/tegra124/include/soc/memlayout.ld b/src/soc/nvidia/tegra124/include/soc/memlayout.ld index 7e2f9ec..7e2cc7a 100644 --- a/src/soc/nvidia/tegra124/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra124/include/soc/memlayout.ld @@ -28,7 +28,8 @@ SRAM_START(0x40000000) TTB(0x40000000, 16K + 32) PRERAM_CBMEM_CONSOLE(0x40004020, 6K - 32) - PRERAM_CBFS_CACHE(0x40005800, 16K) + FMAP_CACHE(0x40005800, 2K) + PRERAM_CBFS_CACHE(0x40006000, 14K) VBOOT2_WORK(0x40009800, 12K) VBOOT2_TPM_LOG(0x4000D800, 2K) STACK(0x4000E000, 8K) diff --git a/src/soc/nvidia/tegra210/include/soc/memlayout.ld b/src/soc/nvidia/tegra210/include/soc/memlayout.ld index 1134da6..b7268d1 100644 --- a/src/soc/nvidia/tegra210/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra210/include/soc/memlayout.ld @@ -29,7 +29,8 @@ { SRAM_START(0x40000000) PRERAM_CBMEM_CONSOLE(0x40000000, 2K) - PRERAM_CBFS_CACHE(0x40000800, 30K) + FMAP_CACHE(0x40000800, 2K) + PRERAM_CBFS_CACHE(0x40001000, 28K) VBOOT2_WORK(0x40008000, 12K) VBOOT2_TPM_LOG(0x4000B000, 2K) #if ENV_ARM64 diff --git a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld index f1a7bc5..6ff1018 100644 --- a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld @@ -34,7 +34,8 @@ /* This includes bootblock image, can be reused after bootblock starts */ /* UBER_SBL(0x0A0C0000, 48K) */
- PRERAM_CBFS_CACHE(0x0A0C0000, 93K) + PRERAM_CBFS_CACHE(0x0A0C0000, 92K) + FMAP_CACHE(0x0A0EF800, 2K)
TTB(0x0A0F0000, 16K) TTB_SUBTABLES(0x0A0F4000, 4K) diff --git a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld index 25db175..595d939 100644 --- a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld @@ -38,7 +38,8 @@ QCA_SHARED_RAM(2A03F000, 4K) */ STACK(0x2A040000, 16K) - PRERAM_CBFS_CACHE(0x2A044000, 93K) + PRERAM_CBFS_CACHE(0x2A044000, 91K) + FMAP_CACHE(0x2A05B000, 2K) TTB_SUBTABLES(0x2A05B800, 2K) TTB(0x2A05C000, 16K) SRAM_END(0x2A060000) diff --git a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld index 68642d6..dd013b5 100644 --- a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld +++ b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld @@ -39,7 +39,8 @@ TIMESTAMP(0x8C4F000, 1K) PRERAM_CBMEM_CONSOLE(0x8C4F400, 32K) PRERAM_CBFS_CACHE(0x8C57400, 70K) - REGION(bsram_unused, 0x8C68C00, 0xA2400, 0x100) + FMAP_CACHE(0x8C68C00, 2K) + REGION(bsram_unused, 0x8C69400, 0xA1C00, 0x100) BSRAM_END(0x8D80000)
DRAM_START(0x80000000) diff --git a/src/soc/qualcomm/sc7180/include/soc/memlayout.ld b/src/soc/qualcomm/sc7180/include/soc/memlayout.ld index b2ee3b2..3f43419 100644 --- a/src/soc/qualcomm/sc7180/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sc7180/include/soc/memlayout.ld @@ -43,6 +43,7 @@ REGION(ddr_training, 0x14850000, 8K, 4K) REGION(qclib_serial_log, 0x14852000, 4K, 4K) REGION(ddr_information, 0x14853000, 1K, 1K) + FMAP_CACHE(0x14853400, 2K) REGION(dcb, 0x14870000, 16K, 4K) REGION(pmic, 0x14874000, 44K, 4K) REGION(limits_cfg, 0x1487F000, 4K, 4K) diff --git a/src/soc/rockchip/rk3399/include/soc/memlayout.ld b/src/soc/rockchip/rk3399/include/soc/memlayout.ld index 293057a..4e46e2d 100644 --- a/src/soc/rockchip/rk3399/include/soc/memlayout.ld +++ b/src/soc/rockchip/rk3399/include/soc/memlayout.ld @@ -33,7 +33,8 @@ #if ENV_RAMSTAGE REGION(bl31_sram, 0xFF8C0000, 64K, 1) #else - PRERAM_CBFS_CACHE(0xFF8C0000, 7K) + PRERAM_CBFS_CACHE(0xFF8C0000, 5K) + FMAP_CACHE(0xFF8C1400, 2K) TIMESTAMP(0xFF8C1C00, 1K) /* 0xFF8C2004 is the entry point address the masked ROM will jump to. */ OVERLAP_DECOMPRESSOR_VERSTAGE_ROMSTAGE(0xFF8C2004, 88K - 4) diff --git a/src/soc/samsung/exynos5250/include/soc/memlayout.ld b/src/soc/samsung/exynos5250/include/soc/memlayout.ld index 0bd319e..7e052f0 100644 --- a/src/soc/samsung/exynos5250/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5250/include/soc/memlayout.ld @@ -31,7 +31,8 @@ ROMSTAGE(0x2030000, 128K) /* 32K hole */ TTB(0x2058000, 16K) - PRERAM_CBFS_CACHE(0x205C000, 78K) + PRERAM_CBFS_CACHE(0x205C000, 76K) + FMAP_CACHE(0x206F000, 2K) VBOOT2_TPM_LOG(0x206F800, 2K) VBOOT2_WORK(0x2070000, 12K) STACK(0x2074000, 16K) diff --git a/src/soc/samsung/exynos5420/include/soc/memlayout.ld b/src/soc/samsung/exynos5420/include/soc/memlayout.ld index bc5d066..ff781d2 100644 --- a/src/soc/samsung/exynos5420/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5420/include/soc/memlayout.ld @@ -32,7 +32,8 @@ ROMSTAGE(0x2030000, 128K) /* 32K hole */ TTB(0x2058000, 16K) - PRERAM_CBFS_CACHE(0x205C000, 76K) + PRERAM_CBFS_CACHE(0x205C000, 74K) + FMAP_CACHE(0x206E800, 2K) STACK(0x206F000, 16K) /* 1K hole for weird kernel-shared CPU/SMP state structure that doesn't * seem to be implemented right now? */ diff --git a/util/cbfstool/fmaptool.c b/util/cbfstool/fmaptool.c index faf6508..1c01ec0 100644 --- a/util/cbfstool/fmaptool.c +++ b/util/cbfstool/fmaptool.c @@ -24,6 +24,7 @@ #define STDIN_FILENAME_SENTINEL "-"
#define HEADER_FMAP_OFFSET "FMAP_OFFSET" +#define HEADER_FMAP_SIZE "FMAP_SIZE"
enum fmaptool_return { FMAPTOOL_EXIT_SUCCESS = 0, @@ -83,7 +84,8 @@ }
static bool write_header(const char *out_fname, - const struct flashmap_descriptor *root) + const struct flashmap_descriptor *root, + const int fmap_size) { assert(out_fname);
@@ -100,21 +102,8 @@
fputs("#ifndef FMAPTOOL_GENERATED_HEADER_H_\n", header); fputs("#define FMAPTOOL_GENERATED_HEADER_H_\n\n", header); - fprintf(header, "#define %s %#x\n\n", HEADER_FMAP_OFFSET, fmap_offset); - - /* also add defines for each CBFS-carrying fmap region: base and size */ - cbfs_section_iterator_t cbfs_it = cbfs_sections_iterator(); - while (cbfs_it) { - const struct flashmap_descriptor *item = - cbfs_sections_iterator_deref(cbfs_it); - assert(item->offset_known && item->size_known); - unsigned abs_base = fmd_calc_absolute_offset(root, item->name); - fprintf(header, "#define ___FMAP__%s_BASE 0x%x\n", - item->name, abs_base); - fprintf(header, "#define ___FMAP__%s_SIZE 0x%x\n", - item->name, item->size); - cbfs_sections_iterator_advance(&cbfs_it); - } + fprintf(header, "#define %s %#x\n", HEADER_FMAP_OFFSET, fmap_offset); + fprintf(header, "#define %s %#x\n\n", HEADER_FMAP_SIZE, fmap_size);
fputs("#endif\n", header);
@@ -245,7 +234,7 @@ fmap_destroy(flashmap);
if (args.header_filename && - !write_header(args.header_filename, descriptor)) { + !write_header(args.header_filename, descriptor, size)) { full_fmd_cleanup(&descriptor); return FMAPTOOL_EXIT_FAILED_WRITING_HEADER; }
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
Since this commit I'm getting a new "ERROR: FMAP cache corrupted?!" message on AMD f15h and f16h AGESA boards (this message itself also got added with this commit), which luckily doesn't seem to affect anything. If you would like, I could test your newer FMAP changes to see if this could be fixed.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
Patch Set 9:
Since this commit I'm getting a new "ERROR: FMAP cache corrupted?!" message on AMD f15h and f16h AGESA boards (this message itself also got added with this commit), which luckily doesn't seem to affect anything. If you would like, I could test your newer FMAP changes to see if this could be fixed.
This code assumes C_ENVIRONMENT_BOOTBLOCK. It's easy to fix. OTOH platforms without it will be dropped after the 4.11 release.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Since this commit I'm getting a new "ERROR: FMAP cache corrupted?!" message on AMD f15h and f16h AGESA boards (this message itself also got added with this commit), which luckily doesn't seem to affect anything. If you would like, I could test your newer FMAP changes to see if this could be fixed.
This code assumes C_ENVIRONMENT_BOOTBLOCK. It's easy to fix. OTOH platforms without it will be dropped after the 4.11 release.
Are there any C_ENVIRONMENT_BOOTBLOCK examples for AMD AGESA boards?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Since this commit I'm getting a new "ERROR: FMAP cache corrupted?!" message on AMD f15h and f16h AGESA boards (this message itself also got added with this commit), which luckily doesn't seem to affect anything. If you would like, I could test your newer FMAP changes to see if this could be fixed.
This code assumes C_ENVIRONMENT_BOOTBLOCK. It's easy to fix. OTOH platforms without it will be dropped after the 4.11 release.
Are there any C_ENVIRONMENT_BOOTBLOCK examples for AMD AGESA boards?
See the patch train ending in https://review.coreboot.org/c/coreboot/+/36915
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36657/9/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/9/src/lib/fmap.c@119 PS9, Line 119: if (!region_device_sz(&cache->rdev)) Suppose car_get_var_ptr returns 0 (e.g. when hitting just the wrong time in CAR migration), this looks for != 0 at address 4-8 (depending on pointer size)? There seem to be issues with FSP 1.0 related to this change.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 9:
(1 comment)
Sorry for the issues and thanks to Arthur for taking care of them (CB:36908)!
https://review.coreboot.org/c/coreboot/+/36657/9/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/9/src/lib/fmap.c@119 PS9, Line 119: if (!region_device_sz(&cache->rdev))
Suppose car_get_var_ptr returns 0 (e.g. […]
Hmm... yeah, no, I assumed that the result of car_get_var_ptr() was always valid. It seems that FSP 1.0 has some more weird quirks.
If I understand it correctly there might be a way to solve with with the car_get_ptr()/car_set_ptr() stuff, but it would be complicated because the relative pointer into another CAR region is inside the struct mem_region_device, so we'd have to take the whole use of region_device in here apart and open code it instead. I don't think that's worth it, so how about just disabling pre-RAM caching on those platforms? CB:36937
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Mike Banon (Code Review)
-
Patrick Georgi (Code Review)
-
Yu-Ping Wu (Code Review)