Change in coreboot[master]: soc/intel/cannonlake/bootblock: Fix FSP CAR

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

soc/intel/cannonlake/bootblock: Fix FSP CAR

Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
Tested on Intel CFL, the new code allows to boot using FSP-T.

Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 14 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/39491/1

diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c
index 4cc15fc..861f2ca 100644
--- a/src/soc/intel/cannonlake/bootblock/bootblock.c
+++ b/src/soc/intel/cannonlake/bootblock/bootblock.c
@@ -23,6 +23,18 @@
 #if CONFIG(FSP_CAR)
 #include <FsptUpd.h>

+/*
+ * Limit CodeRegionSize to 16 MiB as the CAR area is placed right below the memory mapped
+ * BIOS region. A ROM size of 32 MiB will cause wrong MTRRs to be set for the CAR area,
+ * which will brick the platform. Assumes that the memory mapped BIOS region is no more
+ * than 16 MiB.
+ */
+#if CONFIG_ROM_SIZE > 16 * 1024 * 1024
+#define CODE_CACHE_SIZE (16 * 1024 * 1024)
+#else
+#define CODE_CACHE_SIZE CONFIG_ROM_SIZE
+#endif
+
 const FSPT_UPD temp_ram_init_params = {
    .FspUpdHeader = {
    	.Signature = 0x545F4450554C4643ULL,	/* 'CFLUPD_T' */
@@ -44,8 +56,8 @@
    	.MicrocodeRegionBase = 0,
    	.MicrocodeRegionSize = 0,
    	.CodeRegionBase =
-			(uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE),
-		.CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE,
+			(uint32_t)(0x100000000ULL - CODE_CACHE_SIZE,
+		.CodeRegionSize = (uint32_t)CODE_CACHE_SIZE,
    },
 };
 #endif

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 1:

can we set them to 0 and then fast_spi_cache_bios()?

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
File src/soc/intel/cannonlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
PS1, Line 32: #if CONFIG_ROM_SIZE > 16 * 1024 * 1024
            : #define CODE_CACHE_SIZE (16 * 1024 * 1024)
            : #else
            : #define CODE_CACHE_SIZE CONFIG_ROM_SIZE
            : #endif
if run-time determination doesn't work, shall this not be defined by motherboard?

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Johnny Lin, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

soc/intel/cannonlake/bootblock: Fix FSP CAR

Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
Tested on Intel CFL, the new code allows to boot using FSP-T.

Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 6 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/39491/2

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 2: Code-Review+2

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
File src/soc/intel/cannonlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
PS1, Line 32: #if CONFIG_ROM_SIZE > 16 * 1024 * 1024
            : #define CODE_CACHE_SIZE (16 * 1024 * 1024)
            : #else
            : #define CODE_CACHE_SIZE CONFIG_ROM_SIZE
            : #endif
> if run-time determination doesn't work, shall this not be defined by motherboard?

it could be defined by motherboard, but I don't see any issues marking the 16MiB as cached even if less are memory mapped. For simplicity let's stick with CODE_CACHE_SIZE.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/2/src/soc/intel/cannonlake/bo... 
File src/soc/intel/cannonlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/39491/2/src/soc/intel/cannonlake/bo... 
PS2, Line 51: 			(uint32_t)(0x100000000ULL - CACHE_ROM_SIZE,
Does this fit on the previous line?

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Johnny Lin, Angel Pons, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

soc/intel/cannonlake/bootblock: Fix FSP CAR

Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
Tested on Intel CFL, the new code allows to boot using FSP-T.

Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 6 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/39491/3

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/2/src/soc/intel/cannonlake/bo... 
File src/soc/intel/cannonlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/39491/2/src/soc/intel/cannonlake/bo... 
PS2, Line 51: 			(uint32_t)(0x100000000ULL - CACHE_ROM_SIZE,
> Does this fit on the previous line?

Done

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG@9 
PS3, Line 9: Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
What is the problem?

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG@9 
PS3, Line 9: Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
> What is the problem?

That's described on the next line.

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG@9 
PS3, Line 9: Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
> That's described on the next line.

Could we have a more elaborate description on how the old code results in boot failures? It's not obvious.

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG@9 
PS3, Line 9: Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
> Could we have a more elaborate description on how the old code results in boot failures? It's not ob […]

It doesn't boot, no output on serial.

[Patrick Georgi (Code Review)]

Patrick Georgi has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

soc/intel/cannonlake/bootblock: Fix FSP CAR

Fix FSP CAR on platforms that have ROM_SIZE of 32MiB.
CodeRegionSize must be smaller than or equal to 16MiB
to not overlap with LAPIC or the CAR area at 0xfef00000.

Tested on Intel CFL, the new code allows to boot using FSP-T.

Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 6 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/39491/4

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39491/3//COMMIT_MSG@9 
PS3, Line 9: Fix FSP CAR on plaforms that have ROM_SIZE of 32MiB.
> It doesn't boot, no output on serial.

I think the comment in the code is now pretty instructive:
"CodeRegionSize must be smaller than or equal to 16MiB to not overlap with LAPIC or the CAR area at 0xfef00000."

Messing with LAPIC or CAR typically ends in having a bad time.

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
File src/soc/intel/cannonlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/39491/1/src/soc/intel/cannonlake/bo... 
PS1, Line 32: #if CONFIG_ROM_SIZE > 16 * 1024 * 1024
            : #define CODE_CACHE_SIZE (16 * 1024 * 1024)
            : #else
            : #define CODE_CACHE_SIZE CONFIG_ROM_SIZE
            : #endif
> it could be defined by motherboard, but I don't see any issues marking the 16MiB as cached even if l […]

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39491 )

Change subject: soc/intel/cannonlake/bootblock: Fix FSP CAR
......................................................................

soc/intel/cannonlake/bootblock: Fix FSP CAR

Fix FSP CAR on platforms that have ROM_SIZE of 32MiB.
CodeRegionSize must be smaller than or equal to 16MiB
to not overlap with LAPIC or the CAR area at 0xfef00000.

Tested on Intel CFL, the new code allows to boot using FSP-T.

Change-Id: I4dfee230c3cc883fad0cb92977c8f5570e1a927c
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39491
Tested-by: build bot (Jenkins) 
Reviewed-by: Andrey Petrov 
Reviewed-by: Angel Pons 
---
M src/soc/intel/cannonlake/bootblock/bootblock.c
1 file changed, 6 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Andrey Petrov: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c
index 21b8487..73bd81a 100644
--- a/src/soc/intel/cannonlake/bootblock/bootblock.c
+++ b/src/soc/intel/cannonlake/bootblock/bootblock.c
@@ -2,6 +2,7 @@
 /* This file is part of the coreboot project. */

#include <bootblock_common.h>
+#include <cpu/x86/mtrr.h>
 #include <intelblocks/gspi.h>
 #include <intelblocks/uart.h>
 #include <soc/bootblock.h>
@@ -28,12 +29,14 @@
    	 * even before hitting CPU reset vector. Hence skipping FSP-T loading
    	 * microcode after CPU reset by passing '0' value to
    	 * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionSize.
+		 *
+		 * Note: CodeRegionSize must be smaller than or equal to 16MiB to not
+		 * overlap with LAPIC or the CAR area at 0xfef00000.
    	 */
    	.MicrocodeRegionBase = 0,
    	.MicrocodeRegionSize = 0,
-		.CodeRegionBase =
-			(uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE),
-		.CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE,
+		.CodeRegionBase = (uint32_t)0x100000000ULL - CACHE_ROM_SIZE,
+		.CodeRegionSize = (uint32_t)CACHE_ROM_SIZE,
    },
 };
 #endif