Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46483
to review the following change.
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
program_loading: Split source rdev and loaded memory region concepts
In the current struct prog API, prog_rdev() can refer to either the program binary on the storage media or the loaded program in memory, depending on at what time it was called. This context-dependent double meaning is a bit confusing, and it also makes it impossible to define the memory area the program should be loaded into before actually loading it (which will become necessary with the new stage header format). Therefore this patch splits the two so that the rdev will always only refer to the program binary on storage, and a new start pointer and size variable will represent the program in memory.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: If7c0f1c5698fa0c326e23c553ea0fe928b25d202 --- M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/program_loading.h M src/soc/amd/common/block/pi/refcode_loader.c 4 files changed, 29 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46483/1
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index 0b6540e..3c745d4 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -200,7 +200,10 @@ if (s3wake && !CONFIG(NO_STAGE_CACHE)) { printk(BIOS_DEBUG, "Loading FSPS from stage_cache\n"); stage_cache_load_stage(STAGE_REFCODE, fsps); - if (fsp_validate_component(&fsps_hdr, prog_rdev(fsps)) != CB_SUCCESS) + + struct region_device mem_rdev; + prog_chain_mem_rdev(fsps, &mem_rdev); + if (fsp_validate_component(&fsps_hdr, &mem_rdev) != CB_SUCCESS) die("On resume fsps header is invalid\n"); load_done = 1; return; diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c index acc3f4b..62bcc5c 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -177,7 +177,7 @@ uint32_t compression_algo; size_t output_size; void *dest; - struct region_device source_rdev; + struct region_device source_rdev, mem_rdev; struct prog *fsp_prog = &fspld->fsp_prog;
if (fspld->get_destination == NULL) @@ -198,7 +198,8 @@
prog_set_area(fsp_prog, dest, output_size);
- if (fsp_validate_component(hdr, prog_rdev(fsp_prog)) != CB_SUCCESS) { + prog_chain_mem_rdev(fsp_prog, &mem_rdev); + if (fsp_validate_component(hdr, &mem_rdev) != CB_SUCCESS) { printk(BIOS_ERR, "Invalid FSP header after load!\n"); return CB_ERR; } diff --git a/src/include/program_loading.h b/src/include/program_loading.h index e5a2931..e26792e 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -2,6 +2,7 @@ #ifndef PROGRAM_LOADING_H #define PROGRAM_LOADING_H
+#include <assert.h> #include <bootmem.h> #include <commonlib/region.h> #include <types.h> @@ -39,18 +40,14 @@
/* Representation of a program. */ struct prog { - /* The region_device is the source of program content to load. After - * loading program it represents the memory region of the stages and - * payload. For architectures that use a bounce buffer - * then it would represent the bounce buffer. */ enum prog_type type; uint32_t cbfs_type; const char *name; - struct region_device rdev; - /* Entry to program with optional argument. It's up to the architecture - * to decide if argument is passed. */ - void (*entry)(void *); - void *arg; + struct region_device load_rdev; /* Source for loading (e.g. flash). */ + void *start; /* Program start in memory after loading. */ + size_t size; /* Program size in memory (including BSS). */ + void (*entry)(void *); /* Function pointer to entry point. */ + void *arg; /* Optional argument (only valid for some archs). */ };
#define PROG_INIT(type_, name_) \ @@ -76,19 +73,19 @@
static inline struct region_device *prog_rdev(struct prog *prog) { - return &prog->rdev; + return &prog->load_rdev; }
-/* Only valid for loaded programs. */ static inline size_t prog_size(const struct prog *prog) { - return region_device_sz(&prog->rdev); + assert(prog->size); + return prog->size; }
-/* Only valid for loaded programs. */ static inline void *prog_start(const struct prog *prog) { - return rdev_mmap_full(&prog->rdev); + assert(prog->size); + return prog->start; }
static inline void *prog_entry(const struct prog *prog) @@ -104,15 +101,19 @@ /* region_device representing the 32-bit flat address space. */ extern const struct mem_region_device addrspace_32bit;
-static inline void prog_memory_init(struct prog *prog, uintptr_t ptr, - size_t size) +/* Can be used to get an rdev representation of program area in memory. */ +static inline void prog_chain_mem_rdev(const struct prog *prog, + struct region_device *rdev_out) { - rdev_chain(&prog->rdev, &addrspace_32bit.rdev, ptr, size); + assert(prog->size); + rdev_chain(rdev_out, &addrspace_32bit.rdev, + (uintptr_t)prog->start, prog->size); }
static inline void prog_set_area(struct prog *prog, void *start, size_t size) { - prog_memory_init(prog, (uintptr_t)start, size); + prog->start = start; + prog->size = size; }
static inline void prog_set_entry(struct prog *prog, void *e, void *arg) diff --git a/src/soc/amd/common/block/pi/refcode_loader.c b/src/soc/amd/common/block/pi/refcode_loader.c index 2fdbe83..bedd847 100644 --- a/src/soc/amd/common/block/pi/refcode_loader.c +++ b/src/soc/amd/common/block/pi/refcode_loader.c @@ -65,8 +65,9 @@ } }
- return rdev_chain(rdev, prog_rdev(&prog), 0, - region_device_sz(prog_rdev(&prog))); + prog_chain_mem_rdev(&prog, rdev); + + return 0; }
static int agesa_locate_stage_file(const char *name, struct region_device *rdev)