Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
soc/amd/picasso: Move BERT region to cbmem
Allocate storage for the BERT reserved memory in cbmem, and add it in response to a romstage hook. Add a Kconfig option for adjusting the size reserved. This is different from the Stoney Ridge implementation where it was intentionally oversized to ease MTRR use and to keep TSEG aligned.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I4759154d394a8f5b35c0ef0a15994bbef25492e5 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/mca.c 2 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/38694/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 4a0fa56..7be9d94 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -185,6 +185,13 @@ ACPI Boot Error Record Table. This option reserves an 8MB region for building the error structures.
+config ACPI_BERT_SIZE + hex + default 0x4000 + help + Specify the amount of DRAM reserved for gathering the data used to + generate the ACPI table. + config RO_REGION_ONLY string depends on CHROMEOS diff --git a/src/soc/amd/picasso/mca.c b/src/soc/amd/picasso/mca.c index 57fa9c6..8621efb 100644 --- a/src/soc/amd/picasso/mca.c +++ b/src/soc/amd/picasso/mca.c @@ -20,6 +20,7 @@ #include <console/console.h> #include <arch/bert_storage.h> #include <cper.h> +#include <cbmem.h>
struct mca_bank { int bank; @@ -205,3 +206,33 @@ for (i = 0 ; i < num_banks ; i++) wrmsr(IA32_MC0_STATUS + (i * 4), mci.sts); } + +#define BERT_REGION_MAX_SIZE 0x100000 + +void bert_reserved_region(void **start, size_t *size) +{ + const struct cbmem_entry *bert; + + *start = 0; + *size = 0; + + bert = cbmem_entry_find(CBMEM_ID_BERT_RAW_DATA); + if (!bert) + return; + + *start = cbmem_entry_start(bert); + *size = cbmem_entry_size(bert); +} + +static void alloc_bert_in_cbmem(int unused) +{ + void *p; + + if (CONFIG(ACPI_BERT)) { + p = cbmem_add(CBMEM_ID_BERT_RAW_DATA, CONFIG_ACPI_BERT_SIZE); + if (!p) + printk(BIOS_ERR, "Error: BERT region was not added\n"); + } +} + +ROMSTAGE_CBMEM_INIT_HOOK(alloc_bert_in_cbmem)
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c@2... PS7, Line 202: 0 nit: Use NULL so it's clear it's a pointer.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c@2... PS7, Line 202: 0
nit: Use NULL so it's clear it's a pointer.
Yes, definitely.
Felix Held has uploaded a new patch set (#8) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
soc/amd/picasso: Move BERT region to cbmem
Allocate storage for the BERT reserved memory in cbmem, and add it in response to a romstage hook. Add a Kconfig option for adjusting the size reserved. This is different from the Stoney Ridge implementation where it was intentionally oversized to ease MTRR use and to keep TSEG aligned.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: I4759154d394a8f5b35c0ef0a15994bbef25492e5 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/mca.c M src/soc/amd/picasso/memmap.c 3 files changed, 38 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/38694/8
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/38694/7/src/soc/amd/picasso/mca.c@2... PS7, Line 202: 0
Yes, definitely.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 8: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
soc/amd/picasso: Move BERT region to cbmem
Allocate storage for the BERT reserved memory in cbmem, and add it in response to a romstage hook. Add a Kconfig option for adjusting the size reserved. This is different from the Stoney Ridge implementation where it was intentionally oversized to ease MTRR use and to keep TSEG aligned.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: I4759154d394a8f5b35c0ef0a15994bbef25492e5 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38694 Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/mca.c M src/soc/amd/picasso/memmap.c 3 files changed, 38 insertions(+), 23 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 842ba0c..4759f01 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -199,6 +199,13 @@ ACPI Boot Error Record Table. This option reserves an 8MB region for building the error structures.
+config ACPI_BERT_SIZE + hex + default 0x4000 + help + Specify the amount of DRAM reserved for gathering the data used to + generate the ACPI table. + config RO_REGION_ONLY string depends on CHROMEOS diff --git a/src/soc/amd/picasso/mca.c b/src/soc/amd/picasso/mca.c index 2970b94..cdea005 100644 --- a/src/soc/amd/picasso/mca.c +++ b/src/soc/amd/picasso/mca.c @@ -8,6 +8,7 @@ #include <console/console.h> #include <arch/bert_storage.h> #include <cper.h> +#include <cbmem.h>
struct mca_bank { int bank; @@ -193,3 +194,31 @@ for (i = 0 ; i < num_banks ; i++) wrmsr(IA32_MC0_STATUS + (i * 4), mci.sts); } + +void bert_reserved_region(void **start, size_t *size) +{ + const struct cbmem_entry *bert; + + *start = NULL; + *size = 0; + + bert = cbmem_entry_find(CBMEM_ID_BERT_RAW_DATA); + if (!bert) + return; + + *start = cbmem_entry_start(bert); + *size = cbmem_entry_size(bert); +} + +static void alloc_bert_in_cbmem(int unused) +{ + void *p; + + if (CONFIG(ACPI_BERT)) { + p = cbmem_add(CBMEM_ID_BERT_RAW_DATA, CONFIG_ACPI_BERT_SIZE); + if (!p) + printk(BIOS_ERR, "Error: BERT region was not added\n"); + } +} + +ROMSTAGE_CBMEM_INIT_HOOK(alloc_bert_in_cbmem) diff --git a/src/soc/amd/picasso/memmap.c b/src/soc/amd/picasso/memmap.c index 0c8d9c0..c6fd118 100644 --- a/src/soc/amd/picasso/memmap.c +++ b/src/soc/amd/picasso/memmap.c @@ -16,26 +16,6 @@ #include <soc/iomap.h> #include <amdblocks/acpimmio.h>
-#if CONFIG(ACPI_BERT) - #if CONFIG_SMM_TSEG_SIZE == 0x0 - #define BERT_REGION_MAX_SIZE 0x100000 - #else - /* SMM_TSEG_SIZE must stay on a boundary appropriate for its granularity */ - #define BERT_REGION_MAX_SIZE CONFIG_SMM_TSEG_SIZE - #endif -#else - #define BERT_REGION_MAX_SIZE 0 -#endif - -void bert_reserved_region(void **start, size_t *size) -{ - if (CONFIG(ACPI_BERT)) - *start = cbmem_top(); - else - start = NULL; - *size = BERT_REGION_MAX_SIZE; -} - void *cbmem_top_chipset(void) { msr_t tom = rdmsr(TOP_MEM); @@ -45,13 +25,12 @@
/* 8MB alignment to keep MTRR usage low */ return (void *)ALIGN_DOWN(restore_top_of_low_cacheable() - - CONFIG_SMM_TSEG_SIZE - - BERT_REGION_MAX_SIZE, 8*MiB); + - CONFIG_SMM_TSEG_SIZE, 8*MiB); }
static uintptr_t smm_region_start(void) { - return (uintptr_t)cbmem_top() + BERT_REGION_MAX_SIZE; + return (uintptr_t)cbmem_top(); }
static size_t smm_region_size(void)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2448 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2447 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2446 EMULATION_QEMU_AARCH64_FIT_SUPPORT_TIMESTAMPS using payload LinuxBoot_u-root_kexec_AARCH64 : SUCCESS : https://lava.9esec.io/r/2445
Please note: This test is under development and might not be accurate at all!
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
(3 comments)
Ah, sorry, Gerrit just told me, I'm too late.
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig... PS8, Line 199: 8MB Please update.
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... File src/soc/amd/picasso/memmap.c:
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... PS8, Line 35: start = NULL; Looks like a bug, are there more copies of this?
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... PS8, Line 33: return (uintptr_t)cbmem_top(); Where is TSEG actually configured in the hardware? Is the same ALIGN_DOWN() reflected there?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig... PS8, Line 199: 8MB
Please update.
will do
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... File src/soc/amd/picasso/memmap.c:
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... PS8, Line 35: start = NULL;
Looks like a bug, are there more copies of this?
yes, this is a bug and it was copied from stoneyridge. fixed in 40506
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/memmap.... PS8, Line 33: return (uintptr_t)cbmem_top();
Where is TSEG actually configured in the hardware? Is the same ALIGN_DOWN() […]
this code was copied from stoneyridge and will get removed in another patch in the picasso/mandolin patch train. this change was only to sort-of cleanly pull apart the pieces of this change from the other change
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/38694/8/src/soc/amd/picasso/Kconfig... PS8, Line 199: 8MB
will do
fixed in 40507
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
Something changed after CB:28470 ?
"The BERT region must be reported as Reserved to the OSPM, so this code calls out to a system-specific region locator. cbmem is reported as type 16 and is not usable for the BERT region."
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/9/src/soc/amd/picasso/mca.c File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/38694/9/src/soc/amd/picasso/mca.c@2... PS9, Line 224: ROMSTAGE_CBMEM_INIT_HOOK(alloc_bert_in_cbmem) This file is not built for romstage, is it?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38694 )
Change subject: soc/amd/picasso: Move BERT region to cbmem ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38694/9/src/soc/amd/picasso/mca.c File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/38694/9/src/soc/amd/picasso/mca.c@2... PS9, Line 224: ROMSTAGE_CBMEM_INIT_HOOK(alloc_bert_in_cbmem)
This file is not built for romstage, is it?
fixed in 40509