Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33859
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Kconfig: Enable RAMPAYLOAD for x86
This patch makes CONFIG_RAMPAYLOAD default enable upon selection of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Without this CL, CONFIG_RAMPAYLOAD is still disable although mainboard has selected CONFIG_HAVE_RAMPAYLOAD.
Change-Id: I40308bbf970a0dbe5f7e2086ed8a7a70c2a3a32c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/33859/1
diff --git a/src/Kconfig b/src/Kconfig index 72d826f..919d257 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -290,7 +290,7 @@
config RAMPAYLOAD bool "Enable coreboot flow without executing ramstage" - default n + default y if ARCH_X86 depends on HAVE_RAMPAYLOAD help If this option is enabled, coreboot flow will skip ramstage
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33859/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/1//COMMIT_MSG@12 PS1, Line 12: disable disabled
Hello ron minnich, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33859
to look at the new patch set (#2).
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Kconfig: Enable RAMPAYLOAD for x86
This patch makes CONFIG_RAMPAYLOAD default enable upon selection of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Without this CL, CONFIG_RAMPAYLOAD is still disabled although mainboard has selected CONFIG_HAVE_RAMPAYLOAD.
Change-Id: I40308bbf970a0dbe5f7e2086ed8a7a70c2a3a32c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/33859/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33859/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/1//COMMIT_MSG@12 PS1, Line 12: disable
disabled
Done
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 2: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Kconfig: Enable RAMPAYLOAD for x86
This patch makes CONFIG_RAMPAYLOAD default enable upon selection of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Without this CL, CONFIG_RAMPAYLOAD is still disabled although mainboard has selected CONFIG_HAVE_RAMPAYLOAD.
Change-Id: I40308bbf970a0dbe5f7e2086ed8a7a70c2a3a32c Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33859 Reviewed-by: ron minnich rminnich@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified ron minnich: Looks good to me, approved
diff --git a/src/Kconfig b/src/Kconfig index 72d826f..919d257 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -290,7 +290,7 @@
config RAMPAYLOAD bool "Enable coreboot flow without executing ramstage" - default n + default y if ARCH_X86 depends on HAVE_RAMPAYLOAD help If this option is enabled, coreboot flow will skip ramstage
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
It's a huge fundamental change you are pushing through with RAMPAYLOAD.
I am not so happy about this being submitted with the little discussion review it saw. Mostly I feel some amount of boilerplate KConfig variables has been added just before release and without documentation. Other people may feel otherwise who have thoroughly reviewed this work...
Why is RAMPAYLOAD=y correct default? Isn't a board still initially developed to go through the standard ramstage, or does HAVE_RAMPAYLOAD=y imply that RAMPAYLOAD=n for a board might not even work?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
Patch Set 3:
It's a huge fundamental change you are pushing through with RAMPAYLOAD.
I guess this feature has been review through during initial patch set and this patch just to fix below mentioned problem.
I am not so happy about this being submitted with the little discussion review it saw. Mostly I feel some amount of boilerplate KConfig variables has been added just before release and without documentation. Other people may feel otherwise who have thoroughly reviewed this work...
Why is RAMPAYLOAD=y correct default? Isn't a board still initially developed to go through the standard ramstage, or does HAVE_RAMPAYLOAD=y imply that RAMPAYLOAD=n for a board might not even work?
does HAVE_RAMPAYLOAD=y imply that RAMPAYLOAD=n for a board might not even work? Right now i'm seeing this case only and this patch to fix this problem. HAVE_RAMPAYLOAD=y should imply RAMPAYLOAD=y so avoid fallback/ramstage even complied and packed into cbfs.
I'm not sure how i have missed this to check when initially I was trying to build coreboot without ramstage. i can remember now, initially i only had RAMPAYLOAD as master kconfig then Nico has added HAVE_RAMPAYLOAD, Right now i'm seeing although mainboard do select HAVE_RAMPAYLOAD but still RAMPAYLOAD=n.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
Well my criticism was mostly about the process, not the commit context.
I would have preferred to see the fundamental change of RAMPAYLOAD to be submitted, with documentation, after the (pending) release. The concept is now submitted scattered over period of several months, and yet there is not a single 'select HAVE_RAMPAYLOAD' line.
So currently in upstream tree none of this concept is even build-tested? Is there published/pending work in gerrit that validates these changes on some level at least?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
Patch Set 3:
Well my criticism was mostly about the process, not the commit context.
I would have preferred to see the fundamental change of RAMPAYLOAD to be submitted, with documentation,
We are planning to write some document for sure. Thanks for feedback.
after the (pending) release. The concept is now submitted scattered over period of several months, and yet there is not a single 'select HAVE_RAMPAYLOAD' line.
Its WIP and hopefully we should see some sample RVP board to select HAVE_RAMPAYLOAD as POC.
So currently in upstream tree none of this concept is even build-tested?
Mainboard to select this feature is not available in upsteam thats true, but i have my POC running.
Is there published/pending work in gerrit that validates these changes on some level at least?
You can refer to below patch as mentioned previously. https://review.coreboot.org/c/coreboot/+/30985
idea is to divide this entire CL into smaller review worthy CL and submit to enable this feature, I think i have started with 75 files in initial CL and now its 63 files.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
So currently in upstream tree none of this concept is even build-tested?
Mainboard to select this feature is not available in upsteam thats true, but i have my POC running.
Looking at CB:30985 (your POC, right) I did not see any build passing abuild with +1. The details from jenkins are long gone, maybe the running POC build was there somewhere.
Your work becomes much more assuring when you say you have proof-of-concept __rebased on current master__ and running. Once you have that, please push it to gerrit in case someone else wants to actually run it, or just to provide some validation for all the code that was already merged.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG@10 PS3, Line 10: of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform. Why? I don't understand the reasoning. This is a policy decision that shouldn't automatically be selected for just because a mainboard selects HAVE_RAMPAYLOAD.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG@10 PS3, Line 10: of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Why? I don't understand the reasoning. […]
Do you mean, mainboard have to select both HAVE_RAMPAYLOAD and RAMPAYLOAD both ?
Other example of HAVE_FSP_GOP is enable to select RUN_FSP_GOP. isn't that the case ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG@10 PS3, Line 10: of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Do you mean, mainboard have to select both HAVE_RAMPAYLOAD and RAMPAYLOAD both ? […]
i mean in case of HAVE_FSP_GOP selection is enough to select RUN_FSP_GOP, mainboard doesn't really need to select both these kconfigs ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG@10 PS3, Line 10: of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
i mean in case of HAVE_FSP_GOP selection is enough to select RUN_FSP_GOP, mainboard doesn't really n […]
I don't think those equivalent for comparison purposes. In this CL you are changing the whole boot flow for a mainboard once HAVE is selected. It would require someone to deselect that option if they didn't want the behavior. But maybe it's fine. I was thinking HAVE_RAMPAYLOAD as a "has support" thing, but it's really more of a 'there is a rampayload'. So I guess it's fine?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33859 )
Change subject: Kconfig: Enable RAMPAYLOAD for x86 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33859/3//COMMIT_MSG@10 PS3, Line 10: of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
I don't think those equivalent for comparison purposes. In this CL you are changing the whole boot flow for a mainboard once HAVE is selected. It would require someone to deselect that option if they didn't want the behavior. But maybe it's fine.
I was thinking HAVE_RAMPAYLOAD as a "has support" thing, but it's really more of a 'there is a rampayload'. So I guess it's fine?
yes, i also think so
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)