Change in coreboot[master]: ACPI S3: Clean up resume path

[Kyösti Mälkki (Code Review)]

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

Change subject: ACPI S3: Clean up resume path
......................................................................

ACPI S3: Clean up resume path

Remove the obscure path in source code, where ACPI S3 resume
was prohibited and acpi_resume() would return and continue
to BS_WRITE_TABLES.

The condition when ACPI S3 would be prohibited needs to be
checked early in romstage already. For the time being, there
has been little interest to have CMOS option to disable
ACPI S3 resume feature.

Change-Id: If5105912759427f94f84d46d1a3141aa75cbd6ef
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/acpi_s3.c
M src/include/acpi/acpi.h
M src/lib/hardwaremain.c
3 files changed, 6 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/42498/1

diff --git a/src/arch/x86/acpi_s3.c b/src/arch/x86/acpi_s3.c
index d4c697e..fef04ac 100644
--- a/src/arch/x86/acpi_s3.c
+++ b/src/arch/x86/acpi_s3.c
@@ -58,11 +58,6 @@

static void acpi_jump_to_wakeup(void *vector)
 {
-	if (!acpi_s3_resume_allowed()) {
-		printk(BIOS_WARNING, "ACPI: S3 resume not allowed.\n");
-		return;
-	}
-
    /* Copy wakeup trampoline in place. */
    memcpy((void *)WAKEUP_BASE, &__wakeup, __wakeup_size);

@@ -77,7 +72,7 @@
 {
 }

-void acpi_resume(void *wake_vec)
+void __noreturn acpi_resume(void *wake_vec)
 {
    if (CONFIG(HAVE_SMI_HANDLER)) {
    	void *gnvs_address = cbmem_find(CBMEM_ID_ACPI_GNVS);
@@ -95,4 +90,6 @@

post_code(POST_OS_RESUME);
    acpi_jump_to_wakeup(wake_vec);
+
+	die("Failed the jump to wakeup vector\n");
 }
diff --git a/src/include/acpi/acpi.h b/src/include/acpi/acpi.h
index cd99899..316a191 100644
--- a/src/include/acpi/acpi.h
+++ b/src/include/acpi/acpi.h
@@ -984,7 +984,7 @@
    acpi_hest_esd_t *esd, u16 type, void *data, u16 len);

/* For ACPI S3 support. */
-void acpi_resume(void *wake_vec);
+void __noreturn acpi_resume(void *wake_vec);
 void mainboard_suspend_resume(void);
 void *acpi_find_wakeup_vector(void);

diff --git a/src/lib/hardwaremain.c b/src/lib/hardwaremain.c
index 4276027..ba02807 100644
--- a/src/lib/hardwaremain.c
+++ b/src/lib/hardwaremain.c
@@ -169,7 +169,9 @@
    if (CONFIG(HAVE_ACPI_RESUME)) {
    	arch_bootstate_coreboot_exit();
    	acpi_resume(wake_vector);
+		/* We will not come back. */
    }
+	die("Failed OS resume\n");

return BS_WRITE_TABLES;
 }

[Aaron Durbin (Code Review)]

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

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42498 )

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 1: Code-Review+1

(3 comments)

https://review.coreboot.org/c/coreboot/+/42498/1/src/arch/x86/acpi_s3.c 
File src/arch/x86/acpi_s3.c:

https://review.coreboot.org/c/coreboot/+/42498/1/src/arch/x86/acpi_s3.c@94 
PS1, Line 94: 	die("Failed the jump to wakeup vector\n");
We could reset here instead, I think. Not sure if we have a generic reset though.

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c 
File src/lib/hardwaremain.c:

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c@174 
PS1, Line 174: 	die("Failed OS resume\n");
Should we reset instead?

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c@176 
PS1, Line 176: 	return BS_WRITE_TABLES;
this statement is now unreachable

[Kyösti Mälkki (Code Review)]

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

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c 
File src/lib/hardwaremain.c:

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c@174 
PS1, Line 174: 	die("Failed OS resume\n");
> Should we reset instead?

If wake_vector above was lost for S3 resume path, we would not reach here but skip to BS_WRITE_TABLES. I might be a little towards system_reset() for these cases.

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c@176 
PS1, Line 176: 	return BS_WRITE_TABLES;
> this statement is now unreachable

Done

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Angel Pons, Aaron Durbin,

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

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

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

Change subject: ACPI S3: Clean up resume path
......................................................................

ACPI S3: Clean up resume path

Remove the obscure path in source code, where ACPI S3 resume
was prohibited and acpi_resume() would return and continue
to BS_WRITE_TABLES.

The condition when ACPI S3 would be prohibited needs to be
checked early in romstage already. For the time being, there
has been little interest to have CMOS option to disable
ACPI S3 resume feature.

Change-Id: If5105912759427f94f84d46d1a3141aa75cbd6ef
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/acpi_s3.c
M src/include/acpi/acpi.h
M src/lib/hardwaremain.c
3 files changed, 6 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/42498/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42498 )

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 2: Code-Review+1

[Aaron Durbin (Code Review)]

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

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42498 )

Change subject: ACPI S3: Clean up resume path
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/42498/1/src/arch/x86/acpi_s3.c 
File src/arch/x86/acpi_s3.c:

https://review.coreboot.org/c/coreboot/+/42498/1/src/arch/x86/acpi_s3.c@94 
PS1, Line 94: 	die("Failed the jump to wakeup vector\n");
> We could reset here instead, I think. Not sure if we have a generic reset though.

Ack

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c 
File src/lib/hardwaremain.c:

https://review.coreboot.org/c/coreboot/+/42498/1/src/lib/hardwaremain.c@174 
PS1, Line 174: 	die("Failed OS resume\n");
> If wake_vector above was lost for S3 resume path, we would not reach here but skip to BS_WRITE_TABLE […]

Ack

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42498 )

Change subject: ACPI S3: Clean up resume path
......................................................................

ACPI S3: Clean up resume path

Remove the obscure path in source code, where ACPI S3 resume
was prohibited and acpi_resume() would return and continue
to BS_WRITE_TABLES.

The condition when ACPI S3 would be prohibited needs to be
checked early in romstage already. For the time being, there
has been little interest to have CMOS option to disable
ACPI S3 resume feature.

Change-Id: If5105912759427f94f84d46d1a3141aa75cbd6ef
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42498
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
Reviewed-by: Angel Pons 
---
M src/arch/x86/acpi_s3.c
M src/include/acpi/acpi.h
M src/lib/hardwaremain.c
3 files changed, 6 insertions(+), 9 deletions(-)

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

diff --git a/src/arch/x86/acpi_s3.c b/src/arch/x86/acpi_s3.c
index 4872c07..2802bd3 100644
--- a/src/arch/x86/acpi_s3.c
+++ b/src/arch/x86/acpi_s3.c
@@ -57,11 +57,6 @@

static void acpi_jump_to_wakeup(void *vector)
 {
-	if (!acpi_s3_resume_allowed()) {
-		printk(BIOS_WARNING, "ACPI: S3 resume not allowed.\n");
-		return;
-	}
-
    /* Copy wakeup trampoline in place. */
    memcpy((void *)WAKEUP_BASE, &__wakeup, __wakeup_size);

@@ -76,7 +71,7 @@
 {
 }

-void acpi_resume(void *wake_vec)
+void __noreturn acpi_resume(void *wake_vec)
 {
    /* Restore GNVS pointer in SMM if found. */
    apm_control(APM_CNT_GNVS_UPDATE);
@@ -86,4 +81,6 @@

post_code(POST_OS_RESUME);
    acpi_jump_to_wakeup(wake_vec);
+
+	die("Failed the jump to wakeup vector\n");
 }
diff --git a/src/include/acpi/acpi.h b/src/include/acpi/acpi.h
index 6e7db17..58e1dbe 100644
--- a/src/include/acpi/acpi.h
+++ b/src/include/acpi/acpi.h
@@ -987,7 +987,7 @@
    acpi_hest_esd_t *esd, u16 type, void *data, u16 len);

/* For ACPI S3 support. */
-void acpi_resume(void *wake_vec);
+void __noreturn acpi_resume(void *wake_vec);
 void mainboard_suspend_resume(void);
 void *acpi_find_wakeup_vector(void);

diff --git a/src/lib/hardwaremain.c b/src/lib/hardwaremain.c
index 4276027..3fe50c9 100644
--- a/src/lib/hardwaremain.c
+++ b/src/lib/hardwaremain.c
@@ -169,9 +169,9 @@
    if (CONFIG(HAVE_ACPI_RESUME)) {
    	arch_bootstate_coreboot_exit();
    	acpi_resume(wake_vector);
+		/* We will not come back. */
    }
-
-	return BS_WRITE_TABLES;
+	die("Failed OS resume\n");
 }

static boot_state_t bs_write_tables(void *arg)