Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/84039?usp=email )
Change subject: cbmem.h: Change return type of cbmem_get_region ......................................................................
cbmem.h: Change return type of cbmem_get_region
The underlying IMD function already returns an integer which indicates success or failure.
This removes the need to have initialized variables that need to be checked for NULL later. In some cases this actually adds the appropriate check for returned values.
Dying is appropriate if cbmem is not found as it is essential to the bootflow.
Change-Id: Ib3e09a75380faf9f533601368993261f042422ef Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/84039 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/postcar_loader.c M src/include/cbmem.h M src/lib/fit_payload.c M src/lib/imd_cbmem.c M src/security/memory/memory_clear.c M src/soc/intel/xeon_sp/memmap.c M tests/lib/Makefile.mk 7 files changed, 23 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 2d7bb48..aac4af8 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -147,7 +147,8 @@ uintptr_t stage_cache_base; size_t stage_cache_size;
- cbmem_get_region((void **)&cbmem_base, &cbmem_size); + if (cbmem_get_region((void **)&cbmem_base, &cbmem_size)) + die("Could not find cbmem region"); prog_segment_loaded(cbmem_base, cbmem_size, SEG_FINAL); if (CONFIG(TSEG_STAGE_CACHE) && !romstage_handoff_is_resume()) { stage_cache_external_region((void **)&stage_cache_base, &stage_cache_size); diff --git a/src/include/cbmem.h b/src/include/cbmem.h index de305ae..fd258cd 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -102,7 +102,7 @@ /* Add the cbmem memory used to the memory map at boot. */ void cbmem_add_bootmem(void); /* Return the cbmem memory used */ -void cbmem_get_region(void **baseptr, size_t *size); +int cbmem_get_region(void **baseptr, size_t *size); void cbmem_list(void); void cbmem_add_records_to_cbtable(struct lb_header *header);
diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index 21bc4e8..aea1ba7 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -110,8 +110,8 @@ { u32 addr_cells = 1, size_cells = 1; u64 reg_addrs[2], reg_sizes[2]; - void *baseptr = NULL; - size_t size = 0; + void *baseptr; + size_t size;
static const char *firmware_path[] = {"firmware", NULL}; struct device_tree_node *firmware_node = dt_find_node(tree->root, @@ -140,8 +140,7 @@
/* Second is the CBMEM area (which usually includes the coreboot table). */ - cbmem_get_region(&baseptr, &size); - if (!baseptr || size == 0) { + if (cbmem_get_region(&baseptr, &size)) { printk(BIOS_WARNING, "FIT: CBMEM pointer/size not found!\n"); return; } diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index a88cf98..2fc54bf 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -193,16 +193,19 @@
void cbmem_add_bootmem(void) { - void *baseptr = NULL; - size_t size = 0; + void *baseptr; + size_t size;
- cbmem_get_region(&baseptr, &size); + if (cbmem_get_region(&baseptr, &size)) { + printk(BIOS_ERR, "CBMEM pointer/size not found!\n"); + return; + } bootmem_add_range((uintptr_t)baseptr, size, BM_MEM_TABLE); }
-void cbmem_get_region(void **baseptr, size_t *size) +int cbmem_get_region(void **baseptr, size_t *size) { - imd_region_used(&imd, baseptr, size); + return imd_region_used(&imd, baseptr, size); }
#if ENV_PAYLOAD_LOADER || (CONFIG(EARLY_CBMEM_LIST) && ENV_HAS_CBMEM) diff --git a/src/security/memory/memory_clear.c b/src/security/memory/memory_clear.c index 9af3205..9a8a23a 100644 --- a/src/security/memory/memory_clear.c +++ b/src/security/memory/memory_clear.c @@ -74,12 +74,13 @@ BM_MEM_RAM);
/* Add reserved entries */ - void *baseptr = NULL; - size_t size = 0; + void *baseptr; + size_t size;
/* Only skip CBMEM, stage program, stack and heap are included there. */
- cbmem_get_region(&baseptr, &size); + if (cbmem_get_region(&baseptr, &size)) + die("Could not find cbmem region"); memranges_insert(&mem, (uintptr_t)baseptr, size, BM_MEM_TABLE);
if (ENV_X86) { diff --git a/src/soc/intel/xeon_sp/memmap.c b/src/soc/intel/xeon_sp/memmap.c index bf4c13e..b70c1cc 100644 --- a/src/soc/intel/xeon_sp/memmap.c +++ b/src/soc/intel/xeon_sp/memmap.c @@ -35,7 +35,8 @@ size_t cbmem_size;
/* Try account for the CBMEM region currently used and for future use */ - cbmem_get_region((void **)&cbmem_base, &cbmem_size); + if (cbmem_get_region((void **)&cbmem_base, &cbmem_size)) + die("Could not find cbmem region"); printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); printk(BIOS_DEBUG, "cbmem base_ptr: 0x%lx, size: 0x%zx\n", cbmem_base, cbmem_size); /* Assume 4MiB will be enough for future cbmem objects (FSP-S, ramstage, ...) */ diff --git a/tests/lib/Makefile.mk b/tests/lib/Makefile.mk index 7fc5471..1a96c7d 100644 --- a/tests/lib/Makefile.mk +++ b/tests/lib/Makefile.mk @@ -55,6 +55,7 @@ imd-test-srcs += tests/lib/imd-test.c imd-test-srcs += tests/stubs/console.c imd-test-srcs += src/lib/imd.c +imd-test-srcs += tests/stubs/die.c
timestamp-test-srcs += tests/lib/timestamp-test.c timestamp-test-srcs += tests/stubs/timestamp.c @@ -85,12 +86,14 @@ imd_cbmem-ramstage-test-srcs += tests/stubs/console.c imd_cbmem-ramstage-test-srcs += src/lib/imd.c imd_cbmem-ramstage-test-mocks += cbmem_top_chipset +imd_cbmem-ramstage-test-srcs += tests/stubs/die.c
imd_cbmem-romstage-test-stage := romstage imd_cbmem-romstage-test-srcs += tests/lib/imd_cbmem-test.c imd_cbmem-romstage-test-srcs += tests/stubs/console.c imd_cbmem-romstage-test-srcs += src/lib/imd.c imd_cbmem-romstage-test-mocks += cbmem_top_chipset +imd_cbmem-romstage-test-srcs += tests/stubs/die.c
region_file-test-srcs += tests/lib/region_file-test.c region_file-test-srcs += src/commonlib/region.c
-
Felix Held (Code Review)