Change in coreboot[master]: Kconfig: Allow x86 to compress pre-ram stages if not run XIP

List overview All Threads
Download

newer

older

[S] Change in coreboot[master]:...

Arthur Heymans (Code Review)

4 Nov 2019 4 Nov '19

7:17 p.m.

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))

Attachments:

attachment.htm (text/html — 3.6 KB)

Show replies by date

Nico Huber (Code Review)

4 Nov 4 Nov

7:42 p.m.

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 (Code Review)

8:23 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 19:23:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Julius Werner (Code Review)

10:15 p.m.

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).

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 21:15:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Aaron Durbin (Code Review)

11:36 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 22:36:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Arthur Heymans (Code Review)

11:53 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 22:53:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Julius Werner (Code Review)

5 Nov 5 Nov

12:18 a.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 23:18:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Aaron Durbin (Code Review)

12:21 a.m.

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?

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-Comment-Date: Mon, 04 Nov 2019 23:21:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Arthur Heymans (Code Review)

1:17 a.m.

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

Arthur Heymans (Code Review)

30 Nov 30 Nov

6:53 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Marshall Dawson (Code Review)

8:04 p.m.

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'.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 30 Nov 2020 19:04:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Arthur Heymans (Code Review)

8:33 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 30 Nov 2020 19:33:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-MessageType: comment

Arthur Heymans (Code Review)

1 Dec 1 Dec

10:55 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Marshall Dawson (Code Review)

7:04 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 01 Dec 2020 18:04:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Arthur Heymans (Code Review)

8 Apr 8 Apr

2:25 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-MessageType: newpatchset

Arthur Heymans (Code Review)

2 Jun 2 Jun

11:30 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@tutanota.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Comment-Date: Thu, 02 Jun 2022 09:30:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Marshall Dawson marshalldawson3rd@gmail.com Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Arthur Heymans (Code Review)

12:01 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@tutanota.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-MessageType: newpatchset

build bot (Jenkins) (Code Review)

12:02 p.m.

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'?

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@tutanota.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Comment-Date: Thu, 02 Jun 2022 10:02:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

12:08 p.m.

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'?

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@tutanota.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Comment-Date: Thu, 02 Jun 2022 10:08:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Arthur Heymans (Code Review)

12:19 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@tutanota.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-MessageType: newpatchset

Paul Fagerburg (Code Review)

19 Sep 19 Sep

4:57 p.m.

New subject: [S] Change in coreboot[master]: Kconfig: Allow x86 to compress pre-ram stages if not run XIP

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@gmail.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-MessageType: merged

Martin L Roth (Code Review)

20 Sep 20 Sep

11:16 p.m.

New subject: [S] Change in coreboot[master]: Kconfig: Allow x86 to compress pre-ram stages if not run XIP

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 ......................................................................

-- To view, visit https://review.coreboot.org/c/coreboot/+/36613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iac24d243c4bd4cb8c1db14a8e9fc43f508c2cd5d Gerrit-Change-Number: 36613 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Reviewer: Martin L Roth gaumless@gmail.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Sean Rhodes sean@starlabs.systems Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Elyes HAOUAS Gerrit-CC: Elyes Haouas ehaouas@noos.fr Gerrit-CC: Nico Huber nico.h@gmx.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-MessageType: revert

795

days inactive

1846

days old

coreboot-gerrit@coreboot.org

21 comments

8 participants

tags (0)

participants (8)

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)