Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
arch/x86: Add option to compress postcar stage
The LZ4 decompressor was already linked in romstage.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc 2 files changed, 19 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/1
diff --git a/src/Kconfig b/src/Kconfig index 0d56291..17bf545 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -158,6 +158,16 @@ that decompression might slow down booting if the boot flash is connected through a slow link (i.e. SPI).
+config COMPRESS_POSTCAR + bool "Compress postcar with LZ4" + depends on POSTCAR_STAGE + # Default value set at the end of the file + help + Compress postcar 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. + config COMPRESS_PRERAM_STAGES bool "Compress romstage and verstage with LZ4" depends on !ARCH_X86 && (HAVE_ROMSTAGE || HAVE_VERSTAGE) @@ -1203,6 +1213,9 @@ config COMPRESS_RAMSTAGE default y if !UNCOMPRESSED_RAMSTAGE
+config COMPRESS_POSTCAR + default y + config COMPRESS_PRERAM_STAGES depends on !ARCH_X86 default y diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 447fd57..8c35176 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -278,11 +278,16 @@ $(objcbfs)/postcar.elf: $(objcbfs)/postcar.debug.rmod cp $< $@
+CBFS_POSTCAR_COMPRESS_FLAG := none +ifeq ($(CONFIG_COMPRESS_POSTCAR),y) +CBFS_POSTCAR_COMPRESS_FLAG := lz4 +endif + # Add postcar to CBFS cbfs-files-$(CONFIG_POSTCAR_STAGE) += $(CONFIG_CBFS_PREFIX)/postcar $(CONFIG_CBFS_PREFIX)/postcar-file := $(objcbfs)/postcar.elf $(CONFIG_CBFS_PREFIX)/postcar-type := stage -$(CONFIG_CBFS_PREFIX)/postcar-compression := none +$(CONFIG_CBFS_PREFIX)/postcar-compression := $(CBFS_POSTCAR_COMPRESS_FLAG)
############################################################################### # ramstage
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 1: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 1:
I'm a bit confused about how the LZ4 compression works. Who is doing the decompression? romstage or is it sort of done in place? Then caching becomes something to think about (cbmem is uncached there).
I'm confused because with the work on trying to get non-XIP stages to work, it complained about romstage with the .bss and .data (not sure which one) being too small (probably .car.data discarted by cbfstool -S). But it still complained about .bss for compressing romstage.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 1: Code-Review+1
I'm a bit confused about how the LZ4 compression works. Who is doing the decompression? romstage or is it sort of done in place? Then caching becomes something to think about (cbmem is uncached there).
romstage is doing the decompression. It is done in-place into the memory area that postcar is supposed to be loaded into (i.e. it loads the compressed file to the end of that area so that its end lands at _epostcar, and then it starts decompressing that front-to-back into _postcar so that it overwrites itself. If that memory area is not cached at the time you do this, you're gonna have a bad time.
I'm confused because with the work on trying to get non-XIP stages to work, it complained about romstage with the .bss and .data (not sure which one) being too small (probably .car.data discarted by cbfstool -S). But it still complained about .bss for compressing romstage.
Yes, you need a non-zero BSS for this in-place decompression, because the BSS is not part of the image that's being loaded. The BSS is an area of empty space between the end of the decompressed image (_postcar + uncompressed_size) and the end of the buffer (_epostcar). You need a small "gap" there (usually just a few bytes, but the worst-case bound is 8 + uncompressed_size/255) to make sure it doesn't trample over its own feet when overwriting itself. Normally, our BSSes are big enough that this is not a problem, but if you have no BSS at all you'll run into this (and cbfstool verifies that the image can be decompressed cleanly in the available space, so you get that error at build time).
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 1: Code-Review-2
Patch Set 1: Code-Review+1
I'm a bit confused about how the LZ4 compression works. Who is doing the decompression? romstage or is it sort of done in place? Then caching becomes something to think about (cbmem is uncached there).
romstage is doing the decompression. It is done in-place into the memory area that postcar is supposed to be loaded into (i.e. it loads the compressed file to the end of that area so that its end lands at _epostcar, and then it starts decompressing that front-to-back into _postcar so that it overwrites itself. If that memory area is not cached at the time you do this, you're gonna have a bad time.
It's not cached ATM. Something like CB:34992 could do it however. So the question is whether decompressing to uncached ram is worth the size reduction and therefore slow SPI reads. I guess this needs measuring.
I'm confused because with the work on trying to get non-XIP stages to work, it complained about romstage with the .bss and .data (not sure which one) being too small (probably .car.data discarted by cbfstool -S). But it still complained about .bss for compressing romstage.
Yes, you need a non-zero BSS for this in-place decompression, because the BSS is not part of the image that's being loaded. The BSS is an area of empty space between the end of the decompressed image (_postcar + uncompressed_size) and the end of the buffer (_epostcar). You need a small "gap" there (usually just a few bytes, but the worst-case bound is 8 + uncompressed_size/255) to make sure it doesn't trample over its own feet when overwriting itself. Normally, our BSSes are big enough that this is not a problem, but if you have no BSS at all you'll run into this (and cbfstool verifies that the image can be decompressed cleanly in the available space, so you get that error at build time).
Thanks for clarifying. This explains why I had to use the program.ld for x86 non XIP romstage where .bss is after the program instead of defined separately in car.ld (which gets discarted by cbfstool).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 1:
It's not cached ATM. Something like CB:34992 could do it however. So the question is whether decompressing to uncached ram is worth the size reduction and therefore slow SPI reads. I guess this needs measuring.
Yeah, you can profile it, but in my experience, decompressing in uncached memory is prohibitively slow. I mean, fundamentally, all it does is memcpy() small amounts from the previously written areas over and over again.
Hello Aaron Durbin, Subrata Banik, 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/+/36674
to look at the new patch set (#2).
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
arch/x86: Add option to compress postcar stage
The LZ4 decompressor was already linked in romstage.
This adds a Kconfig symbol ROMSTAGE_CACHED_CBMEM to be selected by flatforms that set up WB caching of where cbmem and stage cache will be. Based on this the default to compress postcar stage is selected.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc M src/cpu/x86/Kconfig 3 files changed, 27 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36674/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/2//COMMIT_MSG@9 PS2, Line 9: was is
https://review.coreboot.org/c/coreboot/+/36674/2//COMMIT_MSG@12 PS2, Line 12: flatforms platforms
https://review.coreboot.org/c/coreboot/+/36674/2//COMMIT_MSG@12 PS2, Line 12: that set setting
https://review.coreboot.org/c/coreboot/+/36674/2//COMMIT_MSG@14 PS2, Line 14: selected. Mention *COMPRESS_POSTCAR*.
Hello Aaron Durbin, Subrata Banik, 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/+/36674
to look at the new patch set (#4).
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
arch/x86: Add option to compress postcar stage
The LZ4 decompressor was already linked in romstage.
This adds a Kconfig symbol ROMSTAGE_CACHED_CBMEM to be selected by flatforms that set up WB caching of where cbmem and stage cache will be. Based on this the default to compress postcar stage is selected.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc M src/cpu/x86/Kconfig 3 files changed, 27 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 6: -Code-Review
TESTED on P5QL-EM (Intel x4x) with cbmem and external stage cache cached by MTRR. Is ~4K smaller and ~2.5ms faster.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to compress postcar stage ......................................................................
Patch Set 8: Code-Review-1
Same problem as CB:37198 (almost no graphics (super distorted) on GRUB screen and no graphics at all in Linux). Here's a bootlog obtained with FT232H - https://pastebin.com/NRXKbSUS , you could compare it with a good log here - CB:41924 (https://pastebin.com/u0RXtQvQ). I wonder if it's a matter of bad parent change or a code issues in this change itself
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Subrata Banik, Julius Werner, Mike Banon, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36674
to look at the new patch set (#9).
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
arch/x86: Add option to LZ4 compress postcar stage
This adds a Kconfig symbol ROMSTAGE_CACHED_CBMEM to be selected by flatforms that set up WB caching of where cbmem and stage cache will be. Based on this the default to compress postcar stage is selected.
The reason for selecting LZ4 over LZMA is that LZ4 has a very low stack requirement in comparison with LZMA which is a better idea for when operating in CAR. Other compression methods might be added later if platforms have enough stack in CAR for it.
The LZ4 decompressor was already linked in romstage.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/Kconfig M src/arch/x86/Makefile.inc M src/cpu/x86/Kconfig 3 files changed, 29 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/9
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@10 PS12, Line 10: f p
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@16 PS12, Line 16: when operating in CAR I don't follow this argument. Why would a higher stack requirement be better in CAR?
https://review.coreboot.org/c/coreboot/+/36674/12/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/36674/12/src/cpu/x86/Kconfig@85 PS12, Line 85: This also needs CLFLUSH to work while in CAR. I'm really confused why we're directly linking ROMSTAGE_CACHED_CBMEM with COMPRESS_POSTCAR. They seem to be very separate notions.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@16 PS12, Line 16: when operating in CAR
I don't follow this argument. […]
LZ4 has a very low stack requirement, so it is better than LZMA in CAR.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@16 PS12, Line 16: when operating in CAR
LZ4 has a very low stack requirement, so it is better than LZMA in CAR.
That's what I thought you were getting at. I think the 'with LZMA which' reads like the latter part of the sentence is referring to LZMA. Could you reword it to make that clearer, please?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 13: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 13: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@15 PS12, Line 15: with to
https://review.coreboot.org/c/coreboot/+/36674/12//COMMIT_MSG@16 PS12, Line 16: when operating in CAR
That's what I thought you were getting at. […]
I stumbled upon that, too. Maybe it's enough to add a comma after LZMA or make two sentences instead
Attention is currently required from: Arthur Heymans. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Add option to LZ4 compress postcar stage ......................................................................
Patch Set 15: Code-Review+1
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/36674/comment/a773641a_b4809bd4 PS15, Line 10: flatforms platforms?
Patchset:
PS15: Thank you for reviving this.
Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), Martin Roth, Paul Menzel, Angel Pons, Subrata Banik, Julius Werner, Mike Banon, Michael Niewöhner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36674
to look at the new patch set (#16).
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
arch/x86: Optionally cache cbmem in romstage
To speed up operation on cbmem and the external stage cache add an early cbmem hook that sets up caching on CBMEM.
Change-Id: I89fdc6066027447bf72968c66e6f5eb5fbb630c7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/cache/cache.c 2 files changed, 66 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/36674/16
Attention is currently required from: Aaron Durbin. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 17:
(1 comment)
File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/36674/comment/90bc5fe3_4b69b435 PS12, Line 85: This also needs CLFLUSH to work while in CAR.
I'm really confused why we're directly linking ROMSTAGE_CACHED_CBMEM with COMPRESS_POSTCAR. […]
Done in next CL.
Attention is currently required from: Paul Menzel, Angel Pons, Arthur Heymans, Michael Niewöhner, Aaron Durbin.
Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 22:
(1 comment)
File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/36674/comment/cc68d548_b9011ce2 PS22, Line 82: [ `]`?
Attention is currently required from: Reka Norman, Paul Menzel, Angel Pons, Michael Niewöhner, Aaron Durbin.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 22:
(1 comment)
File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/36674/comment/3f0e1015_c9d05498 PS22, Line 82: [
`]`?
It's an open interval so that's why. However the formatted print is wrong. It should be base + size.
Attention is currently required from: Reka Norman, Paul Menzel, Arthur Heymans, Michael Niewöhner, Aaron Durbin.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 22: Code-Review+1
(1 comment)
File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/36674/comment/200e3fb5_9f016d47 PS22, Line 82: [
`]`? […]
Ah, we're used to `[a, b)` instead of `[a, b[`. https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_...
Attention is currently required from: Reka Norman, Paul Menzel, Arthur Heymans, Michael Niewöhner, Aaron Durbin.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 22:
(1 comment)
Patchset:
PS22: @arthur lets revive this?
Attention is currently required from: Reka Norman, Paul Menzel, Arthur Heymans, Lean Sheng Tan, Michael Niewöhner, Aaron Durbin.
Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36674 )
Change subject: arch/x86: Optionally cache cbmem in romstage ......................................................................
Patch Set 22: Code-Review+1
(1 comment)
Patchset:
PS22:
@arthur lets revive this?
+1
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)
-
Lean Sheng Tan (Code Review)
-
Michael Niewöhner (Code Review)
-
Mike Banon (Code Review)
-
Paul Menzel (Code Review)
-
Reka Norman (Code Review)
-
Sean Rhodes (Code Review)