Hello ron minnich, Subrata Banik, Matt DeVillier,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33263
to review the following change.
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Kconfig: Guard RAMPAYLOAD
The RAMPAYLOAD symbol added by 7e893a02c0 (Kconfig: Create RAMPAYLOAD kconfig) is shown unconditionally for all x86 systems. It generally creates a lot of confusion to prompt for something that isn't imple- mented or not working. So guard it with another Kconfig that can be selected by platforms that actually support it.
Change-Id: I6d158382d1000b8b40ca1368e2efff0c39884f15 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/Kconfig 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/33263/1
diff --git a/src/Kconfig b/src/Kconfig index d30aa99..49f8e6e 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -277,10 +277,13 @@ The path and filename of the file to use as graphical bootsplash screen. The file format has to be jpg.
+config HAVE_RAMPAYLOAD + bool + config RAMPAYLOAD bool "Enable coreboot flow without executing ramstage" default n - depends on ARCH_X86 + depends on HAVE_RAMPAYLOAD help If this option is enabled, coreboot flow will skip ramstage loading and execution of ramstage to load payload.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1: Code-Review+2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33263/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33263/1/src/Kconfig@286 PS1, Line 286: HAVE_RAMPAYLOAD so far RAMPAYLOAD enabling plan is only for x86 platform hence we might need to have arch dependency ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33263/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33263/1/src/Kconfig@286 PS1, Line 286: HAVE_RAMPAYLOAD
so far RAMPAYLOAD enabling plan is only for x86 platform hence we might need to have arch dependency […]
Isn't it basically the same concept Ron wants to use for RISC V? And even if not, as long as nothing (but maybe x86 in the future) selects HAVE_RAMPAYLOAD, there should be no problem?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33263/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33263/1/src/Kconfig@286 PS1, Line 286: HAVE_RAMPAYLOAD
Isn't it basically the same concept Ron wants to use for RISC V? And […]
yes its same concept but i don't think this config has worked on any arch so far apart from what I'm working on x86. I'm only afraid that if anyone !X86 tries to enable it after it might cause different failure. Unless all arch supports this, i don;t think you can remove arch dependency
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33263/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33263/1/src/Kconfig@286 PS1, Line 286: HAVE_RAMPAYLOAD
I'm only afraid that if anyone !X86 tries to enable it after it might cause different failure.
So you are saying one could accidentally put a `select HAVE_RAMPAYLOAD` into their code? But then one could also argue that ARCH_X86 could be selected by accident, no?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33263/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33263/1/src/Kconfig@286 PS1, Line 286: HAVE_RAMPAYLOAD
I'm only afraid that if anyone !X86 tries to enable it after it might cause different failure.
So you are saying one could accidentally put a `select HAVE_RAMPAYLOAD` into their code?
yes that was the point
But then one could also argue that ARCH_X86 could be selected by accident, no?
:) I'm enabling Rampayload for x86 now. So it shouldn't be the problem on x86.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33263 )
Change subject: Kconfig: Guard RAMPAYLOAD ......................................................................
Kconfig: Guard RAMPAYLOAD
The RAMPAYLOAD symbol added by 7e893a02c0 (Kconfig: Create RAMPAYLOAD kconfig) is shown unconditionally for all x86 systems. It generally creates a lot of confusion to prompt for something that isn't imple- mented or not working. So guard it with another Kconfig that can be selected by platforms that actually support it.
Change-Id: I6d158382d1000b8b40ca1368e2efff0c39884f15 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/33263 Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Matt DeVillier matt.devillier@gmail.com Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/Kconfig 1 file changed, 4 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved HAOUAS Elyes: Looks good to me, approved Matt DeVillier: Looks good to me, but someone else must approve Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/Kconfig b/src/Kconfig index d30aa99..49f8e6e 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -277,10 +277,13 @@ The path and filename of the file to use as graphical bootsplash screen. The file format has to be jpg.
+config HAVE_RAMPAYLOAD + bool + config RAMPAYLOAD bool "Enable coreboot flow without executing ramstage" default n - depends on ARCH_X86 + depends on HAVE_RAMPAYLOAD help If this option is enabled, coreboot flow will skip ramstage loading and execution of ramstage to load payload.
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Kyösti Mälkki (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Subrata Banik (Code Review)