Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!HAVE_POSTCAR) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB *3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/include/arch/cpu.h A src/arch/x86/ramstage_loader.c M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 7 files changed, 118 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/1
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 2fad75e..f8d8c5f 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -239,6 +239,9 @@ romstage-y += memset.c romstage-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c romstage-y += postcar_loader.c +ifneq ($(CONFIG_HAVE_POSTCAR),y) +romstage-y += ramstage_loader.c +endif romstage-y += stage_loader.c romstage-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c romstage-$(CONFIG_ARCH_ROMSTAGE_X86_32) += walkcbfs.S @@ -305,6 +308,10 @@
ifeq ($(CONFIG_ARCH_RAMSTAGE_X86_32)$(CONFIG_ARCH_RAMSTAGE_X86_64),y)
+ifneq ($(CONFIG_HAVE_POSTCAR),y) +ramstage-y += exit_car.S +ramstage-y += gdt_init.S +endif ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpigen_dsm.c diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S index 32b848d..463eea9 100644 --- a/src/arch/x86/c_start.S +++ b/src/arch/x86/c_start.S @@ -37,8 +37,13 @@ #else .code32 #endif +#if CONFIG(HAVE_POSTCAR) .globl _start _start: +#else + .globl ramstage_start +ramstage_start: +#endif cli lgdt %cs:gdtaddr #ifndef __x86_64__ diff --git a/src/arch/x86/exit_car.S b/src/arch/x86/exit_car.S index 769a758..662b199 100644 --- a/src/arch/x86/exit_car.S +++ b/src/arch/x86/exit_car.S @@ -127,7 +127,11 @@ /* Align stack to 16 bytes at call instruction. */ andl $0xfffffff0, %esp /* Call into main for postcar. */ +#if CONFIG(HAVE_POSTCAR) call main +#else + call ramstage_start +#endif /* Should never return. */ 1: jmp 1b diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 293ca02..0f8838b 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -345,6 +345,8 @@ */ void run_postcar_phase(struct postcar_frame *pcf);
+void run_ramstage_phase(struct postcar_frame *pcf); + /* * Systems without a native coreboot cache-as-ram teardown may implement * this to use an alternate method. diff --git a/src/arch/x86/ramstage_loader.c b/src/arch/x86/ramstage_loader.c new file mode 100644 index 0000000..bf35ef5 --- /dev/null +++ b/src/arch/x86/ramstage_loader.c @@ -0,0 +1,90 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/cpu.h> +#include <arch/stage_loader.h> +#include <cbmem.h> +#include <cbfs.h> +#include <console/console.h> +#include <cpu/cpu.h> +#include <cpu/x86/msr.h> +#include <cpu/x86/mtrr.h> +#include <program_loading.h> +#include <rmodule.h> +#include <romstage_handoff.h> +#include <stage_cache.h> +#include <timestamp.h> + +static void *ramstage_commit_mtrrs(struct postcar_frame *pcf) +{ + /* + * Place the number of used variable MTRRs on stack then max number + * of variable MTRRs supported in the system. + */ + stack_push(pcf, pcf->num_var_mtrrs); + stack_push(pcf, pcf->max_var_mtrrs); + return (void *) pcf->stack; +} + +static void load_ramstage(struct prog *ramstage, struct postcar_frame *pcf) +{ + struct rmod_stage_load rmod_ram = { + .cbmem_id = CBMEM_ID_RAMSTAGE, + .prog = ramstage, + }; + + if (prog_locate(ramstage)) + die_with_post_code(POST_INVALID_ROM, + "Failed to locate after CAR program.\n"); + + if (rmodule_stage_load(&rmod_ram)) + die_with_post_code(POST_INVALID_ROM, + "Failed to load after CAR program.\n"); + + /* Set the stack pointer within parameters of the program loaded. */ + if (rmod_ram.params == NULL) + die_with_post_code(POST_INVALID_ROM, + "No parameters found in after CAR program.\n"); + + finalize_load(rmod_ram.params, pcf->stack); + + stage_cache_add(STAGE_RAMSTAGE, ramstage); +} + +void run_ramstage_phase(struct postcar_frame *pcf) +{ + struct prog ramstage = + PROG_INIT(PROG_RAMSTAGE, CONFIG_CBFS_PREFIX "/ramstage"); + + timestamp_add_now(TS_END_ROMSTAGE); + + ramstage_commit_mtrrs(pcf); + + timestamp_add_now(TS_START_COPYRAM); + if (!CONFIG(NO_STAGE_CACHE) && + romstage_handoff_is_resume()) { + stage_cache_load_stage(STAGE_RAMSTAGE, &ramstage); + /* This is here to allow platforms to pass different stack + parameters between S3 resume and normal boot. On the + platforms where the values are the same it's a nop. */ + finalize_load(ramstage.arg, pcf->stack); + } else + load_ramstage(&ramstage, pcf); + + /* As postcar exist, it's end of romstage here */ + timestamp_add_now(TS_END_COPYRAM); + + prog_run(&ramstage); +} diff --git a/src/lib/program.ld b/src/lib/program.ld index 851aa75..f6a6421 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -91,13 +91,13 @@ _data = .;
/* - * The postcar phase uses a stack value that is located in the relocatable - * module section. While the postcar stage could be linked like smm and - * other rmodules the postcar stage needs similar semantics of the more - * traditional stages in the coreboot infrastructure. Therefore it's easier - * to specialize this case. + * The postcar or ramstage (incase postcar not enable) phase uses a stack value + * that is located in the relocatable module section. While the postcar/ + * ramstage stage could be linked like smm and other rmodules the postcar/ + * ramstage stage needs similar semantics of the more traditional stages in the + * coreboot infrastructure. Therefore it's easier to specialize this case. */ -#if ENV_RMODULE || ENV_POSTCAR +#if ENV_RMODULE || ENV_POSTCAR || !CONFIG(HAVE_POSTCAR) _rmodule_params = .; KEEP(*(.module_parameters)); _ermodule_params = .; diff --git a/src/soc/intel/common/block/cpu/Makefile.inc b/src/soc/intel/common/block/cpu/Makefile.inc index a6c4f37..1f57598 100644 --- a/src/soc/intel/common/block/cpu/Makefile.inc +++ b/src/soc/intel/common/block/cpu/Makefile.inc @@ -9,5 +9,9 @@ postcar-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CAR) += car/exit_car.S postcar-$(CONFIG_FSP_CAR) += car/exit_car_fsp.S
+ifneq ($(CONFIG_HAVE_POSTCAR),y) +ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CAR) += car/exit_car.S +endif + ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU) += cpulib.c ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CPU_MPINIT) += mp_init.c
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 1: Code-Review-1
Why is the change useful? Is there a customer that don't want postcar? Do you need additional space for FSP? Beside the minor booting time and minor flash size, it only increases code complexity and adds unmaintained code. Also there's no documentation.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
Patch Set 1: Code-Review-1
Why is the change useful?
I hope commit msg has adequate details.
Is there a customer that don't want postcar?
I thought we are working in "open source environment" and should be open for exploratory activity. and yes, FW design should have flexibility to pick and drop it's stages. But looks like on latest socs. Post car is no more an optional stage so system won't boot if soc kconfig just drops POSTCAR_STAGE selection.
Additionally https://review.coreboot.org/c/coreboot/+/34476 comments from Aaron as below
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
Do you need additional space for FSP?
May be yes, sometime we are ending up with FW size being ~16.3MB and due to that recommend user to move to 32 MB SPI, hence its better to optimize the space to see what we need and what we can drop
Beside the minor booting time and minor flash size, it only increases code complexity and adds unmaintained code.
what is "unmaintained " code here, for that matter postcar once added was "unmaintained" code ? but later every soc just migrated into that. Also what is additional "complexity" here, it uses exact same car tear down logic that postcar use. rather tearing down in front of ramstage, we are tearing in ramstage front when postcar not selected
Also there's no documentation.
can you please point me to postcar implementation document so i can "just" add commit msg line there to say it does same work in ramstage in absence of postcar with those additional benefits?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1: Code-Review-1 Do you need additional space for FSP?
May be yes, sometime we are ending up with FW size being ~16.3MB
Hi Subrata, Do you have a coreboot config that results in such a large FW image? I believe postcar is rather small compared to other coreboot parts, so I would like to check if there's something else that could be trimmed down instead. postcar is quite useful as a bridge between romstage and ramstage, so I would prefer if that could be kept. Thanks.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1: Code-Review-1
Why is the change useful?
I hope commit msg has adequate details.
Is there a customer that don't want postcar?
I thought we are working in "open source environment" and should be open for exploratory activity. and yes, FW design should have flexibility to pick and drop it's stages. But looks like on latest socs. Post car is no more an optional stage so system won't boot if soc kconfig just drops POSTCAR_STAGE selection.
Same for other stages like bootblock, romstage, payload ...
It was introduced to get rid of platform specific CAR tear down. I don't see why adding platform specific CAR tear down would help the "open source environment".
As this is an "open source environment", can't you pick and drop FSP, as it's the biggest and slowest? Why do your start with the fastest and slimmest?
Additionally https://review.coreboot.org/c/coreboot/+/34476 comments from Aaron as below
FWIW, putting CAR teardown on the front of ramstage would get rid of the extra stage load. That was my point.
Do you need additional space for FSP?
May be yes, sometime we are ending up with FW size being ~16.3MB and due to that recommend user to move to 32 MB SPI, hence its better to optimize the space to see what we need and what we can drop
Start with FSP?
Beside the minor booting time and minor flash size, it only increases code complexity and adds unmaintained code.
Looking at the MAINTAINERS file Intel doesn't even maintain soc/intel.
what is "unmaintained " code here, for that matter postcar once added was "unmaintained" code ? but later every soc just migrated into that. Also what is additional "complexity" here, it uses exact same car tear down logic that postcar use. rather tearing down in front of ramstage, we are tearing in ramstage front when postcar not selected
It adds more preprocessor code and conditionals, making it harder to read and understand.
Also there's no documentation.
To best postcar documenation we have is in "Documenation/getting_started/architecture.md" Feel free to write a better one.
can you please point me to postcar implementation document so i can "just" add commit msg line there to say it does same work in ramstage in absence of postcar with those additional benefits?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
Guys, remember to remain civil.
Yes, open source software is relatively open to experimentation. No, experimentation isn't immune from having to explain itself. Yes, Intel folks talking up the benefits of open source exploration sounds stale given FSP (where is the source, given that open source is so great?) No, that discussion doesn't belong here.
As far as experiments having to explain themselves:
The postcar phase was only recently introduced, and it helped create clearer boundaries _especially_ in light of FSP (that is, without the FSP1/FSP2 saga, we might have never introduced a postcar stage). Messing with that again needs a better argument than "we want to mess with FSP some more and coreboot makes that inconvenient".
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
I'd like to make a proposal here. It's clear to me that the case for this work is not clear to much of this community. That's fair.
In the Linux world, people do their work in a different repo, evaluate it and, if it looks good, offer it up to the mainstream. That's how we got 9p in: initially, starting in 1998, the pushback was unceasing. By 2007, 9p was in and I doubt it will ever go away; even microsoft uses it now. Subrata, would you be comfortable with just forking coreboot to github.com while we do this work and evaluate it? At some point, if it's something we think the larger community might benefit from, we can come back.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S@42 PS2, Line 42: _start: Would a weakly linked symbol to the beginning of exit_car.S not work? It would avoid having different _start symbols depending on Kconfig options.
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top The stackpointer is now moved twice. Not sure if that can cause issues.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S@42 PS2, Line 42: _start:
Would a weakly linked symbol to the beginning of exit_car. […]
incase we would like to tear down car in ranmstage then ramstage entry point will b exit_car.S rather being conventional c_start.S then avoiding multiple instances of _start for a single stage. Making it weak might also leads into same confusion?
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
The stackpointer is now moved twice. Not sure if that can cause issues.
no, it will be only once, either by postcar or by ramstage based on if postcar is present or not. it won't create overhead because purpose is just to tear down car before continuing
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 2:
@Subrata Thanks for emailing me about this commit. We know that having non public conversion is something you like at Intel, but it's not what we do in coreboot.
Please let us continue writing here. If you prefer emails: coreboot@coreboot.org
From your comments I see that you don't care about flash size or boot times. The "benefits" mentioned in the commit message are more like "side effects". The only purpose of this change is to build without a postcar stage as you pointed out.
The question remains why this change is useful to this "open source environment" or "soc users"? For me it looks more like a working Proof-of-Concept, but nothing that somebody would ever make use of in production.
IMHO reasons against this PoC are (thus giving -1): * it touches to much of common x86 code, which add more work for x86 maintainers as they need to understand and take care of this special use case * It adds a feature that has seems to have no benefit * It breaks the assumption of having a postcar tearing down CAR for the only reason of being able to do so * It doesn't update or add documentation
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, 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/+/34752
to look at the new patch set (#3).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!HAVE_POSTCAR) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB *3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/include/arch/cpu.h A src/arch/x86/ramstage_loader.c M src/cpu/amd/agesa/Kconfig M src/cpu/amd/pi/Kconfig M src/drivers/intel/fsp1_1/Kconfig M src/lib/program.ld M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/mainboard/intel/galileo/Kconfig M src/northbridge/intel/e7505/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i440bx/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/nehalem/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Makefile.inc M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 32 files changed, 155 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
The postcar phase was only recently introduced, and it helped create clearer boundaries _especially_ in light of FSP (that is, without the FSP1/FSP2 saga, we might have never introduced a postcar stage). Messing with that again needs a better argument than "we want to mess with FSP some more and coreboot makes that inconvenient".
To clarify things a bit on this. I put in postcar for the following reasons (not related to FSP at all):
1. romstage path that was loading ramstage dropped into assembly to tear down CAR and then came back into C code. The same c program (romstage) that was linked for running with CAR enabled. This behavior has implications because of the following. 2. Because of the volatility of CAR space there was a necessity for car_get_var() (early_variables.h) API. The CAR data needed to migrate to RAM to maintain its values but it needed to go through that API to steer the load/store to the correct location (RAM vs CAR).
With the introduction of postcar it provides romstage to run to completion without car_get_var() implementation and actually paves the way for complete removal of the API and variable decorators to place globals in the correct section in the linker in CAR space. Instead, these programs act like normal C programs with the .bss section landing in the correct part of the address space. postcar's first order of business is to tear down CAR before it runs C code. Therefore, it provides consistency in all stages/programs of a normal C program of having normal global variables w/o the necessity of car_get_var() API. If all platforms employ postcar currently then we can finally remove early_variables.h entirely (yay!).
As for this change specifically, I haven't looked into the details just yet. A first glance does indicate it's a little messy and could be cleaned up, but it's very much possible to remove postcar and put the CAR teardown at the first part of ramstage. Now whether that's worth it (commit description is 4ms) in comparison to the support cost is another matter that needs to be weighed.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S@42 PS2, Line 42: _start:
incase we would like to tear down car in ranmstage then ramstage entry point will b exit_car. […]
Sorry, I meant a weak symbol that calls the code in exit_car.S where you split off the _start in exit_car.S to a different stub file.
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
no, it will be only once, either by postcar or by ramstage based on if postcar is present or not. […]
The stackpointer is moved here and at L76 in c_start.S "movl $_estack, %esp". If I recall correctly sometimes C code is called to tear down car...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S@42 PS2, Line 42: _start:
Sorry, I meant a weak symbol that calls the code in exit_car. […]
got it, i can try something like and see
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
The stackpointer is moved here and at L76 in c_start.S "movl $_estack, %esp". […]
i don't think we are selecting CONFIG_SOC_SETS_MSRS ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@36 PS3, Line 36: .code64 We can't have the feature of this CL work with RAMSTAGE_X86_64. It would be broken.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@42 PS3, Line 42: _start: Can we get away with #include'ing exit_car.S at this point? Will 2 symbols declared back to back with the same name? There's section mismatch in exit_car.S and line 34 above, but we can fix that. Just wondering how to make this bit less kludgy.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@45 PS3, Line 45: ramstage_start: This doesn't need to be in the else. It's just a symbol.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@48 PS3, Line 48: lgdt %cs:gdtaddr Do we want to load 2 different gdts successively? Or remove the first one in exit_car.S? It at least needs a comment here and exit_car.S to note what's happening and the conclusion for handling the seemingly dual gdt initialization.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... File src/arch/x86/ramstage_loader.c:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... PS3, Line 91: Why is this file essentially duplicated? Can we not modify postcar_loader to handle the 2 different variants? I don't think we should be duplicating the exact same logic with name changes and different timestamp parameters.
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld@100 PS3, Line 100: #if ENV_RMODULE || ENV_POSTCAR || CONFIG(RAMSTAGE_LOADS_FROM_ROMSTAGE) when ramstage is relocatable it already has this section. Why wouldn't we just have RAMSTAGE_LOADS_FROM_ROMSTAGE depend on relocatable ramstage? Currently, you are just getting lucky that things are linked properly because I don't see any changes that enforce such a dependency in this patch.
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... PS3, Line 81: select RAMSTAGE_LOADS_FROM_POSTCAR we wouldn't be switching over all these SoC with this patch. We'd want to evaluate it and enable it separately. Similarly, if this is a direction we want to move why wouldn't it apply every where and nuke postcar all together?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@45 PS3, Line 45: ramstage_start:
This doesn't need to be in the else. It's just a symbol.
agree
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@48 PS3, Line 48: lgdt %cs:gdtaddr
Do we want to load 2 different gdts successively? Or remove the first one in exit_car. […]
yes, we don;t need gdtinit in exit_car.S when we are doing the same from here in case of ramstage tearing down CAR. i will try to get rid of exit_car.S and see what can be done here
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... File src/arch/x86/ramstage_loader.c:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... PS3, Line 91:
Why is this file essentially duplicated? Can we not modify postcar_loader to handle the 2 different […]
yes can be done using some enum used as parameter to know if we wises to load fallback/postcar or fallback/ramstage
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld@100 PS3, Line 100: #if ENV_RMODULE || ENV_POSTCAR || CONFIG(RAMSTAGE_LOADS_FROM_ROMSTAGE)
when ramstage is relocatable it already has this section. […]
agree, i can modify kconfig and make depends on relocatable ramstage
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... PS3, Line 81: select RAMSTAGE_LOADS_FROM_POSTCAR
we wouldn't be switching over all these SoC with this patch. […]
do you misread this as RAMSTAGE_LOADS_FROM_ROMSTAGE ?
i didn't switch anything, its same as previous loading ramstage from postcar.
Just renamed RAMSTAGE_LOADS_FROM_POSTCAR from POSTCAR_STAGE to make it clear about 2 possible option
1. RAMSTAGE_LOADS_FROM_POSTCAR 2. RAMSTAGE_LOADS_FROM_ROMSTAGE
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
i don't think we are selecting CONFIG_SOC_SETS_MSRS ?
Well, if you did then then things would break if there isn't a proper stack selected.
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... PS3, Line 81: select RAMSTAGE_LOADS_FROM_POSTCAR
do you misread this as RAMSTAGE_LOADS_FROM_ROMSTAGE ?
Yes, I did.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
Well, if you did then then things would break if there isn't a proper stack selected.
yes, just wondering isn't that even true for posrcar logic today ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... PS3, Line 81: select RAMSTAGE_LOADS_FROM_POSTCAR
do you misread this as RAMSTAGE_LOADS_FROM_ROMSTAGE ? […]
oh okay, then we are good here
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
yes, just wondering isn't that even true for posrcar logic today ?
Yes, I was thinking this variable was 0'd out but tracked it down after I posted the comment that it defaults to 4KiB.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
Yes, I was thinking this variable was 0'd out but tracked it down after I posted the comment that it […]
got it, i can also look into this tomorrow.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... File src/arch/x86/ramstage_loader.c:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... PS3, Line 91:
yes can be done using some enum used as parameter to know if we wises to load fallback/postcar or fa […]
done here: https://review.coreboot.org/c/coreboot/+/34751/4
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#4).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!HAVE_POSTCAR) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB *3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/include/arch/cpu.h M src/cpu/amd/agesa/Kconfig M src/cpu/amd/pi/Kconfig M src/drivers/intel/fsp1_1/Kconfig M src/lib/program.ld M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/mainboard/intel/galileo/Kconfig M src/northbridge/intel/e7505/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i440bx/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/nehalem/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Makefile.inc M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 31 files changed, 63 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/4
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#5).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!HAVE_POSTCAR) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB *3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/include/arch/cpu.h M src/cpu/amd/agesa/Kconfig M src/cpu/amd/pi/Kconfig M src/drivers/intel/fsp1_1/Kconfig M src/lib/program.ld M src/mainboard/emulation/qemu-i440fx/Kconfig M src/mainboard/emulation/qemu-q35/Kconfig M src/mainboard/intel/galileo/Kconfig M src/northbridge/intel/e7505/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i440bx/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/nehalem/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Makefile.inc M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 30 files changed, 74 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@36 PS3, Line 36: .code64
We can't have the feature of this CL work with RAMSTAGE_X86_64. It would be broken.
Done
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@42 PS3, Line 42: _start:
Can we get away with #include'ing exit_car. […]
Done
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@45 PS3, Line 45: ramstage_start:
agree
Done
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@48 PS3, Line 48: lgdt %cs:gdtaddr
yes, we don;t need gdtinit in exit_car. […]
Done
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld@100 PS3, Line 100: #if ENV_RMODULE || ENV_POSTCAR || CONFIG(RAMSTAGE_LOADS_FROM_ROMSTAGE)
agree, i can modify kconfig and make depends on relocatable ramstage
Done, added depends
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
As for this change specifically, I haven't looked into the details just yet. A first glance does indicate it's a little messy and could be cleaned up, but it's very much possible to remove postcar and put the CAR teardown at the first part of ramstage. Now whether that's worth it (commit description is 4ms) in comparison to the support cost is another matter that needs to be weighed.
When postcar support was added, there was a similar question about being able to run ramstage directly from romstage and have ramstage do the tear down which postcar currently does. IIUC, the reason for not doing that was to avoid coherency issues. Is that not a concern anymore? Reference: https://review.coreboot.org/c/coreboot/+/14140/3//COMMIT_MSG#14
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
Patch Set 5:
As for this change specifically, I haven't looked into the details just yet. A first glance does indicate it's a little messy and could be cleaned up, but it's very much possible to remove postcar and put the CAR teardown at the first part of ramstage. Now whether that's worth it (commit description is 4ms) in comparison to the support cost is another matter that needs to be weighed.
When postcar support was added, there was a similar question about being able to run ramstage directly from romstage and have ramstage do the tear down which postcar currently does. IIUC, the reason for not doing that was to avoid coherency issues. Is that not a concern anymore? Reference: https://review.coreboot.org/c/coreboot/+/14140/3//COMMIT_MSG#14
is that specific to APL? i don't see any such caching related issue so far even on celeron. Might like to hear from Aaron on this ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
I'll prepare patch to set POSTCAR_STAGE as default, that will reduce most of the Kconfig file changes away from this one.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
Patch Set 5:
I'll prepare patch to set POSTCAR_STAGE as default, that will reduce most of the Kconfig file changes away from this one.
yeah, that way we can parallelly do things.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
CB:34803 and CB:34804 not sure if they are complete yet.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
Patch Set 5:
As for this change specifically, I haven't looked into the details just yet. A first glance does indicate it's a little messy and could be cleaned up, but it's very much possible to remove postcar and put the CAR teardown at the first part of ramstage. Now whether that's worth it (commit description is 4ms) in comparison to the support cost is another matter that needs to be weighed.
When postcar support was added, there was a similar question about being able to run ramstage directly from romstage and have ramstage do the tear down which postcar currently does. IIUC, the reason for not doing that was to avoid coherency issues. Is that not a concern anymore? Reference: https://review.coreboot.org/c/coreboot/+/14140/3//COMMIT_MSG#14
I don't think anything has changed there. My response to Patrick was based on me assuming he meant in C. In hindsight that was quite the leap on my part. As long as it's in assembly and executing out of real RAM we should be good. All the prog_segment_loaded() business is sorted for us so I think we should be good.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/c_start.S@42 PS2, Line 42: _start:
got it, i can try something like and see
Done
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/2/src/arch/x86/exit_car.S@48 PS2, Line 48: post_car_stack_top
got it, i can also look into this tomorrow.
Done
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld@100 PS3, Line 100: #if ENV_RMODULE || ENV_POSTCAR || CONFIG(RAMSTAGE_LOADS_FROM_ROMSTAGE)
Done, added depends
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif Can't this be included below at line 45 unconditionally? That was my suggestion. I was also curious if we could define the same symbol twice w/o the assembler complaining. Also the RAMSTAGE_X86_64 is still incompatible with LOADS_FROM_ROMSTAGE on cache as ram employed devices.
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S@37 PS6, Line 37: Incase in case
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S@38 PS6, Line 38: * be handled inside c_start.S hence avoiding dual GDT programming here We'd be reliant on the descriptor cache. If a select is loaded before the ram backed gdt is enabled we would be in trouble. I think it's important to comment on the assumptions.
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/include/arch/c... PS6, Line 355: unnecessary edit
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif
Can't this be included below at line 45 unconditionally? That was my suggestion.
exit_car.S to do car tear down which will be handled by postcar in regular flow, hence keeping #include "exit_car.S" unconditionally below at 45 won't be duplicate if that effort? (in case romstage -> postcar (exit_car.S) -> ramstage (exit_car.S))
I was also curious if we could define the same symbol twice w/o the assembler complaining.
i hope you meant _start symbol?, we can do that but problem is that _start already existed inside exit_car.S hence it will give compilation issue.
1. romstage -> postcar -> ramstage model postcar entry point is exit_car.S ramstage entry point is c_start.S
but 2. romstage -> ramstage model
we need to do tear down hence exit_car.S is added as first entry point now adding c_start.S also add another _start symbol
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S@37 PS6, Line 37: Incase
in case
Done
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/exit_car.S@38 PS6, Line 38: * be handled inside c_start.S hence avoiding dual GDT programming here
We'd be reliant on the descriptor cache. […]
Done
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/include/arch/c... PS6, Line 355:
unnecessary edit
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#7).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 6 files changed, 49 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/7
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#8).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 6 files changed, 49 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/8
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif
Can't this be included below at line 45 unconditionally? That was my suggestion. […]
What I mean is add this stanza to line 45 and remove the conditional around _start and .globl _start. It would result in the following sequence:
.globl _start _start: .globl _start _start:
If the assembler doesn't care then things just work. Does the above cause a compilation problem?
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S@40 PS8, Line 40: * is enabled in c_start.S loaded? you mean no segment selectors used?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S@145 PS8, Line 145: call main Shouldn't this be based on ENV_POSTCAR then? Why leave this part here?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif
What I mean is add this stanza to line 45 and remove the conditional around _start and .globl _start. It would result in the following sequence:
.globl _start _start: .globl _start _start:
If the assembler doesn't care then things just work. Does the above cause a compilation problem?
compiler won't complain unless we select RAMSTAGE_LOADS_FROM_ROMSTAGE from soc. Upon selecting RAMSTAGE_LOADS_FROM_ROMSTAGE config, we will get below error
CC ramstage/arch/x86/c_start.o src/arch/x86/c_start.S: Assembler messages: src/arch/x86/c_start.S:46: Error: symbol `_start' is already defined make: *** [Makefile:357: build/ramstage/arch/x86/c_start.o] Error 1
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S@40 PS8, Line 40: * is enabled in c_start.S
loaded? you mean no segment selectors used?
my bad. added proper comments now
https://review.coreboot.org/c/coreboot/+/34752/8/src/arch/x86/exit_car.S@145 PS8, Line 145: call main
Shouldn't this be based on ENV_POSTCAR then? Why leave this part here?
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#9).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 6 files changed, 57 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/9
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif
What I mean is add this stanza to line 45 and remove the conditional around _start and . […]
Got it. So it does need the guard. Given that, what's the point of the #include here then? It can just be ramstage-$(CONFIG_RAMSTAGE_LOADS_FROM_ROMSTAGE) += exit_car.S since that will contain the _start symbol and the code flow is non-linear anyway.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S@44 PS9, Line 44: executing be executing
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S@146 PS9, Line 146: call ramstage_start I think this should just be a jmp. Once you perform this call the stack is no longer aligned to 16 bytes. C functions expect the calls to be done with 16-byte alignment, but we're just traversing through more assembly. Thoughts?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/6/src/arch/x86/c_start.S@19 PS6, Line 19: #endif
Got it. So it does need the guard. […]
Done
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S File src/arch/x86/exit_car.S:
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S@44 PS9, Line 44: executing
be executing
Done
https://review.coreboot.org/c/coreboot/+/34752/9/src/arch/x86/exit_car.S@146 PS9, Line 146: call ramstage_start
I think this should just be a jmp. […]
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#10).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 7 files changed, 54 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/10
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#12).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 8 files changed, 58 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/12
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#13).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 8 files changed, 75 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/13
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#15).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 8 files changed, 75 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/15
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, build bot (Jenkins), Patrick Georgi, Vanny E, Huang Jin, Lee Leahy, Damien Zammit, David Guckian, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34752
to look at the new patch set (#16).
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
arch/x86: Attempt to boot without postcar stage/phase
This patch provides option for soc to exclude postcar (!RAMSTAGE_LOADS_FROM_ROMSTAGE) stage to avoid an additional stage loading and executing time. This effort has 2 benefits:
1. Save boot time by ~3-4ms 2. Avoid generation of postcar.elf which saves (25kB * 3copies = 75kB) of SPI footprint.
By current design the postcar stage/phase handles the cache-as-ram tear down and as cache-as-ram is volatile and tearing it down leads to its contents disappearing. Therefore provide a shim layer in ramstage (similar to postcar) to perform below operations:
1. Tears down cache-as-ram with a chipset helper function. 2. Loads and runs ramstage.
Because those 2 things are executed out of ram there's no issue of the code's backing store while executing the code that tears down cache-as-ram. The current implementation makes no assumption regarding cacheability of the DRAM itself. If the chipset code wishes to cache DRAM for loading of the ramstage stage/phase then it's also up to the chipset to handle any coherency issues pertaining to cache-as-ram destruction.
With this CL, we will have 2 possible approach to load ramstage 1. romstage -> postcar -> ramstage Referred as RAMSTAGE_LOADS_FROM_POSTCAR (majority of x86 platform selected this from Kconfig)
2. romstage -> ramstage Referred as RAMSTAGE_LOADS_FROM_ROMSTAGE (additional option to load ramstage without introducing intermediate postcar stage)
Change-Id: Ibc88e6727b2fa692585dffe576e69f6d4d0b349d Credit-to: Aaron Durbin adurbin@chromium.org Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/c_start.S M src/arch/x86/exit_car.S M src/arch/x86/postcar_loader.c M src/lib/program.ld M src/soc/intel/common/block/cpu/Makefile.inc 8 files changed, 75 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/34752/16
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 17:
The prior patch only matters if we are still attempting to pursue this patch. Is this patch still something pursue?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34752/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/17/src/Kconfig@1239 PS17, Line 1239: depends on ARCH_X86 && RELOCATABLE_RAMSTAGE This also depends on !x86-64 ramstage since the implication is that ramstage is entered with CAR still enabled.
https://review.coreboot.org/c/coreboot/+/34752/17/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/17/src/lib/program.ld@99 PS17, Line 99: */ I think we should be explicit in the commenting how exit_car.S needs this section for obtaining the stack value. ENV_POSTCAR and RAMSTAGE_LOADS_FROM_ROMSTAGE both need this section because exit_car.S is the stage entry point which needs that information.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 17:
Patch Set 17:
The prior patch only matters if we are still attempting to pursue this patch. Is this patch still something pursue?
i will be waiting for your guidance if you like to make postcar really optional and add ramstage as sama capable to get loaded from romstage directly on x86 ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 17:
Patch Set 17:
Patch Set 17:
The prior patch only matters if we are still attempting to pursue this patch. Is this patch still something pursue?
i will be waiting for your guidance if you like to make postcar really optional and add ramstage as sama capable to get loaded from romstage directly on x86 ?
The original pursuit was to speed boot times up. There were different areas decided on to gain the speed increases. Where do we stand w.r.t. bypassing postcar? With nothing else changing aside from removal of postcar (this and prior patch) what are the numbers to compare? Are they a small net difference that are only achievable by carrying more options/code?
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34752?usp=email )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
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) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)