Change in coreboot[master]: program_loading: Split source rdev and loaded memory region concepts
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)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/1/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/1/src/include/program_loading... PS1, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Andrey Petrov, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46483
to look at the new patch set (#2).
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/cpu/x86/mtrr/xip_cache.c 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 5 files changed, 31 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46483/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/2/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/2/src/include/program_loading... PS2, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/3/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/3/src/include/program_loading... PS3, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 3: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/4/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/4/src/include/program_loading... PS4, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/5/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/5/src/include/program_loading... PS5, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/6/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/6/src/include/program_loading... PS6, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/7/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/7/src/include/program_loading... PS7, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/9/src/include/program_loading... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/9/src/include/program_loading... PS9, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Split source rdev and loaded memory region concepts ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46483/10/src/include/program_loadin... File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/10/src/include/program_loadin... PS10, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Furquan Shaikh. Hello build bot (Jenkins), Jason Glenesk, Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46483
to look at the new patch set (#11).
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
program_loading: Replace prog_rdev() with raw start pointer and size
Since prog_locate() was eliminated, prog_rdev() only ever represents the loaded program in memory now. Using the rdev API for this is unnecessary if we know that the "device" is always just memory. This patch changes it to be represented by a simple pointer and size. Since some code still really wants this to be an rdev, introduce a prog_chain_rdev() helper to translate back to that if necessary.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: If7c0f1c5698fa0c326e23c553ea0fe928b25d202 --- M src/cpu/x86/mtrr/xip_cache.c 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 5 files changed, 25 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46483/11
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 11:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/b277924f_2d7c813c PS11, Line 48: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Furquan Shaikh. Hello build bot (Jenkins), Jason Glenesk, Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46483
to look at the new patch set (#12).
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
program_loading: Replace prog_rdev() with raw start pointer and size
Since prog_locate() was eliminated, prog_rdev() only ever represents the loaded program in memory now. Using the rdev API for this is unnecessary if we know that the "device" is always just memory. This patch changes it to be represented by a simple pointer and size. Since some code still really wants this to be an rdev, introduce a prog_chain_rdev() helper to translate back to that if necessary.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: If7c0f1c5698fa0c326e23c553ea0fe928b25d202 --- M src/cpu/x86/mtrr/xip_cache.c 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 5 files changed, 25 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46483/12
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 12:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/2497e74f_598a804f PS12, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 13:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/08de332b_de439d85 PS13, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 14:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/68516c49_39f306f7 PS14, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Furquan Shaikh, Julius Werner. Julius Werner has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Removed Verified-1 by build bot (Jenkins) no-reply@coreboot.org
Attention is currently required from: Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 16:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/79244ee7_0d35b7cf PS16, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 17:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/da680fa2_e6f89567 PS17, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh, Julius Werner. Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 17: Code-Review+2
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 18:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/6c4d316f_3c1bc246 PS18, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 19:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/e966e383_00f1f2a5 PS19, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 21:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/73617ff6_b0d6795c PS21, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 23:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/1ab0b940_f54ad45c PS23, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 24:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/03efa972_3409b164 PS24, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 25:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/071df795_0128bb23 PS25, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 26:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/6c05deba_b4998813 PS26, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 27:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/83dc31ba_fc64a0e2 PS27, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Attention is currently required from: Raul Rangel, Furquan Shaikh. Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Marshall Dawson, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46483
to look at the new patch set (#28).
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
program_loading: Replace prog_rdev() with raw start pointer and size
Since prog_locate() was eliminated, prog_rdev() only ever represents the loaded program in memory now. Using the rdev API for this is unnecessary if we know that the "device" is always just memory. This patch changes it to be represented by a simple pointer and size. Since some code still really wants this to be an rdev, introduce a prog_chain_rdev() helper to translate back to that if necessary.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: If7c0f1c5698fa0c326e23c553ea0fe928b25d202 --- M src/cpu/x86/mtrr/xip_cache.c 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 5 files changed, 25 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46483/28
Attention is currently required from: Raul Rangel, Furquan Shaikh. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
Patch Set 28:
(1 comment)
File src/include/program_loading.h:
https://review.coreboot.org/c/coreboot/+/46483/comment/d081d82d_4994766d PS28, Line 49: void (*entry)(void *); /* Function pointer to entry point. */ function definition argument 'void *' should also have an identifier name
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46483 )
Change subject: program_loading: Replace prog_rdev() with raw start pointer and size ......................................................................
program_loading: Replace prog_rdev() with raw start pointer and size
Since prog_locate() was eliminated, prog_rdev() only ever represents the loaded program in memory now. Using the rdev API for this is unnecessary if we know that the "device" is always just memory. This patch changes it to be represented by a simple pointer and size. Since some code still really wants this to be an rdev, introduce a prog_chain_rdev() helper to translate back to that if necessary.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: If7c0f1c5698fa0c326e23c553ea0fe928b25d202 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46483 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/x86/mtrr/xip_cache.c 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 5 files changed, 25 insertions(+), 28 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/cpu/x86/mtrr/xip_cache.c b/src/cpu/x86/mtrr/xip_cache.c index d3fbfcb..cd82e4f 100644 --- a/src/cpu/x86/mtrr/xip_cache.c +++ b/src/cpu/x86/mtrr/xip_cache.c @@ -29,8 +29,8 @@
void platform_prog_run(struct prog *prog) { - const uint32_t base = region_device_offset(&prog->rdev); - const uint32_t size = region_device_sz(&prog->rdev); + const uint32_t base = (uintptr_t)prog_start(prog); + const uint32_t size = prog_size(prog); const uint32_t end = base + size; const uint32_t cache_used = max_cache_used(); /* This will accumulate MTRR's as XIP stages are run. diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index 3975a96..c9898f5 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -210,7 +210,10 @@ if (resume_from_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 prog_rdev; + prog_chain_rdev(fsps, &prog_rdev); + if (fsp_validate_component(&fsps_hdr, &prog_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 24097f8..9831e64 100644 --- a/src/drivers/intel/fsp2_0/util.c +++ b/src/drivers/intel/fsp2_0/util.c @@ -180,7 +180,7 @@ uint32_t compression_algo; size_t output_size; void *dest; - struct region_device source_rdev; + struct region_device source_rdev, prog_rdev; struct prog *fsp_prog = &fspld->fsp_prog;
if (fspld->get_destination == NULL) @@ -201,7 +201,8 @@
prog_set_area(fsp_prog, dest, output_size);
- if (fsp_validate_component(hdr, prog_rdev(fsp_prog)) != CB_SUCCESS) { + prog_chain_rdev(fsp_prog, &prog_rdev); + if (fsp_validate_component(hdr, &prog_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 bb4f6ea..d01eff6 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -41,17 +41,13 @@
/* Representation of a program. */ struct prog { - /* The region_device 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; enum cbfs_type 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; + void *start; /* Program start in memory. */ + 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_) \ @@ -75,21 +71,14 @@ return prog->cbfs_type; }
-static inline struct region_device *prog_rdev(struct prog *prog) -{ - return &prog->rdev; -} - -/* Only valid for loaded programs. */ static inline size_t prog_size(const struct prog *prog) { - return region_device_sz(&prog->rdev); + return prog->size; }
-/* Only valid for loaded programs. */ static inline void *prog_start(const struct prog *prog) { - return rdev_mmap_full(&prog->rdev); + return prog->start; }
static inline void *prog_entry(const struct prog *prog) @@ -105,15 +94,18 @@ /* 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_rdev(const struct prog *prog, + struct region_device *rdev_out) { - rdev_chain(&prog->rdev, &addrspace_32bit.rdev, ptr, 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 1d8db89..fe2df5b 100644 --- a/src/soc/amd/common/block/pi/refcode_loader.c +++ b/src/soc/amd/common/block/pi/refcode_loader.c @@ -57,8 +57,9 @@ stage_cache_add(STAGE_REFCODE, &prog); }
- return rdev_chain(rdev, prog_rdev(&prog), 0, - region_device_sz(prog_rdev(&prog))); + prog_chain_rdev(&prog, rdev); + + return 0; }
static int agesa_locate_stage_file(const char *name, struct region_device *rdev)
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)