Change in ...coreboot[master]: {arch, console, drivers, include, lib, soc}: Add new features in postcar
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32667
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 16 files changed, 52 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/1
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 79feaad..9258206 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -838,7 +838,9 @@ unsigned long (*acpi_fill_ivrs)(acpi_ivrs_t *ivrs_struct, unsigned long current));
-#if ENV_RAMSTAGE && !defined(__SIMPLE_DEVICE__) +#if (CONFIG(SKIP_RAMSTAGE) ? ((ENV_RAMSTAGE || ENV_POSTCAR) && \ + !defined(__SIMPLE_DEVICE__)) : (ENV_RAMSTAGE && \ + !defined(__SIMPLE_DEVICE__))) void acpi_create_hpet(acpi_hpet_t *hpet); unsigned long acpi_write_hpet(struct device *device, unsigned long start, acpi_rsdp_t *rsdp); diff --git a/src/console/post.c b/src/console/post.c index 236aa8c..6832151 100644 --- a/src/console/post.c +++ b/src/console/post.c @@ -44,7 +44,7 @@
DECLARE_SPIN_LOCK(cmos_post_lock)
-#if ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR) : ENV_RAMSTAGE) void cmos_post_log(void) { u8 code = 0; @@ -125,7 +125,7 @@ post_log_extra(0); } #endif /* CONFIG_CMOS_POST_EXTRA */ -#endif /* ENV_RAMSTAGE */ +#endif /* ENV_RAMSTAGE (or ENV_POSTCAR) if SKIP_RAMSTAGE is enable */
static void cmos_post_code(u8 value) { diff --git a/src/drivers/uart/uart8250io.c b/src/drivers/uart/uart8250io.c index 62671e2..e536ab8 100644 --- a/src/drivers/uart/uart8250io.c +++ b/src/drivers/uart/uart8250io.c @@ -124,7 +124,7 @@ uart8250_tx_flush(uart_platform_base(idx)); }
-#if ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR) : ENV_RAMSTAGE) void uart_fill_lb(void *data) { struct lb_serial serial; diff --git a/src/drivers/uart/uart8250mem.c b/src/drivers/uart/uart8250mem.c index c3dff6a..d2a45ba 100644 --- a/src/drivers/uart/uart8250mem.c +++ b/src/drivers/uart/uart8250mem.c @@ -147,7 +147,7 @@ uart8250_mem_tx_flush(base); }
-#if ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR) : ENV_RAMSTAGE) void uart_fill_lb(void *data) { struct lb_serial serial; diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 273a80d..98bfdfe 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -39,10 +39,17 @@ #define ARCH_STAGE_HAS_BSS_SECTION 1 #endif
-/* Default is that currently ramstage, smm, and rmodules have a heap. */ + #ifndef ARCH_STAGE_HAS_HEAP_SECTION +#if CONFIG(SKIP_RAMSTAGE) +/* Default is that currently postcar, ramstage, smm, and rmodules have a heap. */ +#define ARCH_STAGE_HAS_HEAP_SECTION (ENV_RAMSTAGE || ENV_SMM || ENV_RMODULE || \ + ENV_POSTCAR) +#else +/* Default is that currently ramstage, smm, and rmodules have a heap. */ #define ARCH_STAGE_HAS_HEAP_SECTION (ENV_RAMSTAGE || ENV_SMM || ENV_RMODULE) #endif +#endif
#define STR(x) #x
diff --git a/src/include/rules.h b/src/include/rules.h index ea8335f..47a98ee 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -268,7 +268,8 @@ * be built with simple device model. */
-#if (defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_X86 && (defined(__PRE_RAM__) || ENV_SMM)) \ + : (defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR)) #define __SIMPLE_DEVICE__ #endif
diff --git a/src/include/stddef.h b/src/include/stddef.h index 993d5f0..c9fdb67 100644 --- a/src/include/stddef.h +++ b/src/include/stddef.h @@ -21,9 +21,10 @@
#define NULL ((void *)0)
-/* The devicetree data structures are only mutable in ramstage. All other - stages have a constant devicetree. */ -#if !ENV_RAMSTAGE +/* The devicetree data structures are only mutable in postcar + (if CONFIG_SKIP_RAMSTAGE) and ramstage. All other stages have a constant + devicetree. */ +#if (CONFIG(SKIP_RAMSTAGE) ? (!ENV_RAMSTAGE && !ENV_POSTCAR) : !ENV_RAMSTAGE) #define DEVTREE_EARLY 1 #else #define DEVTREE_EARLY 0 diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 18f6d5d..faafd2d 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -17,7 +17,7 @@ #include <bootmode.h> #include <vendorcode/google/chromeos/chromeos.h>
-#if ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR) : ENV_RAMSTAGE) static int gfx_init_done = -1;
int gfx_get_init_done(void) diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 1a67ad5..374767b 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -307,8 +307,10 @@ imd_region_used(cbmem_get_imd(), baseptr, size); }
-#if ENV_RAMSTAGE || (CONFIG(EARLY_CBMEM_LIST) \ - && (ENV_POSTCAR || ENV_ROMSTAGE)) +#if (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR || \ + (IS_ENABLED(CONFIG_EARLY_CBMEM_LIST) && (ENV_ROMSTAGE))) \ + : (ENV_RAMSTAGE || (CONFIG(EARLY_CBMEM_LIST) \ + && (ENV_POSTCAR || ENV_ROMSTAGE)))) /* * -fdata-sections doesn't work so well on read only strings. They all * get put in the same section even though those strings may never be diff --git a/src/lib/memrange.c b/src/lib/memrange.c index 96d7524..62706c9 100644 --- a/src/lib/memrange.c +++ b/src/lib/memrange.c @@ -47,7 +47,11 @@ range_entry_unlink(&ranges->free_list, r); return r; } +#if CONFIG(SKIP_RAMSTAGE) + if (ENV_RAMSTAGE || ENV_POSTCAR) +#else if (ENV_RAMSTAGE) +#endif return malloc(sizeof(struct range_entry)); return NULL; } diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 3b77712..ed69b37 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -165,7 +165,8 @@ die("Ramstage was not loaded!\n"); }
-#ifdef __RAMSTAGE__ // gc-sections should take care of this +#if (CONFIG(SKIP_RAMSTAGE) ? (defined(__RAMSTAGE__) || defined(__POSTCAR__)) \ + : defined(__RAMSTAGE__)) // gc-sections should take care of this
static struct prog global_payload = PROG_INIT(PROG_PAYLOAD, CONFIG_CBFS_PREFIX "/payload"); diff --git a/src/lib/program.ld b/src/lib/program.ld index 851aa75..ad65494 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 (CONFIG(SKIP_RAMSTAGE) ? (ENV_RAMSTAGE || ENV_POSTCAR) : ENV_RAMSTAGE) . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _pci_drivers = .; KEEP(*(.rodata.pci_driver)); diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index b6330fa..7eaf078 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -47,8 +47,13 @@ #define USE_TIMESTAMP_REGION 0 #endif
+#if CONFIG(SKIP_RAMSTAGE) +/* The cache location will sit in BSS when in ramstage and postcar. */ +#define TIMESTAMP_CACHE_IN_BSS (ENV_RAMSTAGE || ENV_POSTCAR) +#else /* The cache location will sit in BSS when in ramstage. */ #define TIMESTAMP_CACHE_IN_BSS ENV_RAMSTAGE +#endif
#define HAS_CBMEM (ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_POSTCAR)
@@ -299,8 +304,13 @@ if (ts_cbmem_table->base_time == 0) ts_cbmem_table->base_time = ts_cache_table->base_time;
+#if CONFIG(SKIP_RAMSTAGE) + /* Seed the timestamp tick frequency in ramstage and postcar. */ + if (ENV_RAMSTAGE || ENV_POSTCAR) +#else /* Seed the timestamp tick frequency in ramstage. */ if (ENV_RAMSTAGE) +#endif ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz();
/* Cache no longer required. */ @@ -350,6 +360,9 @@ }
ROMSTAGE_CBMEM_INIT_HOOK(timestamp_sync_cache_to_cbmem) +#if CONFIG(SKIP_RAMSTAGE) +POSTCAR_CBMEM_INIT_HOOK(timestamp_sync_cache_to_cbmem) +#endif RAMSTAGE_CBMEM_INIT_HOOK(timestamp_sync_cache_to_cbmem)
/* Provide default timestamp implementation using monotonic timer. */ diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 455b13c..dbec4e9 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -250,7 +250,11 @@ bios_size = ALIGN_UP(bios_size, alignment); base = 4ULL*GiB - bios_size;
+#if CONFIG(SKIP_RAMSTAGE) + if (ENV_RAMSTAGE || ENV_POSTCAR) { +#else if (ENV_RAMSTAGE) { +#endif mtrr_use_temp_range(base, bios_size, type); } else { int mtrr = get_free_var_mtrr(); diff --git a/src/soc/intel/common/block/i2c/i2c.c b/src/soc/intel/common/block/i2c/i2c.c index a99dfea..1a26802 100644 --- a/src/soc/intel/common/block/i2c/i2c.c +++ b/src/soc/intel/common/block/i2c/i2c.c @@ -47,7 +47,7 @@ return EARLY_I2C_BASE(bus); }
-#if !ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (!ENV_RAMSTAGE && !ENV_POSTCAR) : !ENV_RAMSTAGE) static int lpss_i2c_early_init_bus(unsigned int bus) { const struct dw_i2c_bus_config *config; diff --git a/src/soc/intel/common/block/systemagent/systemagent_early.c b/src/soc/intel/common/block/systemagent/systemagent_early.c index 61f14a9..5c6b261 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_early.c +++ b/src/soc/intel/common/block/systemagent/systemagent_early.c @@ -24,7 +24,7 @@
#include "systemagent_def.h"
-#if !ENV_RAMSTAGE +#if (CONFIG(SKIP_RAMSTAGE) ? (!ENV_RAMSTAGE && !ENV_POSTCAR) : !ENV_RAMSTAGE) void bootblock_systemagent_early_init(void) { uint32_t reg;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32667/1/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/32667/1/src/include/memlayout.h@45 PS1, Line 45: /* Default is that currently postcar, ramstage, smm, and rmodules have a heap. */ line over 80 characters
Hello Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32667
to look at the new patch set (#2).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 16 files changed, 53 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/2
Hello Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32667
to look at the new patch set (#3).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 16 files changed, 55 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/3
Hello Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32667
to look at the new patch set (#4).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 16 files changed, 55 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/4
Hello Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32667
to look at the new patch set (#5).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h A src/include/rules_skip_ramstage.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 17 files changed, 338 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/5
Hello Patrick Rudolph, 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/+/32667
to look at the new patch set (#7).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h A src/include/rules_skip_ramstage.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 17 files changed, 92 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/7
Hello Patrick Rudolph, 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/+/32667
to look at the new patch set (#8).
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
{arch, console, drivers, include, lib, soc}: Add new features in postcar
1. Able to create ACPI table 2. Cache located in BSS in postcar as well 3. Load payload from postcar 4. Specific soc/pch related programming 5. mutable devicetree structure in postcar 6. Make postcar stage as !__SIMPLE_DEVICE__ 7. Now postcar has heap.
Change-Id: Ibf160033362920f737385c792d964d47b3f82bd7 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M Makefile.inc M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h A src/include/rules_skip_ramstage.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/systemagent/systemagent_early.c 17 files changed, 77 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32667/8
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
I guess all the SKIP_RAMSTAGE -> ENV_POSTCAR|ENV_RAMSTAGE things really mean "ENV_LAST_STAGE" (ie. in regular coreboot flows, run in ramstage, if ramstage doesn't exist, run in postcar)?
If so, maybe introduce such an ENV_LAST_STAGE? I think it expresses intent rather than those repeated (yet different) convolutions about which path to take when.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Patch Set 8:
I guess all the SKIP_RAMSTAGE -> ENV_POSTCAR|ENV_RAMSTAGE things really mean "ENV_LAST_STAGE" (ie. in regular coreboot flows, run in ramstage, if ramstage doesn't exist, run in postcar)?
If so, maybe introduce such an ENV_LAST_STAGE? I think it expresses intent rather than those repeated (yet different) convolutions about which path to take when.
I agree. I like the idea of ENV_LAST_STAGE.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
As for Aaron's concerns about postcar bloat: Maybe it's more future proof to strip down ramstage (and make all its features available à la carte)? I guess the main issue there is that full blown pci resource allocation is one of the big ticket items to avoid, and without that, everything else falls apart as well, so maybe not much is gained by going that way?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Patch Set 8:
As for Aaron's concerns about postcar bloat: Maybe it's more future proof to strip down ramstage (and make all its features available à la carte)? I guess the main issue there is that full blown pci resource allocation is one of the big ticket items to avoid, and without that, everything else falls apart as well, so maybe not much is gained by going that way?
I think stripping down ramstage might work someday, but we need to know what we need to know first, in terms of what a minimal ramstage is; all while the ramstage continues to grow. Plus, no matter how you cut it, a stripped down ramstage is still something you have to load, vs. a slightly bigger postcar.
I'd like to try this experiment, though maybe not with this approach.
I liked Patrick's idea of "ENV_LAST_STAGE", which is then set by various checks in rules.h. I'm not a big fan of rules_skip_ramstage.h, since now people have two places to look; is there some compelling reason that makes this new file required? I think you could condition CONFIG_SKIP_RAMSTAGE on x86 maybe, and then the test on architecture in rules_skip_ramstage.h is not needed?
coreboot has traditionally tried to be friendly to experiment, and I think this is one case where it makes sense. I don't see the point of denying the ability of people to run without a ramstage if that's what they need. It's fine if people don't like it or want to use it; I don't see why we would deny this type of change to those people who want to use it. Further, subrata has shown, by measurement, that it provides a significant improvement in boot times.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
(3 comments)
can you rethink this a bit. First off, consider ENV_PAYLOAD_LOADER as the variable, as it's a bit more indicative of why we're doing this. That can replace every single one of your # ? : instances. Also fix that up in rules.h Also shouldn't the guard be systemagent_early.c be ENV_BOOTBLOCK, since you only call that in the bootblock?
https://review.coreboot.org/#/c/32667/8/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/#/c/32667/8/src/lib/timestamp.c@50 PS8, Line 50: #if CONFIG(SKIP_RAMSTAGE) I think this is not workable. I think you could have a variable called ENV_PAYLOADLOADER that is more indicative of what we're after, namely, this is the stage that loads the payload, whatever stage it is, and then all this complexity would be much reduced.
https://review.coreboot.org/#/c/32667/8/src/lib/timestamp.c@307 PS8, Line 307: #if CONFIG(SKIP_RAMSTAGE) we could then define ENV_RAMPAYLOAD, maybe, which is really what this is about: a RAMPAYLOAD which skips the RAMSTAGE.
https://review.coreboot.org/#/c/32667/8/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/#/c/32667/8/src/soc/intel/common/block/systemage... PS8, Line 27: #if (CONFIG(SKIP_RAMSTAGE) ? (!ENV_RAMSTAGE && !ENV_POSTCAR) : !ENV_RAMSTAGE) so is this run in EVERY stage before ramstage or should it be #if ENV_BOOTBLOCK?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
I don't like the fact that CONFIG_SKIP_RAMSTAGE not only skips ramstage, but also includes a bunch of code and guarding logic to postcar stage. It's more like a CONFIG_COMBINED_POSTCAR_RAMSTAGE_LITE
Up to this point no numbers where presented, how much slower a reduced ramstage would be (a quick hacked version which doesn't run PCI or FSP-S). Of course ramstage size matters here, but the linkers ability of dead code elemination should help a lot. Also adding more code to postcar will increase loading times, too.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Patch Set 8:
As for Aaron's concerns about postcar bloat: Maybe it's more
future proof to strip down ramstage (and make all its features available à la carte)? I guess the main issue there is that full blown pci resource allocation is one of the big ticket items to avoid, and without that, everything else falls apart as well, so maybe not much is gained by going that way?
I think stripping down ramstage might work someday, but we need to know what we need to know first, in terms of what a minimal ramstage is; all while the ramstage continues to grow. Plus, no matter how you cut it, a stripped down ramstage is still something you have to load, vs. a slightly bigger postcar.
I'd like to try this experiment, though maybe not with this approach.
I liked Patrick's idea of "ENV_LAST_STAGE", which is then set by various checks in rules.h. I'm not a big fan of rules_skip_ramstage.h, since now people have two places to look; is there some compelling reason that makes this new file required? I think you could condition CONFIG_SKIP_RAMSTAGE on x86 maybe, and then the test on architecture in rules_skip_ramstage.h is not needed?
[Subrata] it appears to me that rules.h don't allow to have CONFIG_SKIP_RAMSTAGE condition and it was giving some compilation error specifically on AMD boards. [looks like rules.h was included from makefile.inc to create agesa library]
Error log, hence i had to add rules_skip_ramstage.h to undef __SIMPLE_DEVICE__
https://qa.coreboot.org/job/coreboot-gerrit/93615/testReport/junit/board/chr...
coreboot has traditionally tried to be friendly to experiment, and I think this is one case where it makes sense. I don't see the point of denying the ability of people to run without a ramstage if that's what they need. It's fine if people don't like it or want to use it; I don't see why we would deny this type of change to those people who want to use it. Further, subrata has shown, by measurement, that it provides a significant improvement in boot times.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
ToT coreboot cbmem -t:
58 entries total:
0:1st timestamp 28,997 5:start of verified boot 59,403 (30,406) 503:starting to initialize TPM 59,987 (584) 504:finished TPM initialization 60,094 (106) 505:starting to verify keyblock/preamble (RSA) 63,125 (3,030) 506:finished verifying keyblock/preamble (RSA) 148,052 (84,927) 507:starting to verify body (load+SHA2+RSA) 148,064 (11) 508:finished loading body (ignore for x86) 288,557 (140,493) 509:finished calculating body hash (SHA2) 410,462 (121,904) 510:finished verifying body signature (RSA) 426,881 (16,418) 511:starting TPM PCR extend 429,045 (2,164) 512:finished TPM PCR extend 429,046 (0) 513:starting locking TPM 429,047 (0) 514:finished locking TPM 429,047 (0) 6:end of verified boot 429,060 (12) 13:starting to load romstage 429,119 (58) 14:finished loading romstage 429,121 (2) 1:start of romstage 429,190 (68) 950:calling FspMemoryInit 438,394 (9,204) 951:returning from FspMemoryInit 551,295 (112,900) 4:end of romstage 587,286 (35,991) 100:<unknown> 597,604 (10,317) 101:<unknown> 597,606 (2) 8:starting to load ramstage 598,386 (779) 15:starting LZMA decompress (ignore for x86) 598,400 (13) 16:finished LZMA decompress (ignore for x86) 736,436 (138,036) 9:finished loading ramstage 779,321 (42,884) 550:starting to load Chrome OS VPD 779,502 (181) 10:start of ramstage 779,988 (485) 30:device enumeration 916,447 (136,459) 954:calling FspSiliconInit 924,800 (8,352) 955:returning from FspSiliconInit 1,114,294 (189,494) 40:device configuration 1,138,774 (24,480) 956:calling FspNotify(AfterPciEnumeration) 1,177,546 (38,771) 957:returning from FspNotify(AfterPciEnumeration) 1,177,974 (428) 50:device enable 1,178,222 (248) 60:device initialization 1,194,583 (16,360) 70:device setup done 1,280,600 (86,017) 75:cbmem post 1,281,286 (686) 80:write tables 1,281,418 (131) 15:starting LZMA decompress (ignore for x86) 1,284,560 (3,142) 16:finished LZMA decompress (ignore for x86) 1,285,556 (996) 85:finalize chips 1,286,638 (1,081) 90:load payload 1,325,515 (38,877) 15:starting LZMA decompress (ignore for x86) 1,326,060 (544) 16:finished LZMA decompress (ignore for x86) 1,544,987 (218,927) 958:calling FspNotify(ReadyToBoot) 1,546,570 (1,582) 959:returning from FspNotify(ReadyToBoot) 1,554,394 (7,824) 960:calling FspNotify(EndOfFirmware) 1,554,520 (125) 961:returning from FspNotify(EndOfFirmware) 1,624,736 (70,215) 99:selfboot jump 1,625,628 (892) 1000:depthcharge start 1,625,682 (53) 1002:RO vboot init 1,630,847 (5,165) 1020:vboot select&load kernel 1,630,849 (2) 1030:finished EC verification 1,635,775 (4,925) 1040:finished storage device initialization 1,663,408 (27,632) 1050:finished reading kernel from disk 1,681,402 (17,993) 1100:finished vboot kernel verification 2,000,071 (318,669) 1101:jumping to kernel 2,002,723 (2,652)
Total Time: 1,973,697
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
with CB: 30985 cbmem -t
37 entries total:
0:1st timestamp 28,792 5:start of verified boot 59,199 (30,407) 503:starting to initialize TPM 59,784 (584) 504:finished TPM initialization 59,866 (82) 505:starting to verify keyblock/preamble (RSA) 62,903 (3,036) 506:finished verifying keyblock/preamble (RSA) 148,276 (85,372) 507:starting to verify body (load+SHA2+RSA) 148,289 (12) 508:finished loading body (ignore for x86) 311,813 (163,524) 509:finished calculating body hash (SHA2) 453,402 (141,588) 510:finished verifying body signature (RSA) 470,011 (16,608) 511:starting TPM PCR extend 472,178 (2,167) 512:finished TPM PCR extend 472,179 (1) 513:starting locking TPM 472,179 (0) 514:finished locking TPM 472,180 (0) 6:end of verified boot 472,193 (12) 13:starting to load romstage 472,289 (96) 14:finished loading romstage 472,292 (2) 1:start of romstage 472,361 (68) 950:calling FspMemoryInit 480,841 (8,480) 951:returning from FspMemoryInit 592,131 (111,289) 4:end of romstage 721,443 (129,312) 100:<unknown> 731,798 (10,354) 954:calling FspSiliconInit 861,728 (129,930) 955:returning from FspSiliconInit 1,051,814 (190,085) 15:starting LZMA decompress (ignore for x86) 1,070,589 (18,774) 16:finished LZMA decompress (ignore for x86) 1,072,417 (1,827) 90:load payload 1,095,997 (23,580) 15:starting LZMA decompress (ignore for x86) 1,097,051 (1,054) 16:finished LZMA decompress (ignore for x86) 1,316,628 (219,576) 99:selfboot jump 1,318,655 (2,026) 1000:depthcharge start 1,318,702 (46) 1002:RO vboot init 1,323,894 (5,191) 1020:vboot select&load kernel 1,323,896 (2) 1030:finished EC verification 1,330,224 (6,328) 1040:finished storage device initialization 1,331,398 (1,174) 1050:finished reading kernel from disk 1,356,585 (25,187) 1100:finished vboot kernel verification 1,676,738 (320,153) 1101:jumping to kernel 1,679,446 (2,708)
Total Time: 1,650,654
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Total savings : 323,043 (~320ms)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Answering size related questions:
ToT Coreboot:
bootblock: 59.1kB romstage: 67.6kB postcar: 24.0 kB ramstage: 243.2kB
with CB: 30985
bootblock: 59.1kB romstage: 67.6kB postcar: 102.0kB
[ignoring ramstage as we are not loading ramstage separately]
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Patch Set 8:
Answering size related questions:
ToT Coreboot:
bootblock: 59.1kB romstage: 67.6kB postcar: 24.0 kB ramstage: 243.2kB
with CB: 30985
bootblock: 59.1kB romstage: 67.6kB postcar: 102.0kB
[ignoring ramstage as we are not loading ramstage separately]
Thanks for providing real numbers! Looking at the timestamp ramstage booting time could be reduced by 330msec if you skip (or minimize the code that is run):
FspNotify(EndOfFirmware) 70msec device enumeration 136msec device configuration 24msec device initialization 16msec device setup done 86msec
This seems to be the same amount you gain from moving all the code to postcar.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Patch Set 8:
Patch Set 8:
Answering size related questions:
ToT Coreboot:
bootblock: 59.1kB romstage: 67.6kB postcar: 24.0 kB ramstage: 243.2kB
with CB: 30985
bootblock: 59.1kB romstage: 67.6kB postcar: 102.0kB
[ignoring ramstage as we are not loading ramstage separately]
Thanks for providing real numbers! Looking at the timestamp ramstage booting time could be reduced by 330msec if you skip (or minimize the code that is run):
[Subrata] yes. right now CB:30985 only incorporate the skip ramstage logic but i remember that i had also execute something that you mentioned, like not calling everything from ramstage, just jump into ramstage and call payload_init() function to load minimum stuff to boot OS.
FspNotify(EndOfFirmware) 70msec device enumeration 136msec device configuration 24msec device initialization 16msec device setup done 86msec
This seems to be the same amount you gain from moving all the code to postcar.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32667 )
Change subject: {arch, console, drivers, include, lib, soc}: Add new features in postcar ......................................................................
Abandoned
-
build bot (Jenkins) (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)