Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33143
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependency in order to build platform with CONFIG_RAMSTAGE enable (without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M toolchain.inc 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/1
diff --git a/Makefile.inc b/Makefile.inc index d4f7597..c0797fe 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -1058,7 +1058,11 @@ FIT_OPTIONS += -q $(FIT_ENTRY) endif
+ifeq ($(CONFIG_RAMPAYLOAD),y) +$(obj)/coreboot.rom: $(obj)/coreboot.pre $(CBFSTOOL) $$(INTERMEDIATE) +else $(obj)/coreboot.rom: $(obj)/coreboot.pre $(objcbfs)/ramstage.elf $(CBFSTOOL) $$(INTERMEDIATE) +endif @printf " CBFS $(subst $(obj)/,,$(@))\n" # The full ROM may be larger than the CBFS part, so create an empty # file (filled with \377 = 0xff) and copy the CBFS image over it. @@ -1128,10 +1132,12 @@ endif # CONFIG_NO_XIP_EARLY_STAGES endif # CONFIG_ARCH_ROMSTAGE_X86_32 / CONFIG_ARCH_ROMSTAGE_X86_64
+ifneq ($(CONFIG_RAMPAYLOAD),y) cbfs-files-y += $(CONFIG_CBFS_PREFIX)/ramstage $(CONFIG_CBFS_PREFIX)/ramstage-file := $(objcbfs)/ramstage.elf $(CONFIG_CBFS_PREFIX)/ramstage-type := stage $(CONFIG_CBFS_PREFIX)/ramstage-compression := $(CBFS_COMPRESS_FLAG) +endif
cbfs-files-$(CONFIG_HAVE_REFCODE_BLOB) += $(CONFIG_CBFS_PREFIX)/refcode $(CONFIG_CBFS_PREFIX)/refcode-file := $(REFCODE_BLOB) diff --git a/src/Kconfig b/src/Kconfig index 4ec6b76..2442a63 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -314,6 +314,7 @@
config RAMPAYLOAD bool "Enable coreboot flow without executing ramstage" + default y if !STAGE_RAMSTAGE default n depends on ARCH_X86 help diff --git a/toolchain.inc b/toolchain.inc index 0486287..9b53d4a 100644 --- a/toolchain.inc +++ b/toolchain.inc @@ -47,7 +47,11 @@ ROMCC=CCC_CC="$(ROMCC_BIN)" $(CC) endif
+ifeq ($(CONFIG_RAMPAYLOAD),y) +COREBOOT_STANDARD_STAGES := decompressor bootblock verstage romstage +else COREBOOT_STANDARD_STAGES := decompressor bootblock verstage romstage ramstage +endif MAP-decompressor := bootblock
ARCHDIR-i386 := x86
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33143
to look at the new patch set (#3).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependency in order to build platform with CONFIG_RAMSTAGE enable (without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M toolchain.inc 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33143/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/33143/3/Makefile.inc@1062 PS3, Line 1062: $(obj)/coreboot.rom: $(obj)/coreboot.pre $(CBFSTOOL) $$(INTERMEDIATE) Why not just introduce a variable that is empty if RAMPAYLOAD is 'y' and set to $(objcbfs)/ramstage.elf is not 'y'?
Then the line would be something like the following w/o duplicating other things:
$(obj)/coreboot.rom: $(obj)/coreboot.pre $(RAMSTAGE) $(CBFSTOOL) $$(INTERMEDIATE)
https://review.coreboot.org/#/c/33143/3/Makefile.inc@1136 PS3, Line 1136: cbfs-files-y += $(CONFIG_CBFS_PREFIX)/ramstage Technically, only this particular line needs to be guarded, fwiw. But you don't have to change it.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y) This shouldn't be necessary. Keeping ramstage in here doesn't hurt anyone if you don't use it. (verstage is also here despite being rarely used.)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
This shouldn't be necessary. Keeping ramstage in here doesn't hurt anyone if you don't use it. […]
without this changes, i'm getting below error when my soc doesn't select ARCH_RAMSTAGE
toolchain.inc:165: *** The toolchain architecture for ramstage is unknown. toolchain.inc:165: *** Check your .config file for CONFIG_ARCH_ramstage_* settings. Stop.
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33143
to look at the new patch set (#4).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependency in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M toolchain.inc 3 files changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33143/3/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/33143/3/Makefile.inc@1062 PS3, Line 1062: $(obj)/coreboot.rom: $(obj)/coreboot.pre $(CBFSTOOL) $$(INTERMEDIATE)
Why not just introduce a variable that is empty if RAMPAYLOAD is 'y' and set to $(objcbfs)/ramstage. […]
Done
https://review.coreboot.org/#/c/33143/3/Makefile.inc@1136 PS3, Line 1136: cbfs-files-y += $(CONFIG_CBFS_PREFIX)/ramstage
Technically, only this particular line needs to be guarded, fwiw. But you don't have to change it.
Ack
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33143/4/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/33143/4/Makefile.inc@1139 PS4, Line 1139: $(CONFIG_CBFS_PREFIX)/ramstage-file := $(RAMSTAGE) Is there existing filtering that doesn't process the cbfs-files-y above? I would expect that to be a problem.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33143/4/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/33143/4/Makefile.inc@1139 PS4, Line 1139: $(CONFIG_CBFS_PREFIX)/ramstage-file := $(RAMSTAGE)
Is there existing filtering that doesn't process the cbfs-files-y above?
Although i don't see any such filtering but you are right, it appears to me that cbfs-files-y above is not getting processed now after i have enabled RAMPAYLOAD check to select ramstage.elf or NULL
I would expect that to be a problem.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33143/4/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/33143/4/Makefile.inc@1139 PS4, Line 1139: $(CONFIG_CBFS_PREFIX)/ramstage-file := $(RAMSTAGE)
Is there existing filtering that doesn't process the cbfs-files-y above? […]
Aaron, do you recommend to apply ifneq ($(CONFIG_RAMPAYLOAD),y) or ifeq ($(CONFIG_ENABLE_RAMSTAGE),y) guard about this code, similar romstage.elf above line 1136 ?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
without this changes, i'm getting below error when my soc doesn't select ARCH_RAMSTAGE […]
That sounds like another good reason to just not touch the toolchain Kconfigs (like ARCH_RAMSTAGE_X86), and deal with this on another level. There's no harm having a toolchain assigned to the ramstage as long as you just don't build it. Doing it that way would require a lot less special casing for this feature.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
That sounds like another good reason to just not touch the toolchain Kconfigs (like ARCH_RAMSTAGE_X8 […]
Today ARCH_RAMSTAGE_X86 is the way to tell if we like to build ramstage or not. At least thats how coreboot soc is structured now.
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33143
to look at the new patch set (#5).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependency in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/Kconfig M toolchain.inc 3 files changed, 19 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
Today ARCH_RAMSTAGE_X86 is the way to tell if we like to build ramstage or not. At least thats how coreboot soc is structured now.
That's not true. That's what you're trying to turn it into and I'm trying to argue to please not do that. ;)
Today ARCH_RAMSTAGE_X86 tells what architecture the ramstage is and nothing else. There is no way to not build a ramstage today. There is a way to not build two other stages (verstage and postcar), but for both of those the option that controls whether the stage is built is completely orthogonal to the option that controls with architecture it is (and the latter is usually still set even if the former is not... e.g. ARCH_VERSTAGE_X86 is still set even if SEPERATE_VERSTAGE=n).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
Today ARCH_RAMSTAGE_X86 is the way to tell if we like to build ramstage or not. At least thats how coreboot soc is structured now.
That's not true. That's what you're trying to turn it into and I'm trying to argue to please not do that. ;)
Today ARCH_RAMSTAGE_X86 tells what architecture the ramstage is and nothing else. There is no way to not build a ramstage today.
You got the right point, here the new requirement is to build coreboot without RAMSTAGE like verstage/postcar :)
Thats the reason we are trying to remove root dependency from ramstage in coreboot code today.
There is a way to not build two other stages (verstage and postcar), but for both of those the option that controls whether the stage is built is completely orthogonal to the option that controls with architecture it is (and the latter is usually still set even if the former is not... e.g. ARCH_VERSTAGE_X86 is still set even if SEPERATE_VERSTAGE=n).
So you are telling to have ENABLE_STAGE_RAMSTAGE=n and ARCH_RAMSTAGE_X86 can still be yes ? is that what you wish to conclude then i can make code changes accordingly.
I guess Aaron has recommended to have ENABLE_STAGE_<bootblock/romstage/ramsatge> selection from ARCH_ config. And looks like you don't wnat that to be interdependent ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
There is a way to not build two other stages (verstage and postcar), but for both of those the option that controls whether the stage is built is completely orthogonal to the option that controls with architecture it is (and the latter is usually still set even if the former is not... e.g. ARCH_VERSTAGE_X86 is still set even if SEPERATE_VERSTAGE=n).
So you are telling to have ENABLE_STAGE_RAMSTAGE=n and ARCH_RAMSTAGE_X86 can still be yes ? is that what you wish to conclude then i can make code changes accordingly.
I guess Aaron has recommended to have ENABLE_STAGE_<bootblock/romstage/ramsatge> selection from ARCH_ config. And looks like you don't wnat that to be interdependent ?
I was saying that's the path of least resistance w.r.t. selecting the high level configs. Julius wants to put that selection somewhere else. In either case, getting the Makefile root dependencies decoupled from the implicit assumptions and base them on high level configs needs to be done *regardless* of the point of selection and steering the high level configs.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
There is a way to not build two other stages (verstage and postcar), but for both of those the option that controls whether the stage is built is completely orthogonal to the option that controls with architecture it is (and the latter is usually still set even if the former is not... e.g. ARCH_VERSTAGE_X86 is still set even if SEPERATE_VERSTAGE=n).
So you are telling to have ENABLE_STAGE_RAMSTAGE=n and ARCH_RAMSTAGE_X86 can still be yes ? is that what you wish to conclude then i can make code changes accordingly.
I guess Aaron has recommended to have ENABLE_STAGE_<bootblock/romstage/ramsatge> selection from ARCH_ config. And looks like you don't wnat that to be interdependent ?
I was saying that's the path of least resistance w.r.t. selecting the high level configs. Julius wants to put that selection somewhere else. In either case, getting the Makefile root dependencies decoupled from the implicit assumptions and base them on high level configs needs to be done *regardless* of the point of selection and steering the high level configs.
Oh yes, looks like root dependency is almost sorted now except CL: https://review.coreboot.org/c/coreboot/+/33114 which you might need to take a look.
Now stage selection is holding this ramstage independent coreboot build :(
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
There is a way to not build two other stages (verstage and postcar), but for both of those the […]
Yes, my main concern is not using the architecture toolchain configs for this. If you want a separate ENABLE_<stage> for each stage we can do that, but I think it should be independent from ARCH_<stage>_<arch>.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
Yes, my main concern is not using the architecture toolchain configs for this. […]
Got it, i will try to honor that
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33143/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33143/7//COMMIT_MSG@9 PS7, Line 9: dependency dependencies
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33143
to look at the new patch set (#8).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependency in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M toolchain.inc 2 files changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/8
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33143
to look at the new patch set (#9).
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Rampayload: Able to build coreboot without ramstage
This patch removes all possible dependencies in order to build platform with CONFIG_RAMPAYLOAD enable(without ramstage).
Change-Id: Id000dfac2eb57a8eaaa505c89d798fb30b3706fe Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M toolchain.inc 2 files changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/33143/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33143/3/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/3/toolchain.inc@50 PS3, Line 50: ifeq ($(CONFIG_RAMPAYLOAD),y)
Got it, i will try to honor that
Can you please review this patch tree, i guess i could able to remove ARCH_<> dependencies now.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Patch Set 9:
(1 comment)
I think this patch should be squashed with the first
https://review.coreboot.org/#/c/33143/9/toolchain.inc File toolchain.inc:
https://review.coreboot.org/#/c/33143/9/toolchain.inc@51 PS9, Line 51: please remove this change from the patch
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33143 )
Change subject: Rampayload: Able to build coreboot without ramstage ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)