Change in coreboot[master]: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region

APEI (ACPI Platform Error Interface) defines BERT (Boot Error Record
Table) memory region:
* Bootloader (firmware) generates UEFI CPER (Common Platform Error Record)
records, and populates BERT region. In case of FSP 2.x, coreboot
gets BERT region raw data from FSP (PREV_BOOT_ERROR_SRC_HOB).
* OS parses ACPI BERT table, finds the BERT region address, inteprets
the data and processes it accordingly.

When CONFIG_ACPI_BERT is defined, update FSP UPD BootLoaderTolumSize, so
FSP allocates memory region for it. The APEI BERT region is placed on top
of CBMEM, for the size of CONFIG_ACPI_BERT_SIZE.

Change-Id: Ie72240e4c5fa01fcf937d33678c40f9ca826487a
Signed-off-by: Jonathan Zhang 
---
M src/drivers/intel/fsp2_0/cbmem.c
M src/drivers/intel/fsp2_0/hob_verify.c
M src/drivers/intel/fsp2_0/memory_init.c
3 files changed, 21 insertions(+), 5 deletions(-)

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

diff --git a/src/drivers/intel/fsp2_0/cbmem.c b/src/drivers/intel/fsp2_0/cbmem.c
index 0efb462..f836b36 100644
--- a/src/drivers/intel/fsp2_0/cbmem.c
+++ b/src/drivers/intel/fsp2_0/cbmem.c
@@ -6,7 +6,10 @@
 void *cbmem_top_chipset(void)
 {
    struct range_entry tolum;
+	void *tolum_base;

fsp_find_bootloader_tolum(&tolum);
-	return (void *)(uintptr_t)range_entry_end(&tolum);
+	tolum_base = (void *)(uintptr_t)range_entry_base(&tolum);
+
+	return tolum_base + cbmem_overhead_size();
 }
diff --git a/src/drivers/intel/fsp2_0/hob_verify.c b/src/drivers/intel/fsp2_0/hob_verify.c
index ec526e8..94fc653e 100644
--- a/src/drivers/intel/fsp2_0/hob_verify.c
+++ b/src/drivers/intel/fsp2_0/hob_verify.c
@@ -43,9 +43,7 @@
    	die("Space between FSP reserved region and BIOS TOLUM!\n");
    }

-	if (range_entry_end(&tolum) != (uintptr_t)cbmem_top()) {
-		printk(BIOS_CRIT, "TOLUM end: 0x%08llx != %p: cbmem_top\n",
+	if (range_entry_end(&tolum) != (uintptr_t)cbmem_top())
+		printk(BIOS_DEBUG, "TOLUM end: 0x%08llx != %p: cbmem_top\n",
    		range_entry_end(&tolum), cbmem_top());
-		die("Space between cbmem_top and BIOS TOLUM!\n");
-	}
 }
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 07c4463..1ef9324 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -276,6 +276,21 @@
    /* Reserve enough memory under TOLUD to save CBMEM header */
    arch_upd->BootLoaderTolumSize = cbmem_overhead_size();

+	/*
+	 * If ACPI APEI BERT region size is defined, reserve memory for it.
+	 * +------------------------+ range_entry_top(tolum)
+	 * | Other reserved regions |
+	 * | APEI BERT region       |
+	 * +------------------------+ cbmem_top()
+	 * | CBMEM IMD ROOT         |
+	 * | CBMEM IMD SMALL        |
+	 * +------------------------+ range_entry_base(tolum), TOLUM
+	 * | CBMEM FSP MEMORY       |
+	 * | Other CBMEM regions... |
+	 */
+	if (CONFIG(ACPI_BERT))
+		arch_upd->BootLoaderTolumSize += CONFIG_ACPI_BERT_SIZE;
+
    /* Fill common settings on behalf of chipset. */
    if (fsp_fill_common_arch_params(arch_upd, s3wake, fsp_version,
    				memmap) != CB_SUCCESS)

[Aaron Durbin (Code Review)]

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

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
File src/drivers/intel/fsp2_0/cbmem.c:

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 9: 	void *tolum_base;
Let's avoid void * arithmetic if we can. Just use uint8_t *?

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 14: 	return tolum_base + cbmem_overhead_size();
Please add a comment why range_entry_end() cannot be used.

[Marshall Dawson (Code Review)]

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

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

Patch Set 1:

This looks workable to me.  Once it lands, I think we can remove the assignment in:
https://review.coreboot.org/plugins/gitiles/coreboot/+/292afef2fbb5eaf46dd3e...
and treat the UPD as deprecated.

Then consolidate bert_reserved_region() in CB:45392 and in picasso/memmap.c into drivers//fsp2_0 after that.

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG@7 
PS1, Line 7: ask
Use

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG@20 
PS1, Line 20: 
I am confused by this commit message. Some explanation of why this needs to be allocated this way would be helpful.

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
File src/drivers/intel/fsp2_0/cbmem.c:

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 15: }
It doesn't seem like cbmem_top_chipset() should change. It would be useful to to have a *bert_base() to be used and not have bert and chipset code assume bert is above cbmem. Also used in the hob_verify.c to checkit's location and size.

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/ho... 
File src/drivers/intel/fsp2_0/hob_verify.c:

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/ho... 
PS1, Line 47: 		printk(BIOS_DEBUG, "TOLUM end: 0x%08llx != %p: cbmem_top\n",
It seems that this should do the correct check for tolum, cbmem_top, and bert.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

Patch Set 1:
> Patch Set 1:
>
> This looks workable to me.  Once it lands, I think we can remove the assignment in:
> https://review.coreboot.org/plugins/gitiles/coreboot/+/292afef2fbb5eaf46dd3e...
> and treat the UPD as deprecated.
>
> Then consolidate bert_reserved_region() in CB:45392 and in picasso/memmap.c into drivers//fsp2_0 after that.

Thanks Marshall for the feedback. I did not realize that AMD picasso has FSP and conforms to FSP2.0 spec. What about AMD's server processors?

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: ask FSP to allocate APEI BERT memory region
......................................................................

Patch Set 1:

(6 comments)

Thanks for the feedbacks!

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG@7 
PS1, Line 7: ask
> Use

Done

https://review.coreboot.org/c/coreboot/+/45391/1//COMMIT_MSG@20 
PS1, Line 20:
> I am confused by this commit message. […]

Done

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
File src/drivers/intel/fsp2_0/cbmem.c:

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 9: 	void *tolum_base;
> Let's avoid void * arithmetic if we can. […]

Done

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 14: 	return tolum_base + cbmem_overhead_size();
> Please add a comment why range_entry_end() cannot be used.

Done

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/cb... 
PS1, Line 15: }
> It doesn't seem like cbmem_top_chipset() should change. […]

In order to support RAS, a new type of memory region needs to be added. Such memory region holds data such as boot time error record and run time error record. The memory region needs to be accessible by both firmware and OS, but reserved from regular OS usage.
CBMEM cannot be used for this, as CBMEM is added as type 16 as "configuration table". OS (bert and hest) drivers can not access data in CBMEM.
Hence we reserve this memory region together with CBMEM meta data through FSP. CBMEM metadata has to bee next to the rest of CBMEM data, so this (BERT) memory region needs to be on top of CBMEM region. In this memory map arrangement, bert_base is the same as cbmem_top.

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/ho... 
File src/drivers/intel/fsp2_0/hob_verify.c:

https://review.coreboot.org/c/coreboot/+/45391/1/src/drivers/intel/fsp2_0/ho... 
PS1, Line 47: 		printk(BIOS_DEBUG, "TOLUM end: 0x%08llx != %p: cbmem_top\n",
> It seems that this should do the correct check for tolum, cbmem_top, and bert.

Done

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), John Looney, Subrata Banik, Angel Pons, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Marc Jones, Jason Glenesk, Marshall Dawson, Rocky Phagura, ron minnich, Felix Held,

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

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

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

APEI (ACPI Platform Error Interface) defines BERT (Boot Error Record
Table) memory region:
* Bootloader (firmware) generates UEFI CPER (Common Platform Error Record)
records, and populates BERT region. In case of FSP 2.x, coreboot
gets BERT region raw data from FSP (PREV_BOOT_ERROR_SRC_HOB).
* OS parses ACPI BERT table, finds the BERT region address, inteprets
the data and processes it accordingly.

When CONFIG_ACPI_BERT is defined, update FSP UPD BootLoaderTolumSize, so
FSP allocates memory region for it. The APEI BERT region is placed on top
of CBMEM, for the size of CONFIG_ACPI_BERT_SIZE.

Apart from APEI BERT region, we also have plan to add APEI HEST region
which holds OS runtime hardware error record, based on firmware
first hardware error handling model. HEST region will be reserved same way
as BERT region.

Note that CBMEM region can not be used for such purpose, the OS (bert/hest)
drivers are not able to access data held in CBMEM region, as CBMEM is
set as type 16 (configuration table).

Another option is to reserve the BERT region under CBMEM. We do not
know the size of CBMEM till acpi tables are set up. On the other hand,
BERT region needs to be filled up before ACPI BERT table is finalized.
This approach is unnecessarily sophisticated.

Change-Id: Ie72240e4c5fa01fcf937d33678c40f9ca826487a
Signed-off-by: Jonathan Zhang 
---
M src/drivers/intel/fsp2_0/cbmem.c
M src/drivers/intel/fsp2_0/hob_verify.c
M src/drivers/intel/fsp2_0/memory_init.c
3 files changed, 32 insertions(+), 2 deletions(-)

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

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Angel Pons (Code Review)]

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/45391/2//COMMIT_MSG@11 
PS2, Line 11: * Bootloader (firmware) generates UEFI CPER (Common Platform Error Record)
nit: wrap commit message lines at 72 characters

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), John Looney, Angel Pons, Subrata Banik, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Marc Jones, Jason Glenesk, Marshall Dawson, Rocky Phagura, ron minnich, Felix Held,

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

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

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

APEI (ACPI Platform Error Interface) defines BERT (Boot Error Record
Table) memory region:
* Bootloader (firmware) generates UEFI CPER (Common Platform Error
Record) records, and populates BERT region. In case of FSP 2.x,
oreboot gets BERT region raw data from FSP (PREV_BOOT_ERROR_SRC_HOB).
* OS parses ACPI BERT table, finds the BERT region address, inteprets
the data and processes it accordingly.

When CONFIG_ACPI_BERT is defined, update FSP UPD BootLoaderTolumSize,
so FSP allocates memory region for it. The APEI BERT region is placed
on top of CBMEM, for the size of CONFIG_ACPI_BERT_SIZE.

Apart from APEI BERT region, we also have plan to add APEI HEST region
which holds OS runtime hardware error record, based on firmware
first hardware error handling model. HEST region will be reserved
same way as BERT region.

Note that CBMEM region can not be used for such purpose, the OS
(bert/hest) drivers are not able to access data held in CBMEM region,
as CBMEM is set as type 16 (configuration table).

Another option is to reserve the BERT region under CBMEM. We do not
know the size of CBMEM till acpi tables are set up. On the other hand,
BERT region needs to be filled up before ACPI BERT table is finalized.
This approach is unnecessarily sophisticated.

Change-Id: Ie72240e4c5fa01fcf937d33678c40f9ca826487a
Signed-off-by: Jonathan Zhang 
---
M src/drivers/intel/fsp2_0/cbmem.c
M src/drivers/intel/fsp2_0/hob_verify.c
M src/drivers/intel/fsp2_0/memory_init.c
3 files changed, 32 insertions(+), 2 deletions(-)

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

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Marshall Dawson (Code Review)]

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 3:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@13 
PS3, Line 13: oreboot 
nit: maybe this typo happened with the reflow.

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@12 
PS3, Line 12: In case of FSP 2.x,
            : oreboot gets BERT region raw data from FSP (PREV_BOOT_ERROR_SRC_HOB).
I would put this info about getting the raw data from FSP into the commit message for your "generate" patch CB:45392.  This one only deals with the memory reservation.

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@30 
PS3, Line 30: Another option is to reserve the BERT region under CBMEM. 
nit: maybe "An option considered was...  However..."

https://review.coreboot.org/c/coreboot/+/45391/3/src/drivers/intel/fsp2_0/ho... 
File src/drivers/intel/fsp2_0/hob_verify.c:

https://review.coreboot.org/c/coreboot/+/45391/3/src/drivers/intel/fsp2_0/ho... 
PS3, Line 52: 
remove extra line

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 3:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@13 
PS3, Line 13: oreboot
> nit: maybe this typo happened with the reflow.

Done

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@12 
PS3, Line 12: In case of FSP 2.x,
            : oreboot gets BERT region raw data from FSP (PREV_BOOT_ERROR_SRC_HOB).
> I would put this info about getting the raw data from FSP into the commit message for your "generate […]

Done

https://review.coreboot.org/c/coreboot/+/45391/3//COMMIT_MSG@30 
PS3, Line 30: Another option is to reserve the BERT region under CBMEM.
> nit: maybe "An option considered was...  However... […]

Done

https://review.coreboot.org/c/coreboot/+/45391/3/src/drivers/intel/fsp2_0/ho... 
File src/drivers/intel/fsp2_0/hob_verify.c:

https://review.coreboot.org/c/coreboot/+/45391/3/src/drivers/intel/fsp2_0/ho... 
PS3, Line 52:
> remove extra line

Done

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), John Looney, Angel Pons, Subrata Banik, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Marc Jones, Jason Glenesk, Marshall Dawson, Rocky Phagura, ron minnich, Felix Held,

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

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

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

APEI (ACPI Platform Error Interface) defines BERT (Boot Error Record
Table) memory region:
* Bootloader (firmware) generates UEFI CPER (Common Platform Error
Record) records, and populates BERT region.
* OS parses ACPI BERT table, finds the BERT region address, inteprets
the data and processes it accordingly.

When CONFIG_ACPI_BERT is defined, update FSP UPD BootLoaderTolumSize,
so FSP allocates memory region for it. The APEI BERT region is placed
on top of CBMEM, for the size of CONFIG_ACPI_BERT_SIZE.

Apart from APEI BERT region, we also have plan to add APEI HEST region
which holds OS runtime hardware error record, based on firmware
first hardware error handling model. HEST region will be reserved
same way as BERT region.

Note that CBMEM region can not be used for such purpose, the OS
(bert/hest) drivers are not able to access data held in CBMEM region,
as CBMEM is set as type 16 (configuration table).

An option considered was to reserve the BERT region under CBMEM.
However, we do not know the size of CBMEM till acpi tables are set up.
On the other hand, BERT region needs to be filled up before ACPI BERT
table is finalized.

Change-Id: Ie72240e4c5fa01fcf937d33678c40f9ca826487a
Signed-off-by: Jonathan Zhang 
---
M src/drivers/intel/fsp2_0/cbmem.c
M src/drivers/intel/fsp2_0/hob_verify.c
M src/drivers/intel/fsp2_0/memory_init.c
3 files changed, 31 insertions(+), 2 deletions(-)

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

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/45391/2//COMMIT_MSG@11 
PS2, Line 11: * Bootloader (firmware) generates UEFI CPER (Common Platform Error Record)
> nit: wrap commit message lines at 72 characters

Done

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45391 )

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 4:

Thanks for the review. Just a friendly ping.

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Angel Pons (Code Review)]

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

[Angel Pons (Code Review)]

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

Change subject: drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region
......................................................................

drivers/intel/fsp2_0: use FSP to allocate APEI BERT memory region

APEI (ACPI Platform Error Interface) defines BERT (Boot Error Record
Table) memory region:
* Bootloader (firmware) generates UEFI CPER (Common Platform Error
Record) records, and populates BERT region.
* OS parses ACPI BERT table, finds the BERT region address, inteprets
the data and processes it accordingly.

When CONFIG_ACPI_BERT is defined, update FSP UPD BootLoaderTolumSize,
so FSP allocates memory region for it. The APEI BERT region is placed
on top of CBMEM, for the size of CONFIG_ACPI_BERT_SIZE.

Apart from APEI BERT region, we also have plan to add APEI HEST region
which holds OS runtime hardware error record, based on firmware
first hardware error handling model. HEST region will be reserved
same way as BERT region.

Note that CBMEM region can not be used for such purpose, the OS
(bert/hest) drivers are not able to access data held in CBMEM region,
as CBMEM is set as type 16 (configuration table).

An option considered was to reserve the BERT region under CBMEM.
However, we do not know the size of CBMEM till acpi tables are set up.
On the other hand, BERT region needs to be filled up before ACPI BERT
table is finalized.

Change-Id: Ie72240e4c5fa01fcf937d33678c40f9ca826487a
Signed-off-by: Jonathan Zhang 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45391
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/drivers/intel/fsp2_0/cbmem.c
M src/drivers/intel/fsp2_0/hob_verify.c
M src/drivers/intel/fsp2_0/memory_init.c
3 files changed, 31 insertions(+), 2 deletions(-)

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

diff --git a/src/drivers/intel/fsp2_0/cbmem.c b/src/drivers/intel/fsp2_0/cbmem.c
index 0efb462..5388b89 100644
--- a/src/drivers/intel/fsp2_0/cbmem.c
+++ b/src/drivers/intel/fsp2_0/cbmem.c
@@ -6,7 +6,14 @@
 void *cbmem_top_chipset(void)
 {
    struct range_entry tolum;
+	uint8_t *tolum_base;

fsp_find_bootloader_tolum(&tolum);
-	return (void *)(uintptr_t)range_entry_end(&tolum);
+	tolum_base = (uint8_t *)(uintptr_t)range_entry_base(&tolum);
+
+	/*
+	 * The TOLUM range may have other memory regions (such as APEI
+	 * BERT region on top of CBMEM (IMD root and IMD small) region.
+	 */
+	return tolum_base + cbmem_overhead_size();
 }
diff --git a/src/drivers/intel/fsp2_0/hob_verify.c b/src/drivers/intel/fsp2_0/hob_verify.c
index ec526e8..9bfb0f1 100644
--- a/src/drivers/intel/fsp2_0/hob_verify.c
+++ b/src/drivers/intel/fsp2_0/hob_verify.c
@@ -43,9 +43,16 @@
    	die("Space between FSP reserved region and BIOS TOLUM!\n");
    }

-	if (range_entry_end(&tolum) != (uintptr_t)cbmem_top()) {
+	if (!CONFIG(ACPI_BERT) && range_entry_end(&tolum) != (uintptr_t)cbmem_top()) {
    	printk(BIOS_CRIT, "TOLUM end: 0x%08llx != %p: cbmem_top\n",
    		range_entry_end(&tolum), cbmem_top());
    	die("Space between cbmem_top and BIOS TOLUM!\n");
    }
+
+	if (CONFIG(ACPI_BERT) &&
+		range_entry_end(&tolum) != (uintptr_t)cbmem_top() + CONFIG_ACPI_BERT_SIZE) {
+		printk(BIOS_CRIT, "TOLUM end: 0x%08llx != %p: cbmem_top + 0x%x: BERT\n",
+			range_entry_end(&tolum), cbmem_top(), CONFIG_ACPI_BERT_SIZE);
+		die("Space between cbmem_top and APEI BERT!\n");
+	}
 }
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 07c4463..1ef9324 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -276,6 +276,21 @@
    /* Reserve enough memory under TOLUD to save CBMEM header */
    arch_upd->BootLoaderTolumSize = cbmem_overhead_size();

+	/*
+	 * If ACPI APEI BERT region size is defined, reserve memory for it.
+	 * +------------------------+ range_entry_top(tolum)
+	 * | Other reserved regions |
+	 * | APEI BERT region       |
+	 * +------------------------+ cbmem_top()
+	 * | CBMEM IMD ROOT         |
+	 * | CBMEM IMD SMALL        |
+	 * +------------------------+ range_entry_base(tolum), TOLUM
+	 * | CBMEM FSP MEMORY       |
+	 * | Other CBMEM regions... |
+	 */
+	if (CONFIG(ACPI_BERT))
+		arch_upd->BootLoaderTolumSize += CONFIG_ACPI_BERT_SIZE;
+
    /* Fill common settings on behalf of chipset. */
    if (fsp_fill_common_arch_params(arch_upd, s3wake, fsp_version,
    				memmap) != CB_SUCCESS)