Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Rampayload: Attempt to boot coreboot without ramstage
This patch makes below code changes to boot platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Able to compile ASL code in postcar. B. Also add option to create smm.manual/smmstub.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
C. Avoid compilation of ramstage specific files from x86/Makefile.inc when !CONFIG_HAVE_RAMSTAGE.
D. Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER to include required functions incase CONFIG_RAMPAYLOAD is enable.
Change-Id: I22994c11317cf6936828c07fcac2cf14fca8a74b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/lib/program.ld M src/mainboard/google/dragonegg/chromeos.c M src/mainboard/intel/icelake_rvp/chromeos.c 6 files changed, 28 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34476/1
diff --git a/Makefile.inc b/Makefile.inc index 2cad230..399dccc 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -252,6 +252,12 @@ # possibly true in many cases. In other cases it seems that an empty # ResourceTemplate is the correct code. # As it's valid ASL, disable the warning. +ifeq ($(CONFIG_RAMPAYLOAD),y) +TARGET_ASL_STAGE=postcar +else +TARGET_ASL_STAGE=ramstage +endif + EMPTY_RESOURCE_TEMPLATE_WARNING = 3150 IGNORED_IASL_WARNINGS = -vw $(EMPTY_RESOURCE_TEMPLATE_WARNING)
@@ -263,7 +269,7 @@ -include $(obj)/$(1).d $(obj)/$(1).aml: $(src)/mainboard/$(MAINBOARDDIR)/$(1).asl $(obj)/config.h @printf " IASL $$(subst $(top)/,,$$(@))\n" - $(CC_ramstage) -x assembler-with-cpp -E -MMD -MT $$(@) $$(CPPFLAGS_ramstage) -D__ACPI__ -P -include $(src)/include/kconfig.h -I$(obj) -I$(src) -I$(src)/include -I$(src)/arch/$(ARCHDIR-$(ARCH-ramstage-y))/include -I$(src)/mainboard/$(MAINBOARDDIR) $$< -o $(obj)/$(1).asl + $(CC_$(TARGET_ASL_STAGE)) -x assembler-with-cpp -E -MMD -MT $$(@) $$(CPPFLAGS_$(TARGET_ASL_STAGE)) -D__ACPI__ -P -include $(src)/include/kconfig.h -I$(obj) -I$(src) -I$(src)/include -I$(src)/arch/$(ARCHDIR-$(ARCH-$(TARGET_ASL_STAGE)-y))/include -I$(src)/mainboard/$(MAINBOARDDIR) $$< -o $(obj)/$(1).asl cd $$(dir $$@); $(IASL) $(IGNORED_IASL_WARNINGS) -we -p $$(notdir $$@) $(1).asl if ! $(IASL) -d $$@ 2>&1 | grep -Eq 'ACPI (Warning|Error)'; then \ echo " IASL $$@ disassembled correctly."; \ diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 32e0173..c2ca697 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -302,7 +302,7 @@ # ramstage ###############################################################################
-ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32)$(CONFIG_ARCH_RAMSTAGE_X86_64),y) +ifeq ($(CONFIG_HAVE_RAMSTAGE),y)
ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen.c diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 5c7aab3..4b227f5 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -13,15 +13,17 @@ ## GNU General Public License for more details. ##
-ramstage-y += smm_module_loader.c - -ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call create_class_compiler,smm,x86_32)) -$(eval $(call create_class_compiler,smmstub,x86_32)) +ifeq ($(CONFIG_HAVE_RAMSTAGE),y) +TARGET_STAGE=ramstage +else ifeq ($(CONFIG_RAMPAYLOAD),y) +TARGET_STAGE=postcar else -$(eval $(call create_class_compiler,smm,x86_64)) -$(eval $(call create_class_compiler,smmstub,x86_64)) +$(error Halting the build due to unknown TARGET_STAGE select) endif +$(TARGET_STAGE)-y += smm_module_loader.c + +$(eval $(call create_class_compiler,smm,$(ARCH-$(TARGET_STAGE)-y))) +$(eval $(call create_class_compiler,smmstub,$(ARCH-$(TARGET_STAGE)-y)))
smmstub-generic-ccopts += -D__SMM__ smm-generic-ccopts += -D__SMM__ @@ -32,12 +34,12 @@
# change to the target path because objcopy will use the path name in its # ELF symbol names. -$(call src-to-obj,ramstage,$(obj)/cpu/x86/smm/smm.manual): $(obj)/smm/smm +$(call src-to-obj,$(TARGET_STAGE),$(obj)/cpu/x86/smm/smm.manual): $(obj)/smm/smm @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" cd $(dir $<); $(OBJCOPY_smm) -I binary $(notdir $<) $(target-objcopy) $(abspath $@)
ifeq ($(CONFIG_HAVE_SMI_HANDLER),y) -ramstage-srcs += $(obj)/cpu/x86/smm/smm.manual +$(TARGET_STAGE)-srcs += $(obj)/cpu/x86/smm/smm.manual endif
ifeq ($(CONFIG_SMM_TSEG),y) @@ -46,7 +48,7 @@
smm-y += smm_module_handler.c
-ramstage-srcs += $(obj)/cpu/x86/smm/smmstub.manual +$(TARGET_STAGE)-srcs += $(obj)/cpu/x86/smm/smmstub.manual
# SMM Stub Module. The stub is used as a trampoline for relocation and normal # SMM handling. @@ -54,26 +56,18 @@ $(LD_smmstub) -nostdlib -r -o $@ $(COMPILER_RT_FLAGS_smmstub) --whole-archive --start-group $(smmstub-objs) --no-whole-archive $(COMPILER_RT_smmstub) --end-group
# Link the SMM stub module with a 0-byte heap. -ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call rmodule_link,$(obj)/smmstub/smmstub.elf, $(obj)/smmstub/smmstub.o, 0,x86_32)) -else -$(eval $(call rmodule_link,$(obj)/smmstub/smmstub.elf, $(obj)/smmstub/smmstub.o, 0,x86_64)) -endif +$(eval $(call rmodule_link,$(obj)/smmstub/smmstub.elf, $(obj)/smmstub/smmstub.o, 0,$(ARCH-$(TARGET_STAGE)-y)))
$(obj)/smmstub/smmstub: $(obj)/smmstub/smmstub.elf.rmod $(OBJCOPY_smmstub) -O binary $< $@
-$(call src-to-obj,ramstage,$(obj)/cpu/x86/smm/smmstub.manual): $(obj)/smmstub/smmstub +$(call src-to-obj,$(TARGET_STAGE),$(obj)/cpu/x86/smm/smmstub.manual): $(obj)/smmstub/smmstub @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" cd $(dir $<); $(OBJCOPY_smmstub) -I binary $(notdir $<) $(target-objcopy) $(abspath $@)
# C-based SMM handler.
-ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32),y) -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_32)) -else -$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),x86_64)) -endif +$(eval $(call rmodule_link,$(obj)/smm/smm.elf, $(obj)/smm/smm.o, $(CONFIG_SMM_MODULE_HEAP_SIZE),$(ARCH-$(TARGET_STAGE)-y)))
$(obj)/smm/smm: $(obj)/smm/smm.elf.rmod $(OBJCOPY_smm) -O binary $< $@ diff --git a/src/lib/program.ld b/src/lib/program.ld index 851aa75..66421d1 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -55,7 +55,7 @@ KEEP(*(.rsbe_init)); _ersbe_init_begin = .;
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _pci_drivers = .; KEEP(*(.rodata.pci_driver)); diff --git a/src/mainboard/google/dragonegg/chromeos.c b/src/mainboard/google/dragonegg/chromeos.c index 7132b04..7ef8070 100644 --- a/src/mainboard/google/dragonegg/chromeos.c +++ b/src/mainboard/google/dragonegg/chromeos.c @@ -21,7 +21,7 @@
#include <variant/gpio.h>
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER #include <boot/coreboot_tables.h>
void fill_lb_gpios(struct lb_gpios *gpios) @@ -36,7 +36,7 @@ }; lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios)); } -#endif /* ENV_RAMSTAGE */ +#endif /* ENV_PAYLOAD_LOADER */
int get_write_protect_state(void) { diff --git a/src/mainboard/intel/icelake_rvp/chromeos.c b/src/mainboard/intel/icelake_rvp/chromeos.c index ce8e548..127903b 100644 --- a/src/mainboard/intel/icelake_rvp/chromeos.c +++ b/src/mainboard/intel/icelake_rvp/chromeos.c @@ -20,7 +20,7 @@ #include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h>
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER #include <boot/coreboot_tables.h>
void fill_lb_gpios(struct lb_gpios *gpios) @@ -33,7 +33,7 @@ }; lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios)); } -#endif /* ENV_RAMSTAGE */ +#endif /* ENV_PAYLOAD_LOADER */
int get_lid_switch(void) {
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34476
to look at the new patch set (#2).
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Rampayload: Attempt to boot coreboot without ramstage
This patch makes below code changes to boot platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Able to compile ASL code in postcar. B. Also add option to create smm.manual/smmstub.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
C. Avoid compilation of ramstage specific files from x86/Makefile.inc when !CONFIG_HAVE_RAMSTAGE.
D. Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER to include required functions incase CONFIG_RAMPAYLOAD is enable.
Change-Id: I22994c11317cf6936828c07fcac2cf14fca8a74b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/arch/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/lib/program.ld M src/mainboard/google/dragonegg/chromeos.c M src/mainboard/intel/icelake_rvp/chromeos.c 6 files changed, 29 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34476/2
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 2:
(5 comments)
I've got questions, I hope you have answers :-)
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc@255 PS2, Line 255: ifeq ($(CONFIG_RAMPAYLOAD),y) Is there no way to use ENV_PAYLOAD_LOADER here somehow? Guess not.
https://review.coreboot.org/c/coreboot/+/34476/2/src/cpu/x86/smm/Makefile.in... File src/cpu/x86/smm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34476/2/src/cpu/x86/smm/Makefile.in... PS2, Line 16: ifeq ($(CONFIG_HAVE_RAMSTAGE),y) This is a very similar conditional to the one in the IASL, is there now way to create an over-arching variable to be used everywhere? If not, I guess that's ok.
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld@58 PS2, Line 58: #if ENV_PAYLOAD_LOADER oh dear. How many of these pci drivers are we using and why? I had hoped to avoid this. Aaron is going to point this out to me, I just know it!
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... File src/mainboard/google/dragonegg/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... PS2, Line 24: #if ENV_PAYLOAD_LOADER why do we need this? I thought we'd agreed that for now chromebooks and similar devices were not going to be supported? Do we have to have GPIO in the payload loader?
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... File src/mainboard/intel/icelake_rvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... PS2, Line 23: #if ENV_PAYLOAD_LOADER same question. If you are doing this just to get it all to build, let's take another look. If you need it for real reasons, I'm wondering what those reasons are?
Also, for SMM, given that I'm working on several "SMM MUST DIE" projects; can we just have the kernel do the SMM and not set it up in coreboot?
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34476
to look at the new patch set (#3).
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Rampayload: Attempt to boot coreboot without ramstage
This patch makes below code changes to boot platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create makefile variable as TARGET_STAGE. B. Able to compile ASL code in postcar. C. Also add option to create smm.manual/smmstub.manual based on $(TARGET_STAGE) variable.
$(TARGET_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(TARGET_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I22994c11317cf6936828c07fcac2cf14fca8a74b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile M Makefile.inc M src/cpu/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc 4 files changed, 18 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34476/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc@255 PS2, Line 255: ifeq ($(CONFIG_RAMPAYLOAD),y)
Is there no way to use ENV_PAYLOAD_LOADER here somehow? […]
i don't think ENV_PAYLOAD_LOADER macro will able to tell us what is the stage name.
https://review.coreboot.org/c/coreboot/+/34476/2/src/cpu/x86/smm/Makefile.in... File src/cpu/x86/smm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34476/2/src/cpu/x86/smm/Makefile.in... PS2, Line 16: ifeq ($(CONFIG_HAVE_RAMSTAGE),y)
This is a very similar conditional to the one in the IASL, is there now way to create an over-archin […]
Done
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld@58 PS2, Line 58: #if ENV_PAYLOAD_LOADER
oh dear. How many of these pci drivers are we using and why? I had hoped to avoid this. […]
we will try to avoid this. removing now, let see later
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... File src/mainboard/google/dragonegg/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... PS2, Line 24: #if ENV_PAYLOAD_LOADER
why do we need this? I thought we'd agreed that for now chromebooks and similar devices were not goi […]
if we plan to not support chrome device then we don't need this now
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... File src/mainboard/intel/icelake_rvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... PS2, Line 23: #if ENV_PAYLOAD_LOADER
same question. If you are doing this just to get it all to build, let's take another look. If you need it for real reasons, I'm wondering what those reasons are?
removing now.
Also, for SMM, given that I'm working on several "SMM MUST DIE" projects; can we just have the kernel do the SMM and not set it up in coreboot?
this is interesting
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34476/2/Makefile.inc@255 PS2, Line 255: ifeq ($(CONFIG_RAMPAYLOAD),y)
i don't think ENV_PAYLOAD_LOADER macro will able to tell us what is the stage name.
Done
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld@58 PS2, Line 58: #if ENV_PAYLOAD_LOADER
we will try to avoid this. […]
Done
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... File src/mainboard/google/dragonegg/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/google/dragon... PS2, Line 24: #if ENV_PAYLOAD_LOADER
if we plan to not support chrome device then we don't need this now
Done
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... File src/mainboard/intel/icelake_rvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/34476/2/src/mainboard/intel/icelake... PS2, Line 23: #if ENV_PAYLOAD_LOADER
same question. If you are doing this just to get it all to build, let's take another look. […]
Done
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 3:
This is much more what I was hoping was possible. But, you've introduced a new name TARGET_STAGE and then in the config.h we have ENV_PAYLOAD_LOADER
But these two things are connected in function, but not in the name. They are in some sense the terminal stage before the payload.
This seems confusing to me. Could you not have PAYLOAD_LOADER_STAGE or PRE_PAYLOAD_STAGE or FINAL_STAGE or something? Sorry, again, TARGET_STAGE doesn't communicate anything to me.
Is there some other variable name that shows a connection to the fact that the stage in question is the payload loader? Or am I missing the point here? It's just that, looking at the name TARGET_STAGE, it's hard to draw a meaning from the name. Thanks.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 3:
Patch Set 3:
This is much more what I was hoping was possible. But, you've introduced a new name TARGET_STAGE and then in the config.h we have ENV_PAYLOAD_LOADER
But these two things are connected in function, but not in the name. They are in some sense the terminal stage before the payload.
This seems confusing to me. Could you not have PAYLOAD_LOADER_STAGE or PRE_PAYLOAD_STAGE or FINAL_STAGE or something? Sorry, again, TARGET_STAGE doesn't communicate anything to me.
Is there some other variable name that shows a connection to the fact that the stage in question is the payload loader? Or am I missing the point here? It's just that, looking at the name TARGET_STAGE, it's hard to draw a meaning from the name. Thanks.
Ron, the idea is to not add redundant code block between ramstage or postcar to compile. For an example: AML generation code might be required for paylaod stage (ramstage/postcar) but how do we pass the target_stage name where we want to compile "ramstage" or "postcar" ?
Right now ENV_PAYLOAD_LOADER macro will tell us if payload stage is selected/enable but it won't tell us the is the name of that stage ? "ramstage" or "postcar"? here i was trying to create a new makefile variable to solve that gap and using the variable to avoid adding redundant code block
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
This is much more what I was hoping was possible. But, you've introduced a new name TARGET_STAGE and then in the config.h we have ENV_PAYLOAD_LOADER
But these two things are connected in function, but not in the name. They are in some sense the terminal stage before the payload.
This seems confusing to me. Could you not have PAYLOAD_LOADER_STAGE or PRE_PAYLOAD_STAGE or FINAL_STAGE or something? Sorry, again, TARGET_STAGE doesn't communicate anything to me.
Is there some other variable name that shows a connection to the fact that the stage in question is the payload loader? Or am I missing the point here? It's just that, looking at the name TARGET_STAGE, it's hard to draw a meaning from the name. Thanks.
Ron, the idea is to not add redundant code block between ramstage or postcar to compile. For an example: AML generation code might be required for paylaod stage (ramstage/postcar) but how do we pass the target_stage name where we want to compile "ramstage" or "postcar" ?
Right now ENV_PAYLOAD_LOADER macro will tell us if payload stage is selected/enable but it won't tell us the is the name of that stage ? "ramstage" or "postcar"? here i was trying to create a new makefile variable to solve that gap and using the variable to avoid adding redundant code block
but agree that we should give some better name for better readability. i'm not good at naming :(
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34476
to look at the new patch set (#4).
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Rampayload: Attempt to boot coreboot without ramstage
This patch makes below code changes to boot platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create makefile variable as PAYLOAD_LOADER_STAGE. B. Able to compile ASL code in postcar. C. Also add option to create smm.manual/smmstub.manual based on $(PAYLOAD_LOADER_STAGE) variable.
$(PAYLOAD_LOADER_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(PAYLOAD_LOADER_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
Change-Id: I22994c11317cf6936828c07fcac2cf14fca8a74b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile M Makefile.inc M src/cpu/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc 4 files changed, 26 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34476/4
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34476
to look at the new patch set (#5).
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Rampayload: Attempt to boot coreboot without ramstage
This patch makes below code changes to boot platform with CONFIG_RAMPAYLOAD enable(without ramstage).
A. Create makefile variable as PAYLOAD_LOADER_STAGE. B. Able to compile ASL code in postcar. C. Also add option to create smm.manual/smmstub.manual based on $(PAYLOAD_LOADER_STAGE) variable.
$(PAYLOAD_LOADER_STAGE)=ramstage if user selects CONFIG_HAVE_RAMSTAGE $(PAYLOAD_LOADER_STAGE)=postcar if user selects CONFIG_RAMPAYLOAD
D. Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER to perform mp initialization (cpu_initialize(index)) incase CONFIG_RAMPAYLOAD is enable.
Change-Id: I22994c11317cf6936828c07fcac2cf14fca8a74b Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile M Makefile.inc M src/cpu/x86/Makefile.inc M src/cpu/x86/smm/Makefile.inc M src/lib/program.ld 5 files changed, 27 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34476/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34476/2/src/lib/program.ld@58 PS2, Line 58: #if ENV_PAYLOAD_LOADER
Done
looks like _cpu_drivers is need for cpu_initialize() during MP init from coreboot
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
I still don't understand the direction here. I'm still of the opinion we're essentially recreating ramstage and calling it a new name. What are the limitations of romstage/ramstage pair that this is trying to solve? As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
What are the limitations of romstage/ramstage pair that this is trying to solve?
we can remove romstage from the scope of coreboot-lite/rampaylaod work discussion as romstage is going to stay here for sure
what we are trying to achieve is that avoid loading 1 dedicated stage (i.e ramstage here) and if we can just pull in required functionality into previous stage for booting a platform.
So far we have seen ~240ms of time saving in this approach with some known WA.
migrating to rampayload might be long pole for chrome platform but we might think about some use case for coreboot to compete with ABL, where bootloader has to be more thinner and meet certain boot time restriction.
As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
i'm also afraid of same situation but so far if i look at my POC patch for reference. From POC CL to here, i have almost ported required functionalities like MP init, AML generation into this CL.
Now next steps would be thinking about possible solutions to make dynamic BAR assignment of limited PCI resource (Input, Output and boot device) and see how do we create run time AML for peripheral devices (touch, tpm etc). In this process we might need to compile FSP call into previous stage (postcar here)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
What are the limitations of romstage/ramstage pair that this is trying to solve?
we can remove romstage from the scope of coreboot-lite/rampaylaod work discussion as romstage is going to stay here for sure
So are we packing in memory training and other things into this new stage. i.e. combine romstage and ramstage? From a high level, that's how I read what you are attempting, but there's lots of weird details there so I don't think that's happening. I still think we're doing romstge -> newstage -> kernel
what we are trying to achieve is that avoid loading 1 dedicated stage (i.e ramstage here) and if we can just pull in required functionality into previous stage for booting a platform.
So far we have seen ~240ms of time saving in this approach with some known WA.
Can you quantify where the savings is coming from? We should have an idea what is the source of the savings because we could focus on improving that aspect in ramstage.
migrating to rampayload might be long pole for chrome platform but we might think about some use case for coreboot to compete with ABL, where bootloader has to be more thinner and meet certain boot time restriction.
As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
i'm also afraid of same situation but so far if i look at my POC patch for reference. From POC CL to here, i have almost ported required functionalities like MP init, AML generation into this CL.
Now next steps would be thinking about possible solutions to make dynamic BAR assignment of limited PCI resource (Input, Output and boot device) and see how do we create run time AML for peripheral devices (touch, tpm etc). In this process we might need to compile FSP call into previous stage (postcar here)
I think it'd be instructive to do a side by side comparison of our "traditional" boot flow along w/ the actions/details each stage is performing against your target boot flow. There are high level semantics that are needed to be employed. e.g. CAR tear down for a clean program environment, etc. Those building blocks would ideally be able to be moved around to form different boot flows. However, I think we should understand the current limitations and see if we can improve those before going down a more complicated solution. e.g. do you save time by tearing down CAR in ramstage itself? i.e. prologue of tearing down CAR is in first part of ramstage.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
What are the limitations of romstage/ramstage pair that this is trying to solve?
we can remove romstage from the scope of coreboot-lite/rampaylaod work discussion as romstage is going to stay here for sure
So are we packing in memory training and other things into this new stage. i.e. combine romstage and ramstage?
We might don't want to disturb romstage if we have postcar existed, then basically romstage -> postcar -> kernel
From a high level, that's how I read what you are attempting, but there's lots of weird details there so I don't think that's happening. I still think we're doing romstge -> newstage -> kernel
you are right that postcar might be extended in feature to pull in required functions from ramstage to boot to OS. Like MP init, ASL generation
what we are trying to achieve is that avoid loading 1 dedicated stage (i.e ramstage here) and if we can just pull in required functionality into previous stage for booting a platform.
So far we have seen ~240ms of time saving in this approach with some known WA.
Can you quantify where the savings is coming from? We should have an idea what is the source of the savings because we could focus on improving that aspect in ramstage.
yes, i have those break down, i will share time details to you over email as i can't attach .log files here with RAMPAYLOAD enable.
migrating to rampayload might be long pole for chrome platform but we might think about some use case for coreboot to compete with ABL, where bootloader has to be more thinner and meet certain boot time restriction.
As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
i'm also afraid of same situation but so far if i look at my POC patch for reference. From POC CL to here, i have almost ported required functionalities like MP init, AML generation into this CL.
Now next steps would be thinking about possible solutions to make dynamic BAR assignment of limited PCI resource (Input, Output and boot device) and see how do we create run time AML for peripheral devices (touch, tpm etc). In this process we might need to compile FSP call into previous stage (postcar here)
I think it'd be instructive to do a side by side comparison of our "traditional" boot flow along w/ the actions/details each stage is performing against your target boot flow. There are high level semantics that are needed to be employed. e.g. CAR tear down for a clean program environment, etc. Those building blocks would ideally be able to be moved around to form different boot flows. However, I think we should understand the current limitations and see if we can improve those before going down a more complicated solution. e.g. do you save time by tearing down CAR in ramstage itself? i.e. prologue of tearing down CAR is in first part of ramstage.
if i'm not wrong, we are tearning down the CAR in postcar for IA platform and i don't see much savings if we don't tear the CAR in postcar.
at high level i think 2 place where definitely we could improve would be
1. Avoid entire PCI enumeration and setup 2. Executing any stage has its own problem of finding from cbfs, decompressesion and loading into memory.
This approach actually tries to broadly avoid these 2 things in ramstage and achieve the same via postcar itself
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
What are the limitations of romstage/ramstage pair that this is trying to solve?
we can remove romstage from the scope of coreboot-lite/rampaylaod work discussion as romstage is going to stay here for sure
So are we packing in memory training and other things into this new stage. i.e. combine romstage and ramstage?
We might don't want to disturb romstage if we have postcar existed, then basically romstage -> postcar -> kernel
From a high level, that's how I read what you are attempting, but there's lots of weird details there so I don't think that's happening. I still think we're doing romstge -> newstage -> kernel
you are right that postcar might be extended in feature to pull in required functions from ramstage to boot to OS. Like MP init, ASL generation
what we are trying to achieve is that avoid loading 1 dedicated stage (i.e ramstage here) and if we can just pull in required functionality into previous stage for booting a platform.
So far we have seen ~240ms of time saving in this approach with some known WA.
Can you quantify where the savings is coming from? We should have an idea what is the source of the savings because we could focus on improving that aspect in ramstage.
yes, i have those break down, i will share time details to you over email as i can't attach .log files here with RAMPAYLOAD enable.
I think that analysis should be documented to live on as well as serve as a basis for the approach as a whole. I'm still not convinced that the approach we're taking is the right way.
migrating to rampayload might be long pole for chrome platform but we might think about some use case for coreboot to compete with ABL, where bootloader has to be more thinner and meet certain boot time restriction.
As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
i'm also afraid of same situation but so far if i look at my POC patch for reference. From POC CL to here, i have almost ported required functionalities like MP init, AML generation into this CL.
Now next steps would be thinking about possible solutions to make dynamic BAR assignment of limited PCI resource (Input, Output and boot device) and see how do we create run time AML for peripheral devices (touch, tpm etc). In this process we might need to compile FSP call into previous stage (postcar here)
I think it'd be instructive to do a side by side comparison of our "traditional" boot flow along w/ the actions/details each stage is performing against your target boot flow. There are high level semantics that are needed to be employed. e.g. CAR tear down for a clean program environment, etc. Those building blocks would ideally be able to be moved around to form different boot flows. However, I think we should understand the current limitations and see if we can improve those before going down a more complicated solution. e.g. do you save time by tearing down CAR in ramstage itself? i.e. prologue of tearing down CAR is in first part of ramstage.
if i'm not wrong, we are tearning down the CAR in postcar for IA platform and i don't see much savings if we don't tear the CAR in postcar.
That's not what I'm saying. My mental model is that you are slowly adding in features/properties of ramstage into postcar as you realize you need them. As such this comes back to where the savings in boot time is coming from. If we just bolted on CAR teardown in ramstage entry does that reap the gains that you are seeing? If not, what other low hanging fruit is there?
at high level i think 2 place where definitely we could improve would be
- Avoid entire PCI enumeration and setup
At what cost? Putting in lots of device specific code to program bars as needed? Similarly, can't this be achieved by tweaking the exiting boot flow? In the end it has to be an option because one cannot rely on the eventual kernel/payload performing the actions you are trying to defer here.
- Executing any stage has its own problem of finding from cbfs, decompressesion and loading into memory.
But this is a solved problem. All stages know how to load next program.
This approach actually tries to broadly avoid these 2 things in ramstage and achieve the same via postcar itself
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
What are the limitations of romstage/ramstage pair that this is trying to solve?
we can remove romstage from the scope of coreboot-lite/rampaylaod work discussion as romstage is going to stay here for sure
So are we packing in memory training and other things into this new stage. i.e. combine romstage and ramstage?
We might don't want to disturb romstage if we have postcar existed, then basically romstage -> postcar -> kernel
From a high level, that's how I read what you are attempting, but there's lots of weird details there so I don't think that's happening. I still think we're doing romstge -> newstage -> kernel
you are right that postcar might be extended in feature to pull in required functions from ramstage to boot to OS. Like MP init, ASL generation
what we are trying to achieve is that avoid loading 1 dedicated stage (i.e ramstage here) and if we can just pull in required functionality into previous stage for booting a platform.
So far we have seen ~240ms of time saving in this approach with some known WA.
Can you quantify where the savings is coming from? We should have an idea what is the source of the savings because we could focus on improving that aspect in ramstage.
yes, i have those break down, i will share time details to you over email as i can't attach .log files here with RAMPAYLOAD enable.
I think that analysis should be documented to live on as well as serve as a basis for the approach as a whole. I'm still not convinced that the approach we're taking is the right way.
Sure i will add a live document link to capture this.
migrating to rampayload might be long pole for chrome platform but we might think about some use case for coreboot to compete with ABL, where bootloader has to be more thinner and meet certain boot time restriction.
As I noted before, you'll be pulling in almost all of the functionality from ramstage in order to get a system proper configured.
i'm also afraid of same situation but so far if i look at my POC patch for reference. From POC CL to here, i have almost ported required functionalities like MP init, AML generation into this CL.
Now next steps would be thinking about possible solutions to make dynamic BAR assignment of limited PCI resource (Input, Output and boot device) and see how do we create run time AML for peripheral devices (touch, tpm etc). In this process we might need to compile FSP call into previous stage (postcar here)
I think it'd be instructive to do a side by side comparison of our "traditional" boot flow along w/ the actions/details each stage is performing against your target boot flow. There are high level semantics that are needed to be employed. e.g. CAR tear down for a clean program environment, etc. Those building blocks would ideally be able to be moved around to form different boot flows. However, I think we should understand the current limitations and see if we can improve those before going down a more complicated solution. e.g. do you save time by tearing down CAR in ramstage itself? i.e. prologue of tearing down CAR is in first part of ramstage.
if i'm not wrong, we are tearning down the CAR in postcar for IA platform and i don't see much savings if we don't tear the CAR in postcar.
That's not what I'm saying. My mental model is that you are slowly adding in features/properties of ramstage into postcar as you realize you need them. As such this comes back to where the savings in boot time is coming from. If we just bolted on CAR teardown in ramstage entry does that reap the gains that you are seeing?
no, CAR teardown in ramstage won't help much
If not, what other low hanging fruit is there?
As i told low hanging fruits would be limiting the time that we are spending during PCI enumeration and setting up the resources. I would advocate about limiting the functionality in ramstage as well (without introducing RAMPAYLOAD concept), if i could skip entire PCI tree enumeration and do in a fixed manner means a way where user provides list of PCI devices (might have from BIOS side to boot to paylaod/kernel) based on some Kconfig options. Ultimately BIOS is only bother about fixing some chipset WA/recommended programming and setting up BAR to communicate with devices to boot from.
Today entire PCI enumeration takes ~160ms+ time, which we can limit and still could able to boot to kernel.
30:device enumeration 466,492 (50,800) 40:device configuration 589,305 (21,695) 50:device enable 638,470 (258) 60:device initialization 665,478 (27,007) 70:device setup done 755,515 (90,037) 75:cbmem post 756,226 (711) 80:write tables 756,437 (210)
Also loading any additional stage takes some time. Just loading fallback/ramstage will take ~30ms
8:starting to load ramstage 383,195 (138) 15:starting LZMA decompress (ignore for x86) 383,210 (14) 16:finished LZMA decompress (ignore for x86) 408,031 (24,820) 9:finished loading ramstage 415,242 (7,211) 550:starting to load Chrome OS VPD 415,321 (78) 10:start of ramstage 415,692 (370)
at high level i think 2 place where definitely we could improve would be
- Avoid entire PCI enumeration and setup
At what cost? Putting in lots of device specific code to program bars as needed? Similarly, can't this be achieved by tweaking the exiting boot flow? In the end it has to be an option because one cannot rely on the eventual kernel/payload performing the actions you are trying to defer here.
I guess the concern would be how do we limit our FW space and make FW doing minimum then what is doing today. You are right that skipping entire PCI enumeration in BIOS space means it will happen inside kernel (if kernel has provision) and it might add up to same time (if we bother about end to end booting time). But the counter argument would be why to bloat BIOS with an already existed kernel feature and user won't love to see more time spent on FW space and might interested to see OS UI as soon as power on the device. if we could save ~200ms+ time using this approach then we would be closer to ~600ms of booting time (per with ABL standard even with CB without making any HW BOM change)
Agree that there might be some price to pay for that and it could have done using existing ramstage way as well (Furquan had requested for the same and i have provided him the time estimation). Based on above data, you can say, dropping fallback/ramstage might saves you ~40ms of boot time and ~150kB * 3 copies = ~450kB spi footprint reduction over doing everything on top of ramstage. (without rampayload)
- Executing any stage has its own problem of finding from cbfs, decompressesion and loading into memory.
But this is a solved problem. All stages know how to load next program.
Sorry for the confusion, i mean to say loading time, not the locating mechanism :) i have provided time saving data above.
This approach actually tries to broadly avoid these 2 things in ramstage and achieve the same via postcar itself
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
<- snip ->
I think that analysis should be documented to live on as well as serve as a basis for the approach as a whole. I'm still not convinced that the approach we're taking is the right way.
Sure i will add a live document link to capture this.
I just meant it should be in some documentation in the tree as well as the commit description.
<- snip ->
If not, what other low hanging fruit is there?
As i told low hanging fruits would be limiting the time that we are spending during PCI enumeration and setting up the resources. I would advocate about limiting the functionality in ramstage as well (without introducing RAMPAYLOAD concept), if i could skip entire PCI tree enumeration and do in a fixed manner means a way where user provides list of PCI devices (might have from BIOS side to boot to paylaod/kernel) based on some Kconfig options. Ultimately BIOS is only bother about fixing some chipset WA/recommended programming and setting up BAR to communicate with devices to boot from.
Today entire PCI enumeration takes ~160ms+ time, which we can limit and still could able to boot to kernel.
30:device enumeration 466,492 (50,800) 40:device configuration 589,305 (21,695) 50:device enable 638,470 (258) 60:device initialization 665,478 (27,007) 70:device setup done 755,515 (90,037) 75:cbmem post 756,226 (711) 80:write tables 756,437 (210)
Also loading any additional stage takes some time. Just loading fallback/ramstage will take ~30ms
8:starting to load ramstage 383,195 (138) 15:starting LZMA decompress (ignore for x86) 383,210 (14) 16:finished LZMA decompress (ignore for x86) 408,031 (24,820) 9:finished loading ramstage 415,242 (7,211) 550:starting to load Chrome OS VPD 415,321 (78) 10:start of ramstage 415,692 (370)
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
At what cost? Putting in lots of device specific code to program bars as needed? Similarly, can't this be achieved by tweaking the exiting boot flow? In the end it has to be an option because one cannot rely on the eventual kernel/payload performing the actions you are trying to defer here.
I guess the concern would be how do we limit our FW space and make FW doing minimum then what is doing today. You are right that skipping entire PCI enumeration in BIOS space means it will happen inside kernel (if kernel has provision) and it might add up to same time (if we bother about end to end booting time). But the counter argument would be why to bloat BIOS with an already existed kernel feature and user won't love to see more time spent on FW space and might interested to see OS UI as soon as power on the device. if we could save ~200ms+ time using this approach then we would be closer to ~600ms of booting time (per with ABL standard even with CB without making any HW BOM change)
This is a new feature as well. Providing BARs to statically allocate and slam in for initialization in the boot flow. I definitely don't think we should be manually solving such a thing though. i.e. writing specific code for initializing BARs. It should be done at build time and instructions for which BAR/register to init should just be executed. That is a more scalable approach if one wanted to go down this path.
Agree that there might be some price to pay for that and it could have done using existing ramstage way as well (Furquan had requested for the same and i have provided him the time estimation). Based on above data, you can say, dropping fallback/ramstage might saves you ~40ms of boot time and ~150kB * 3 copies = ~450kB spi footprint reduction over doing everything on top of ramstage. (without rampayload)
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
<- snip ->
I think that analysis should be documented to live on as well as serve as a basis for the approach as a whole. I'm still not convinced that the approach we're taking is the right way.
Sure i will add a live document link to capture this.
I just meant it should be in some documentation in the tree as well as the commit description.
<- snip ->
If not, what other low hanging fruit is there?
As i told low hanging fruits would be limiting the time that we are spending during PCI enumeration and setting up the resources. I would advocate about limiting the functionality in ramstage as well (without introducing RAMPAYLOAD concept), if i could skip entire PCI tree enumeration and do in a fixed manner means a way where user provides list of PCI devices (might have from BIOS side to boot to paylaod/kernel) based on some Kconfig options. Ultimately BIOS is only bother about fixing some chipset WA/recommended programming and setting up BAR to communicate with devices to boot from.
Today entire PCI enumeration takes ~160ms+ time, which we can limit and still could able to boot to kernel.
30:device enumeration 466,492 (50,800) 40:device configuration 589,305 (21,695) 50:device enable 638,470 (258) 60:device initialization 665,478 (27,007) 70:device setup done 755,515 (90,037) 75:cbmem post 756,226 (711) 80:write tables 756,437 (210)
Also loading any additional stage takes some time. Just loading fallback/ramstage will take ~30ms
8:starting to load ramstage 383,195 (138) 15:starting LZMA decompress (ignore for x86) 383,210 (14) 16:finished LZMA decompress (ignore for x86) 408,031 (24,820) 9:finished loading ramstage 415,242 (7,211) 550:starting to load Chrome OS VPD 415,321 (78) 10:start of ramstage 415,692 (370)
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
I believe you are referring to reduced feature ramstage (without introducing RAMPAYLOAD concept) ?
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
yes, for sure we will save postcar loading time. as postcar by default is smaller in size hence saving won't be that much as compare to ramstage but i totally agree here.
At what cost? Putting in lots of device specific code to program bars as needed? Similarly, can't this be achieved by tweaking the exiting boot flow? In the end it has to be an option because one cannot rely on the eventual kernel/payload performing the actions you are trying to defer here.
I guess the concern would be how do we limit our FW space and make FW doing minimum then what is doing today. You are right that skipping entire PCI enumeration in BIOS space means it will happen inside kernel (if kernel has provision) and it might add up to same time (if we bother about end to end booting time). But the counter argument would be why to bloat BIOS with an already existed kernel feature and user won't love to see more time spent on FW space and might interested to see OS UI as soon as power on the device. if we could save ~200ms+ time using this approach then we would be closer to ~600ms of booting time (per with ABL standard even with CB without making any HW BOM change)
This is a new feature as well. Providing BARs to statically allocate and slam in for initialization in the boot flow. I definitely don't think we should be manually solving such a thing though. i.e. writing specific code for initializing BARs. It should be done at build time and instructions for which BAR/register to init should just be executed. That is a more scalable approach if one wanted to go down this path.
yes, Aaron. i'm looking for something like that and would be great if you can help on this line.
Agree that there might be some price to pay for that and it could have done using existing ramstage way as well (Furquan had requested for the same and i have provided him the time estimation). Based on above data, you can say, dropping fallback/ramstage might saves you ~40ms of boot time and ~150kB * 3 copies = ~450kB spi footprint reduction over doing everything on top of ramstage. (without rampayload)
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
We are planning to present those data in OSFC hopefully :) . originally i have done this POC to gather those number in boot time saving, spi flash saving etc. Without running a POC, it might not helpful to have. And if you see the RAMPAYLOAD patch series, i haven't introduced any new piece that exclusive for RAMPAYLOAD activity. I was just organizing the current code to reduce POC code size.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Interesting discussion. I’d prefer if you moved the discussion to the mailing list.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
I believe you are referring to reduced feature ramstage (without introducing RAMPAYLOAD concept) ?
correct
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
yes, for sure we will save postcar loading time. as postcar by default is smaller in size hence saving won't be that much as compare to ramstage but i totally agree here.
It gets amortized when it's included in a single program, though. The smaller code size the other piece of your goal (as noted below).
<- snip ->
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
We are planning to present those data in OSFC hopefully :) . originally i have done this POC to gather those number in boot time saving, spi flash saving etc. Without running a POC, it might not helpful to have. And if you see the RAMPAYLOAD patch series, i haven't introduced any new piece that exclusive for RAMPAYLOAD activity. I was just organizing the current code to reduce POC code size.
Sure, but I still stand that we don't need this RAMPAYLOAD if we attack the problems directly in ramstage.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
I believe you are referring to reduced feature ramstage (without introducing RAMPAYLOAD concept) ?
correct
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
yes, for sure we will save postcar loading time. as postcar by default is smaller in size hence saving won't be that much as compare to ramstage but i totally agree here.
It gets amortized when it's included in a single program, though. The smaller code size the other piece of your goal (as noted below).
<- snip ->
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
We are planning to present those data in OSFC hopefully :) . originally i have done this POC to gather those number in boot time saving, spi flash saving etc. Without running a POC, it might not helpful to have. And if you see the RAMPAYLOAD patch series, i haven't introduced any new piece that exclusive for RAMPAYLOAD activity. I was just organizing the current code to reduce POC code size.
Sure, but I still stand that we don't need this RAMPAYLOAD if we attack the problems directly in ramstage.
yes Aaron, RAMPAYLOAD would be an additional step which might take a step back for now, till the time we fixes PCI BAR allocation issue for limited pci device programmatically as you have mentioned above, I might need some help there.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
I believe you are referring to reduced feature ramstage (without introducing RAMPAYLOAD concept) ?
correct
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
yes, for sure we will save postcar loading time. as postcar by default is smaller in size hence saving won't be that much as compare to ramstage but i totally agree here.
It gets amortized when it's included in a single program, though. The smaller code size the other piece of your goal (as noted below).
<- snip ->
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
We are planning to present those data in OSFC hopefully :) . originally i have done this POC to gather those number in boot time saving, spi flash saving etc. Without running a POC, it might not helpful to have. And if you see the RAMPAYLOAD patch series, i haven't introduced any new piece that exclusive for RAMPAYLOAD activity. I was just organizing the current code to reduce POC code size.
Sure, but I still stand that we don't need this RAMPAYLOAD if we attack the problems directly in ramstage.
yes Aaron, RAMPAYLOAD would be an additional step which might take a step back for now, till the time we fixes PCI BAR allocation issue for limited pci device programmatically as you have mentioned above, I might need some help there.
I disagree. It creates churn and confusion without attacking the problem head-on. It's fine for a proof of concept, but I don't think we should be introducing this complexity in order to avoid working on the final solution.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34476 )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
<- snip ->
It seems you've identified a feature you want to add that reduces PCI enumeration and init costs (or speed up the current approach). That's a way different direction than trying to recreate ramstage on a piecemeal basis. Given what you are indicating that seems like the better approach.
I believe you are referring to reduced feature ramstage (without introducing RAMPAYLOAD concept) ?
correct
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
yes, for sure we will save postcar loading time. as postcar by default is smaller in size hence saving won't be that much as compare to ramstage but i totally agree here.
It gets amortized when it's included in a single program, though. The smaller code size the other piece of your goal (as noted below).
<- snip ->
Again, this is another goal: reduce size of ramstage. Great. Where's the analysis with the break down of code size? And why aren't we targeting ways to reduce that directly in ramstage?
We are planning to present those data in OSFC hopefully :) . originally i have done this POC to gather those number in boot time saving, spi flash saving etc. Without running a POC, it might not helpful to have. And if you see the RAMPAYLOAD patch series, i haven't introduced any new piece that exclusive for RAMPAYLOAD activity. I was just organizing the current code to reduce POC code size.
Sure, but I still stand that we don't need this RAMPAYLOAD if we attack the problems directly in ramstage.
yes Aaron, RAMPAYLOAD would be an additional step which might take a step back for now, till the time we fixes PCI BAR allocation issue for limited pci device programmatically as you have mentioned above, I might need some help there.
I disagree. It creates churn and confusion without attacking the problem head-on. It's fine for a proof of concept, but I don't think we should be introducing this complexity in order to avoid working on the final solution.
got your point, we should spend more on reduced ramstage feature then
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34476?usp=email )
Change subject: Rampayload: Attempt to boot coreboot without ramstage ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aaron Durbin (Code Review) -
Martin L Roth (Code Review)
-
Paul Menzel (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)