Change in coreboot[master]: AGESA,binaryPI: Fix stack location on entry to romstage

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

Hello Michał Żygowski,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

AGESA,binaryPI: Fix stack location on entry to romstage

For BSP CPU, setup stack location to match the symbol
from car.ld. For AP CPUs the stack is located outside
_car_region and is currently not accounted for in the
linker scripts.

Change-Id: I0ec84ae4e73ecca5034f799cdc2a5c1056ad8b74
Signed-off-by: Michał Żygowski 
Signed-off-by: Kyösti Mälkki 
---
M src/drivers/amd/agesa/cache_as_ram.S
M src/drivers/amd/agesa/romstage.c
M src/include/cpu/amd/car.h
3 files changed, 27 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37351/1

diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S
index 4f1cbea..8d5db98 100644
--- a/src/drivers/amd/agesa/cache_as_ram.S
+++ b/src/drivers/amd/agesa/cache_as_ram.S
@@ -22,6 +22,7 @@
  */

#include "gcccar.inc"
+#include <cpu/x86/lapic_def.h>
 #include <cpu/x86/post_code.h>

.code32
@@ -36,6 +37,19 @@

AMD_ENABLE_STACK

+	/*
+	 * Setup bootblock stack on BSP.
+	 * AMD_ENABLE_STACK macro sets up a stack for BSP at BSP_STACK_BASE_ADDR
+	 * which is 0x30000 (_car_region_end), but for C bootblock the stack
+	 * begins at _ecar_stack (see arch/x86/car.ld)
+	 */
+	mov	$LAPIC_BASE_MSR, %ecx
+	rdmsr
+	test	$LAPIC_BASE_MSR_BOOTSTRAP_PROCESSOR, %eax
+	jz	ap_entry
+
+	mov	$_ecar_stack, %esp
+
    /* Align the stack. */
    and	$0xFFFFFFF0, %esp

@@ -47,11 +61,19 @@
    pushl	%eax
    call	romstage_main

-	/* Should never see this postcode */
-	post_code(0xae)
+	/* Never reached. */

stop:
+	post_code(POST_DEAD_CODE)
    hlt
    jmp	stop

+ap_entry:
+	/* Align the stack for call to ap_romstage_main() */
+	and	$0xfffffff0, %esp
+	call	ap_romstage_main
+
+	/* Never reached. */
+	jmp	stop
+
 _cache_as_ram_setup_end:
diff --git a/src/drivers/amd/agesa/romstage.c b/src/drivers/amd/agesa/romstage.c
index 8460035..571397f 100644
--- a/src/drivers/amd/agesa/romstage.c
+++ b/src/drivers/amd/agesa/romstage.c
@@ -39,7 +39,7 @@
    agesa_set_interface(cb);
 }

-static void bsp_romstage_main(void)
+asmlinkage void romstage_main(unsigned long bist)
 {
    struct postcar_frame pcf;
    struct sysinfo romstage_state;
@@ -99,7 +99,7 @@
    /* We do not return. */
 }

-static void __noreturn ap_romstage_main(void)
+asmlinkage void ap_romstage_main(void)
 {
    struct sysinfo romstage_state;
    struct sysinfo *cb = &romstage_state;
@@ -116,11 +116,3 @@
    /* Not reached. */
    halt();
 }
-
-asmlinkage void romstage_main(unsigned long bist)
-{
-	if (boot_cpu())
-		bsp_romstage_main();
-	else
-		ap_romstage_main();
-}
diff --git a/src/include/cpu/amd/car.h b/src/include/cpu/amd/car.h
index 7e2fccd..f57ea82 100644
--- a/src/include/cpu/amd/car.h
+++ b/src/include/cpu/amd/car.h
@@ -4,5 +4,6 @@
 #include <arch/cpu.h>

asmlinkage void romstage_main(unsigned long bist);
+asmlinkage void ap_romstage_main(void);

#endif

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37351 )

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
PS1, Line 46: 	mov	$LAPIC_BASE_MSR, %ecx
Shouldn't SOC_AMD_COMMON_BLOCK_CAR have the same fix? Arthur already pointed that in my previous patch and you probably intended to fix that in yours

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
PS1, Line 46: 	mov	$LAPIC_BASE_MSR, %ecx
> Shouldn't SOC_AMD_COMMON_BLOCK_CAR have the same fix? Arthur already pointed that in my previous pat […]

SilverBack/AMD showed no interest to fix it for several months now. I believe I identified moving the sback might break their car.ld layout, so their production branch (read-only bootblocks) could not adopt the change.

The implementation in soc/amd relies on (undocumented) modifications to vendorcode so I am just ignoring SOC_AMD_COMMON_BLOCK_CAR. Not sure how we are going to deal with AP CAR teardowns :/.

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37351 )

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
PS1, Line 46: 	mov	$LAPIC_BASE_MSR, %ecx
> SilverBack/AMD showed no interest to fix it for several months now. […]

Nothing we can do about it then, I guess. Isn't AP car teardown done by AGESA? What is the big deal here (sorry I am a little bit unaware)?

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
PS1, Line 46: 	mov	$LAPIC_BASE_MSR, %ecx
> Nothing we can do about it then, I guess. […]

CB:24999 is all I know

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37351 )

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/1/src/drivers/amd/agesa/cache... 
PS1, Line 46: 	mov	$LAPIC_BASE_MSR, %ecx
> CB:24999 is all I know

Ack

[Paul Menzel (Code Review)]

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG@9 
PS2, Line 9: setup
Verb is spelled with a space: set up

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG@9 
PS2, Line 9: For BSP CPU, setup stack location to match the symbol
           : from car.ld. For AP CPUs the stack is located outside
           : _car_region and is currently not accounted for in the
           : linker scripts.
Allowed text width is 75 characters.

https://review.coreboot.org/c/coreboot/+/37351/2/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/2/src/drivers/amd/agesa/cache... 
PS2, Line 41: Setup
Set up

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37351 )

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 2:

This looks like some old cold I did not worked with. That said, only Stoney Ridge (Google) has CAR tear down moved outside of AGESA, all previous CPU had it done within AGESA.

[Angel Pons (Code Review)]

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 2: Code-Review+2

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

Hello Aaron Durbin, Angel Pons, Marshall Dawson, Richard Spiegel, build bot (Jenkins), Michał Żygowski,

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

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

AGESA,binaryPI: Fix stack location on entry to romstage

For BSP CPU, set up stack location to match the symbol from car.ld.
For AP CPUs the stack is located outside _car_region and is currently
not accounted for in the linker scripts.

Change-Id: I0ec84ae4e73ecca5034f799cdc2a5c1056ad8b74
Signed-off-by: Michał Żygowski 
Signed-off-by: Kyösti Mälkki 
---
M src/drivers/amd/agesa/cache_as_ram.S
M src/drivers/amd/agesa/romstage.c
M src/include/cpu/amd/car.h
3 files changed, 27 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37351/3

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

Hello Aaron Durbin, Angel Pons, Marshall Dawson, Richard Spiegel, build bot (Jenkins), Michał Żygowski,

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

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

AGESA,binaryPI: Fix stack location on entry to romstage

For BSP CPU, set up stack location to match the symbol from car.ld.
For AP CPUs the stack is located outside _car_region and is currently
not accounted for in the linker scripts.

Change-Id: I0ec84ae4e73ecca5034f799cdc2a5c1056ad8b74
Signed-off-by: Michał Żygowski 
Signed-off-by: Kyösti Mälkki 
---
M src/drivers/amd/agesa/cache_as_ram.S
M src/drivers/amd/agesa/romstage.c
M src/include/cpu/amd/car.h
3 files changed, 27 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37351/4

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG@9 
PS2, Line 9: setup
> Verb is spelled with a space: set up

Done

https://review.coreboot.org/c/coreboot/+/37351/2//COMMIT_MSG@9 
PS2, Line 9: For BSP CPU, setup stack location to match the symbol
           : from car.ld. For AP CPUs the stack is located outside
           : _car_region and is currently not accounted for in the
           : linker scripts.
> Allowed text width is 75 characters.

Done

https://review.coreboot.org/c/coreboot/+/37351/2/src/drivers/amd/agesa/cache... 
File src/drivers/amd/agesa/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37351/2/src/drivers/amd/agesa/cache... 
PS2, Line 41: Setup
> Set up

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37351 )

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

Patch Set 4: Code-Review+2

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

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

Change subject: AGESA,binaryPI: Fix stack location on entry to romstage
......................................................................

AGESA,binaryPI: Fix stack location on entry to romstage

For BSP CPU, set up stack location to match the symbol from car.ld.
For AP CPUs the stack is located outside _car_region and is currently
not accounted for in the linker scripts.

Change-Id: I0ec84ae4e73ecca5034f799cdc2a5c1056ad8b74
Signed-off-by: Michał Żygowski 
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37351
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/drivers/amd/agesa/cache_as_ram.S
M src/drivers/amd/agesa/romstage.c
M src/include/cpu/amd/car.h
3 files changed, 27 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S
index 4f1cbea..e86830f 100644
--- a/src/drivers/amd/agesa/cache_as_ram.S
+++ b/src/drivers/amd/agesa/cache_as_ram.S
@@ -22,6 +22,7 @@
  */

#include "gcccar.inc"
+#include <cpu/x86/lapic_def.h>
 #include <cpu/x86/post_code.h>

.code32
@@ -36,6 +37,19 @@

AMD_ENABLE_STACK

+	/*
+	 * Set up bootblock stack on BSP.
+	 * AMD_ENABLE_STACK macro sets up a stack for BSP at BSP_STACK_BASE_ADDR
+	 * which is 0x30000 (_car_region_end), but for C bootblock the stack
+	 * begins at _ecar_stack (see arch/x86/car.ld)
+	 */
+	mov	$LAPIC_BASE_MSR, %ecx
+	rdmsr
+	test	$LAPIC_BASE_MSR_BOOTSTRAP_PROCESSOR, %eax
+	jz	ap_entry
+
+	mov	$_ecar_stack, %esp
+
    /* Align the stack. */
    and	$0xFFFFFFF0, %esp

@@ -47,11 +61,19 @@
    pushl	%eax
    call	romstage_main

-	/* Should never see this postcode */
-	post_code(0xae)
+	/* Never reached. */

stop:
+	post_code(POST_DEAD_CODE)
    hlt
    jmp	stop

+ap_entry:
+	/* Align the stack for call to ap_romstage_main() */
+	and	$0xfffffff0, %esp
+	call	ap_romstage_main
+
+	/* Never reached. */
+	jmp	stop
+
 _cache_as_ram_setup_end:
diff --git a/src/drivers/amd/agesa/romstage.c b/src/drivers/amd/agesa/romstage.c
index 8460035..571397f 100644
--- a/src/drivers/amd/agesa/romstage.c
+++ b/src/drivers/amd/agesa/romstage.c
@@ -39,7 +39,7 @@
    agesa_set_interface(cb);
 }

-static void bsp_romstage_main(void)
+asmlinkage void romstage_main(unsigned long bist)
 {
    struct postcar_frame pcf;
    struct sysinfo romstage_state;
@@ -99,7 +99,7 @@
    /* We do not return. */
 }

-static void __noreturn ap_romstage_main(void)
+asmlinkage void ap_romstage_main(void)
 {
    struct sysinfo romstage_state;
    struct sysinfo *cb = &romstage_state;
@@ -116,11 +116,3 @@
    /* Not reached. */
    halt();
 }
-
-asmlinkage void romstage_main(unsigned long bist)
-{
-	if (boot_cpu())
-		bsp_romstage_main();
-	else
-		ap_romstage_main();
-}
diff --git a/src/include/cpu/amd/car.h b/src/include/cpu/amd/car.h
index 7e2fccd..f57ea82 100644
--- a/src/include/cpu/amd/car.h
+++ b/src/include/cpu/amd/car.h
@@ -4,5 +4,6 @@
 #include <arch/cpu.h>

asmlinkage void romstage_main(unsigned long bist);
+asmlinkage void ap_romstage_main(void);

#endif