Change in coreboot[master]: arch/x86: Fix S3 resume without stage cache

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Kyösti Mälkki (Code Review)

12 Dec 2019 12 Dec '19

11:32 p.m.

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

It is possible to have NO_STAGE_CACHE=n and at the same time have TSEG_STAGE_CACHE=n and CBMEB_STAGE_CACHE=n. This resulted with a failing attempt to load STAGE_POSTCAR from the stage cache, but not loading it from CBFS either.

Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/postcar_loader.c 1 file changed, 19 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/37683/1

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index b53cbf8..46a62e8 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -19,6 +19,7 @@ #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> #include <program_loading.h> +#include <reset.h> #include <rmodule.h> #include <romstage_handoff.h> #include <stage_cache.h> @@ -208,6 +209,15 @@ MTRR_TYPE_WRBACK); }

+static void postcar_cache_invalid(void) +{ + if (!CONFIG(TSEG_STAGE_CACHE) && !CONFIG(CBMEM_STAGE_CACHE)) + return; + + printk(BIOS_ERR, "postcar cache invalid.\n"); + board_reset(); +} + void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog = @@ -215,14 +225,19 @@

postcar_commit_mtrrs(pcf);

- if (!CONFIG(NO_STAGE_CACHE) && - romstage_handoff_is_resume()) { + if (romstage_handoff_is_resume()) { stage_cache_load_stage(STAGE_POSTCAR, &prog); + /* This is here to allow platforms to pass different stack parameters between S3 resume and normal boot. On the platforms where the values are the same it's a nop. */ - finalize_load(prog.arg, pcf->stack); - } else + if (prog_entry(&prog) != NULL) + finalize_load(prog.arg, pcf->stack); + else + postcar_cache_invalid(); + } + + if (prog_entry(&prog) == NULL) load_postcar_cbfs(&prog, pcf);

/* As postcar exist, it's end of romstage here */

Attachments:

attachment.htm (text/html — 4.9 KB)

Show replies by date

Marshall Dawson (Code Review)

13 Dec 13 Dec

8:35 p.m.

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37683/2/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/c/coreboot/+/37683/2/src/arch/x86/postcar_loader... PS2, Line 237: postcar_cache_invalid(); Maybe a comment here to indicate that execution falls through via this function in the event there is no stage_cache. Not that it's a bug, just a little harder to follow the scenario of !NO_STAGE_CACHE since the check was removed above.

Mike Banon (Code Review)

16 Dec 16 Dec

9:52 a.m.

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 4: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-Comment-Date: Mon, 16 Dec 2019 08:52:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Paul Menzel (Code Review)

11:25 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37683/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/37683/4//COMMIT_MSG@10 PS4, Line 10: CBMEB CBMEM

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 16 Dec 2019 10:25:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

17 Dec 17 Dec

11:09 p.m.

Hello Mike Banon, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37683

to look at the new patch set (#5).

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c 2 files changed, 20 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/37683/5

Kyösti Mälkki (Code Review)

11:47 p.m.

Hello Mike Banon, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37683

to look at the new patch set (#6).

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

It was possible to have NO_STAGE_CACHE=n and at the same time have TSEG_STAGE_CACHE=n and CBMEB_STAGE_CACHE=n. This resulted with a failing attempt to load STAGE_POSTCAR from the stage cache, but not loading it from CBFS either.

Make it a three-way choice between different STAGE_CACHE options. For S3 resume path, failing to load postcar from enabled stage_cache will reset board.

For AGESA disable CBMEM_STAGE_CACHE by default, as it is no longer needed to have functional ACPI S3 resume and it is not allowed se use keyword select for symbols inside choice blocks.

Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/Kconfig M src/arch/x86/postcar_loader.c M src/cpu/amd/agesa/Kconfig M src/lib/prog_loaders.c 4 files changed, 40 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/37683/6

Kyösti Mälkki (Code Review)

11:49 p.m.

Hello Mike Banon, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37683

to look at the new patch set (#7).

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

It was possible to have NO_STAGE_CACHE=n and at the same time have TSEG_STAGE_CACHE=n and CBMEM_STAGE_CACHE=n. This resulted with a failing attempt to load STAGE_POSTCAR from the stage cache, but not loading it from CBFS either.

Make it a three-way choice between different STAGE_CACHE options. For S3 resume path, failing to load postcar from enabled stage_cache will reset board.

For AGESA disable CBMEM_STAGE_CACHE by default, as it is no longer needed to have functional ACPI S3 resume and it is not allowed se use keyword select for symbols inside choice blocks.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/37683/7

Kyösti Mälkki (Code Review)

11:53 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37683/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/37683/4//COMMIT_MSG@10 PS4, Line 10: CBMEB

...

CBMEM

Done

https://review.coreboot.org/c/coreboot/+/37683/2/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/c/coreboot/+/37683/2/src/arch/x86/postcar_loader... PS2, Line 237: postcar_cache_invalid();

...

Maybe a comment here to indicate that execution falls through via this function in the event there i […]

Unless otherwise noted it is expected for functions to return... And NO_STAGE_CACHE condition is back in there.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 7 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 17 Dec 2019 22:53:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Marshall Dawson marshalldawson3rd@gmail.com Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Aaron Durbin (Code Review)

18 Dec 18 Dec

5:50 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37683/7/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/c/coreboot/+/37683/7/src/arch/x86/postcar_loader... PS7, Line 240: if (prog_entry(&prog) == NULL) Am I correct that this logic is assuming prog_entry(&prog) == NULL under these conditions?

1. Non-resume path 2. Resume with NO_STAGE_CACHE

Can we explicitly handle those conditions in the if above and here? I feel like it's hard to follow the various combinations we're handling. I think the compiler can figure out the duplicate code paths and make it more readable for humans.

if (romstange_handoff_is_resume()) { if (CONFIG(NO_STAGE_CACHE)) { load_postcar_cbfs(); } else { stage_cache_load_stage() finalize_load() if (prog_entry(&prog) == NULL) postcar_cache_invalid(); } } else { load_postcar_cbfs(); }

That seems more straight forward in handling the conditions. I know it's more verbose, but it's easy to follow.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 7 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 18 Dec 2019 16:50:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

7:25 p.m.

Hello Mike Banon, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37683

to look at the new patch set (#8).

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

Make it a three-way choice between different STAGE_CACHE options. For AGESA disable CBMEM_STAGE_CACHE by default, as it is no longer needed to have functional ACPI S3 resume and it is not allowed se use keyword select for symbols inside choice blocks.

Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/Kconfig M src/cpu/amd/agesa/Kconfig 2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/37683/8

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 8 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Kyösti Mälkki (Code Review)

7:26 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37683/7/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/c/coreboot/+/37683/7/src/arch/x86/postcar_loader... PS7, Line 240: if (prog_entry(&prog) == NULL)

...

Am I correct that this logic is assuming prog_entry(&prog) == NULL under these conditions? […]

With the Kconfig change I don't really need to touch this at all.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 8 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 18 Dec 2019 18:26:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

7:36 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 8: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 8 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 18 Dec 2019 18:36:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

19 Dec 19 Dec

8:30 p.m.

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

arch/x86: Fix S3 resume without stage cache

Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37683 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/Kconfig M src/cpu/amd/agesa/Kconfig 2 files changed, 17 insertions(+), 13 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved

diff --git a/src/Kconfig b/src/Kconfig index 25bb450..b78b162 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -274,18 +274,28 @@ wake. When selecting this option the romstage is responsible for determing a stack location to use for loading the ramstage.

+choice + prompt "Stage Cache for ACPI S3 resume" + default NO_STAGE_CACHE if !HAVE_ACPI_RESUME || !RELOCATABLE_RAMSTAGE + default TSEG_STAGE_CACHE if SMM_TSEG + +config NO_STAGE_CACHE + bool "Disabled" + help + Do not save any component in stage cache for resume path. On resume, + all components would be read back from CBFS again. + config TSEG_STAGE_CACHE - bool - default y - depends on !NO_STAGE_CACHE && SMM_TSEG + bool "TSEG" + depends on SMM_TSEG help The option enables stage cache support for platform. Platform can stash copies of postcar, ramstage and raw runtime data inside SMM TSEG, to be restored on S3 resume path.

config CBMEM_STAGE_CACHE - bool "Cache stages in CBMEM" - depends on !NO_STAGE_CACHE && !TSEG_STAGE_CACHE + bool "CBMEM" + depends on !SMM_TSEG help The option enables stage cache support for platform. Platform can stash copies of postcar, ramstage and raw runtime data @@ -297,6 +307,8 @@

If unsure, select 'N'

+endchoice + config UPDATE_IMAGE bool "Update existing coreboot.rom image" help @@ -1153,13 +1165,6 @@ building relocatable modules in the RAM stage. Those modules can be loaded anywhere and all the relocations are handled automatically.

-config NO_STAGE_CACHE - bool - default y if !HAVE_ACPI_RESUME || !RELOCATABLE_RAMSTAGE - help - Do not save any component in stage cache for resume path. On resume, - all components would be read back from CBFS again. - config GENERIC_GPIO_LIB bool help diff --git a/src/cpu/amd/agesa/Kconfig b/src/cpu/amd/agesa/Kconfig index 2c8f9c5..fae2565 100644 --- a/src/cpu/amd/agesa/Kconfig +++ b/src/cpu/amd/agesa/Kconfig @@ -26,7 +26,6 @@ select UDELAY_LAPIC select LAPIC_MONOTONIC_TIMER select SPI_FLASH if HAVE_ACPI_RESUME - select CBMEM_STAGE_CACHE if HAVE_ACPI_RESUME select SMM_ASEG select NO_FIXED_XIP_ROM_SIZE select SSE2

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 9 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

Paul Menzel (Code Review)

26 Apr 26 Apr

10:53 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 9:

Starting from scratch (no `.config`), `make menuconfig` shows this prompt under *General setup*. Is that intended?

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 9 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 26 Apr 2020 08:53:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kyösti Mälkki (Code Review)

12:57 p.m.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 9:

...

Patch Set 9:

Starting from scratch (no `.config`), `make menuconfig` shows this prompt under *General setup*. Is that intended?

I don't have ideas how to improve it from this without implementing TSEG on AGESA and removing CBMEM_STAGE_CACHE.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 9 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 26 Apr 2020 10:57:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Paul Menzel (Code Review)

27 Apr 27 Apr

10:10 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 9:

...

Patch Set 9:

...
Patch Set 9:

Starting from scratch (no `.config`), `make menuconfig` shows this prompt under *General setup*. Is that intended?

I don't have ideas how to improve it from this without implementing TSEG on AGESA and removing CBMEM_STAGE_CACHE.

Should it be user selectable, or could the prompt be hidden?

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 9 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 27 Apr 2020 08:10:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

7:41 p.m.

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37683 )

Change subject: arch/x86: Fix S3 resume without stage cache ......................................................................

Patch Set 9:

...

Patch Set 9:

...
Patch Set 9:

...
Patch Set 9:

Starting from scratch (no `.config`), `make menuconfig` shows this prompt under *General setup*. Is that intended?

I don't have ideas how to improve it from this without implementing TSEG on AGESA and removing CBMEM_STAGE_CACHE.

Should it be user selectable, or could the prompt be hidden?

I think it should be user selectable: i.e - if I remember correctly - there's a boot failure on AMD boards if CBMEM_STAGE_CACHE is enabled while a SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT is also enabled. Since SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT is user selectable, CBMEM_STAGE_CACHE should be too.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0da3e1cf4c92817ffabbb02eda3476ecdfdfa278 Gerrit-Change-Number: 37683 Gerrit-PatchSet: 9 Gerrit-Owner: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Marshall Dawson marshalldawson3rd@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 27 Apr 2020 17:41:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1838

days inactive

1975

days old

coreboot-gerrit@coreboot.org

16 comments

5 participants

tags (0)

participants (5)

Aaron Durbin (Code Review)
Kyösti Mälkki (Code Review)
Marshall Dawson (Code Review)
Mike Banon (Code Review)
Paul Menzel (Code Review)