Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Program loading: Handoff cbmem_top via calling arguments
There are a lot of different implementations to pass information from romstage to ramstage. These could all be unified by passing this information via cbmem. Often however these methods exist for that very purpose. This solves this by passing cbmem_top via the programs arguments.
Change-Id: Id2031f7bb81ce65fc318313c270eb1fbae3b2114 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/include/program_loading.h M src/lib/prog_loaders.c 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/36272/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index c6149ab..0a5d50c 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -225,5 +225,7 @@ /* As postcar exist, it's end of romstage here */ timestamp_add_now(TS_END_ROMSTAGE);
+ prog_set_arg(&prog, cbmem_top()); + prog_run(&prog); } diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 6dec192..8257a1e 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -136,6 +136,11 @@ prog->arg = arg; }
+static inline void prog_set_arg(struct prog *prog, void *arg) +{ + prog->arg = arg; +} + /* Locate the identified program to run. Return 0 on success. < 0 on error. */ int prog_locate(struct prog *prog);
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 2ef6bdf..3d1c4e6 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -90,6 +90,8 @@ /* Load the cached ramstage to runtime location. */ stage_cache_load_stage(STAGE_RAMSTAGE, ramstage);
+ prog_set_arg(ramstage, cbmem_top()); + if (prog_entry(ramstage) != NULL) { printk(BIOS_DEBUG, "Jumping to image.\n"); prog_run(ramstage); @@ -142,6 +144,9 @@
timestamp_add_now(TS_END_COPYRAM);
+ /* This overrides the arg fetched from the relocatable module */ + prog_set_arg(&ramstage, cbmem_top()); + prog_run(&ramstage);
fail:
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... PS1, Line 228: prog_set_arg(&prog, cbmem_top()); where does postcar pick this up?
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c@147 PS1, Line 147: /* This overrides the arg fetched from the relocatable module */ You should indicate that it doesn't matter. But you could also do what you want by having a ramstage module parameters section for x86 and just setting the parameter in load_relocatable_ramstage(). Hrmm. Looks like we still have some random x86 platforms that don't use relocatable ramstage. Ugh.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... PS1, Line 228: prog_set_arg(&prog, cbmem_top());
where does postcar pick this up?
CB:36274 exit_car.S
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c@147 PS1, Line 147: /* This overrides the arg fetched from the relocatable module */
You should indicate that it doesn't matter. But you could also do what you want by having a ramstage module parameters section for x86 and just setting the parameter in load_relocatable_ramstage(). Hrmm. Looks like we still have some random x86 platforms that don't use relocatable ramstage. Ugh.
I opted for passing cbmem_top via calling args as it should be possible to have a single cbmem_top implementation for all architectures regardless of relocatable ramstage.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 2:
How does SMM reach cbmem_top()? If I am not mistaken ASEG SMM (AGESA,PI,QEMU-Q35) is not rmodule either, in case that matters.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 2:
Patch Set 2:
How does SMM reach cbmem_top()? If I am not mistaken ASEG SMM (AGESA,PI,QEMU-Q35) is not rmodule either, in case that matters.
Hmm. I guess the answer is SMM does not have to, and it did not reach it before either.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
How does SMM reach cbmem_top()? If I am not mistaken ASEG SMM (AGESA,PI,QEMU-Q35) is not rmodule either, in case that matters.
Hmm. I guess the answer is SMM does not have to, and it did not reach it before either.
Looks like the residual smihandler does not need it. It should be rather easy to provide an implementation however, via an apmc call.
Btw with parallel_mp the smm relocation code calls in ramstage code. which knows where cbmem is, which in turn also allows it to print to the cbmem console.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
How does SMM reach cbmem_top()? If I am not mistaken ASEG SMM (AGESA,PI,QEMU-Q35) is not rmodule either, in case that matters.
Hmm. I guess the answer is SMM does not have to, and it did not reach it before either.
Looks like the residual smihandler does not need it. It should be rather easy to provide an implementation however, via an apmc call.
Btw with parallel_mp the smm relocation code calls in ramstage code. which knows where cbmem is, which in turn also allows it to print to the cbmem console.
Currently, even with DEBUG_SMI=y, SMM would not output to CBMEM console. I was just a bit surprised to notice SMI did not have CBMEM access at all.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36272
to look at the new patch set (#6).
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Program loading: Handoff cbmem_top via calling arguments
There are a lot of different implementations to pass information from romstage to ramstage. These could all be unified by passing this information via cbmem. Often however these methods exist for that very purpose. This solves this by passing cbmem_top via the programs arguments.
Change-Id: Id2031f7bb81ce65fc318313c270eb1fbae3b2114 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/include/program_loading.h M src/lib/prog_loaders.c 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/36272/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 6: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/arch/x86/postcar_loader... PS1, Line 228: prog_set_arg(&prog, cbmem_top());
where does postcar pick this up? […]
Done
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36272/1/src/lib/prog_loaders.c@147 PS1, Line 147: /* This overrides the arg fetched from the relocatable module */
You should indicate that it doesn't matter. […]
Done
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36272
to look at the new patch set (#7).
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Program loading: Handoff cbmem_top via calling arguments
There are a lot of different implementations to pass information from romstage to ramstage. These could all be unified by passing this information via cbmem. Often however these methods exist for that very purpose. This solves this by passing cbmem_top via the programs arguments.
Change-Id: Id2031f7bb81ce65fc318313c270eb1fbae3b2114 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/include/program_loading.h M src/lib/prog_loaders.c 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/36272/7
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36272 )
Change subject: Program loading: Handoff cbmem_top via calling arguments ......................................................................
Program loading: Handoff cbmem_top via calling arguments
There are a lot of different implementations to pass information from romstage to ramstage. These could all be unified by passing this information via cbmem. Often however these methods exist for that very purpose. This solves this by passing cbmem_top via the programs arguments.
Change-Id: Id2031f7bb81ce65fc318313c270eb1fbae3b2114 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36272 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/postcar_loader.c M src/include/program_loading.h M src/lib/prog_loaders.c 3 files changed, 12 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index c6149ab..0a5d50c 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -225,5 +225,7 @@ /* As postcar exist, it's end of romstage here */ timestamp_add_now(TS_END_ROMSTAGE);
+ prog_set_arg(&prog, cbmem_top()); + prog_run(&prog); } diff --git a/src/include/program_loading.h b/src/include/program_loading.h index 601847d..1b71fad 100644 --- a/src/include/program_loading.h +++ b/src/include/program_loading.h @@ -137,6 +137,11 @@ prog->arg = arg; }
+static inline void prog_set_arg(struct prog *prog, void *arg) +{ + prog->arg = arg; +} + /* Locate the identified program to run. Return 0 on success. < 0 on error. */ int prog_locate(struct prog *prog); /* The prog_locate_hook() is called prior to CBFS traversal. The hook can be diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 5048c99..183a22b 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -96,6 +96,8 @@ /* Load the cached ramstage to runtime location. */ stage_cache_load_stage(STAGE_RAMSTAGE, ramstage);
+ prog_set_arg(ramstage, cbmem_top()); + if (prog_entry(ramstage) != NULL) { printk(BIOS_DEBUG, "Jumping to image.\n"); prog_run(ramstage); @@ -148,6 +150,9 @@
timestamp_add_now(TS_END_COPYRAM);
+ /* This overrides the arg fetched from the relocatable module */ + prog_set_arg(&ramstage, cbmem_top()); + prog_run(&ramstage);
fail:
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)