Change in coreboot[master]: Kconfig: Allow x86 to compress pre-ram stages if not run XIP
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc 2 files changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/1
diff --git a/src/Kconfig b/src/Kconfig index 793927a..df86246 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -160,7 +160,7 @@
config COMPRESS_PRERAM_STAGES bool "Compress romstage and verstage with LZ4" - depends on !ARCH_X86 && (HAVE_ROMSTAGE || HAVE_VERSTAGE) + depends on HAVE_ROMSTAGE || HAVE_VERSTAGE # Default value set at the end of the file help Compress romstage and (if it exists) verstage with LZ4 to save flash @@ -1217,7 +1217,7 @@ default y if !UNCOMPRESSED_RAMSTAGE
config COMPRESS_PRERAM_STAGES - depends on !ARCH_X86 + depends on NO_XIP_EARLY_STAGES default y
config INCLUDE_CONFIG_FILE diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 447fd57..db4f054 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -119,6 +119,7 @@ ifeq ($(CONFIG_C_ENVIRONMENT_BOOTBLOCK),y)
bootblock-y += bootblock_crt0.S +bootblock-y += memmove.c
ifeq ($(CONFIG_ARCH_BOOTBLOCK_X86_32),y) $(eval $(call early_x86_stage,bootblock,elf32-i386))
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@169 PS1, Line 169: (assume all : ARCH_X86 for now) needs update
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES I'm not sure but doesn't this and the dependencies above turn into `HAVE_ROMSTAGE || HAVE_VERSTAGE || NO_XIP_EARLY_STAGES`.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
I'm not sure but doesn't this and the dependencies above turn into […]
I'm not sure it's worth the overhead in bootblock and all stages needing to carry compression code along. Check out the case statement in cbfs_load_and_decompress()(src/lib/cbfs.c) in handling this support. Have you run the size comparisons of what this does from a code and cbfs footprint? Also, there is some baked-in policy in that file for what compression algos are supported. Makefile/Kconfig need to match.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
I'm not sure it's worth the overhead in bootblock and all stages needing to carry compression code a […]
LZ4 is really tiny (on the order of a few hundred bytes). Even with a 20k romstage and an 80% compression ratio, you're coming out way ahead (and real numbers should be much better than that).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
LZ4 is really tiny (on the order of a few hundred bytes). […]
Everything's already baked that LZ4 is only compression scheme to use for early stages? Looks that way:
ifeq ($(CONFIG_COMPRESS_PRERAM_STAGES),y) CBFS_PRERAM_COMPRESS_FLAG:=LZ4 endif
Numbers would be good for comparison purposes.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
LZ4 is really tiny (on the order of a few hundred bytes). Even with a 20k romstage and an 80% compression ratio, you're coming out way ahead (and real numbers should be much better than that).
Sizewise it indeed seem like a big win. Any idea how compute intensive decompressing is?
Everything's already baked that LZ4 is only compression scheme to use for early stages? Looks that way:
ifeq ($(CONFIG_COMPRESS_PRERAM_STAGES),y) CBFS_PRERAM_COMPRESS_FLAG:=LZ4 endif
Numbers would be good for comparison purposes.
In bytes
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303
non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203
non compressed: - romstage: 13924
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
Sizewise it indeed seem like a big win. Any idea how compute intensive decompressing is?
It should be barely measurable (e.g. a millisecond or less), LZ4 is super fast. It should always be a win compared to loading more bytes over SPI. The only exceptions would be really pathological cases like running on uncached memory or with a super low CPU clock or something like that.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/1/src/Kconfig@1220 PS1, Line 1220: depends on NO_XIP_EARLY_STAGES
Sizewise it indeed seem like a big win. Any idea how compute intensive decompressing is? […]
Arthur could you please put those sizes into the commit description?
Hello 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/+/36613
to look at the new patch set (#2).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36613
to look at the new patch set (#3).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: succesfully boot the up/squared.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/3/src/Kconfig@147 PS3, Line 147: depends on (HAVE_ROMSTAGE || HAVE_VERSTAGE) && NO_XIP_EARLY_STAGES It looks like you need to replicate the depends on at line 1293. Otherwise CONFIG_COMPRESS_PRERAM_STAGES is getting defaulted to 'y'.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36613/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/3/src/Kconfig@147 PS3, Line 147: depends on (HAVE_ROMSTAGE || HAVE_VERSTAGE) && NO_XIP_EARLY_STAGES
It looks like you need to replicate the depends on at line 1293. Otherwise CONFIG_COMPRESS_PRERAM_STAGES is getting defaulted to 'y'.
nice catch. thanks.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36613
to look at the new patch set (#4).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: succesfully boot the up/squared.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc 2 files changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/4
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 4: Code-Review+1
Hello build bot (Jenkins), Martin Roth, Marshall Dawson, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36613
to look at the new patch set (#5).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: succesfully boot the up/squared.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/5
Attention is currently required from: Marshall Dawson. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 5:
(1 comment)
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/36613/comment/72a3b16c_49d9ee78 PS3, Line 147: depends on (HAVE_ROMSTAGE || HAVE_VERSTAGE) && NO_XIP_EARLY_STAGES
It looks like you need to replicate the depends on at line 1293. […]
Done
Attention is currently required from: Marshall Dawson. Hello build bot (Jenkins), Martin L Roth, Marshall Dawson, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36613
to look at the new patch set (#6).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: succesfully boot the up/squared & google/vilboz.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/6
Attention is currently required from: Marshall Dawson. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 6:
(1 comment)
Commit Message:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-150484): https://review.coreboot.org/c/coreboot/+/36613/comment/b50d0081_ae710e9b PS6, Line 26: 'succesfully' may be misspelled - perhaps 'successfully'?
Attention is currently required from: Marshall Dawson. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Patch Set 7:
(1 comment)
Commit Message:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-150487): https://review.coreboot.org/c/coreboot/+/36613/comment/107cd586_23ff900e PS7, Line 26: 'succesfully' may be misspelled - perhaps 'successfully'?
Attention is currently required from: Marshall Dawson. Hello build bot (Jenkins), Martin L Roth, Marshall Dawson, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36613
to look at the new patch set (#8).
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: successfully boot the up/squared & google/vilboz.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/36613/8
Paul Fagerburg has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
Kconfig: Allow x86 to compress pre-ram stages if not run XIP
On the intel/glkrvp compressed: - romstage: 29659 - verstage: 31303 non compressed: - romstage: 46244 - verstage: 47012
On qemu (with some additional patch to not run XIP) compressed: - romstage: 11203 non compressed: - romstage: 13924
Even with a small romstage the size improvements are substantial, which should result in a speedup when loading the stage. On the up/squared loading romstage is sped up by 9ms.
TESTED: successfully boot the up/squared & google/vilboz.
Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36613 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sean Rhodes sean@starlabs.systems --- M src/Kconfig 1 file changed, 37 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Sean Rhodes: Looks good to me, approved
diff --git a/src/Kconfig b/src/Kconfig index 0d3879e..38126fa 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -173,14 +173,14 @@
config COMPRESS_PRERAM_STAGES bool "Compress romstage and verstage with LZ4" - depends on !ARCH_X86 && (HAVE_ROMSTAGE || HAVE_VERSTAGE) + depends on (HAVE_ROMSTAGE || HAVE_VERSTAGE) && NO_XIP_EARLY_STAGES # Default value set at the end of the file help Compress romstage and (if it exists) verstage with LZ4 to save flash space and speed up boot, since the time for reading the image from SPI (and in the vboot case verifying it) is usually much greater than the - time spent decompressing. Doesn't work for XIP stages (assume all - ARCH_X86 for now) for obvious reasons. + time spent decompressing. Doesn't work for XIP stages for obvious + reasons.
config COMPRESS_BOOTBLOCK bool @@ -1370,7 +1370,7 @@ default y if !UNCOMPRESSED_RAMSTAGE
config COMPRESS_PRERAM_STAGES - depends on !ARCH_X86 + depends on (HAVE_ROMSTAGE || HAVE_VERSTAGE) && NO_XIP_EARLY_STAGES default y
config INCLUDE_CONFIG_FILE
Martin L Roth has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/36613 )
Change subject: Kconfig: Allow x86 to compress pre-ram stages if not run XIP ......................................................................
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Marshall Dawson (Code Review)
-
Martin L Roth (Code Review)
-
Nico Huber (Code Review)
-
Paul Fagerburg (Code Review)