Change in ...coreboot[master]: [WIP] nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31150
Change subject: [WIP] nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
[WIP] nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT
Fail builds if MRC blobs pool heap would get corrupted by CAR relocatable data from coreboot proper.
Patch the blob instead: https://review.coreboot.org/c/blobs/+/31149
Change-Id: Ibc771b592b35d77be81fce87769314fe6bb84c87 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31150/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 7b10f43..fe55071 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -89,6 +89,13 @@ _car_global_end = .; _car_relocatable_data_end = .;
+#if IS_ENABLED(CONFIG_NORTHBRIDGE_INTEL_SANDYBRIDGE) && !IS_ENABLED(CONFIG_USE_NATIVE_RAMINIT) + . = ABSOLUTE(0xff7e1000); + _mrc_var_heap_start = .; + . += 0x3000; + _mrc_var_heap_end = .; +#endif + #if !IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) _car_stack_start = .; _car_stack_end = _car_region_end;
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31150
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT
Fail builds if MRC blobs pool heap would get corrupted by CAR relocatable data from coreboot proper.
Add runtime logging how much pool was required.
Change-Id: Ibc771b592b35d77be81fce87769314fe6bb84c87 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/northbridge/intel/sandybridge/raminit_mrc.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31150/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... PS2, Line 267: (CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE) please, no spaces at the start of a line
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... PS2, Line 294: mrc_var = (void*)DCACHE_RAM_MRC_VAR_BASE; "(foo*)" should be "(foo *)"
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 2:
(1 comment)
With the style issues addressed, this looks good.
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... PS2, Line 301: BIOS_DEBUG Should that be an error or warning?
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31150
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT
Fail builds if MRC blobs pool heap would get corrupted by CAR relocatable data from coreboot proper.
Add runtime logging how much pool was required.
Change-Id: Ibc771b592b35d77be81fce87769314fe6bb84c87 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/northbridge/intel/sandybridge/raminit_mrc.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/31150/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
(1 comment)
Matt, can you experiment/extend this to the platforms you need to use MRC blob with? I am hoping to see they would have their pool at a better location.
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... PS2, Line 301: BIOS_DEBUG
Should that be an error or warning?
Good question. If we were able to parse (MRC internal structure) and the used pool did not end up where linker script had its reserve, I might even add die() above...
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
kmalkki: so test on HSW, BDW, BYT then? Assuming IVB is covered by this already
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31150/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31150/3//COMMIT_MSG@8 PS3, Line 8: : Fail builds How is this achieved?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
Patch Set 3:
kmalkki: so test on HSW, BDW, BYT then? Assuming IVB is covered by this already
Well I am hoping this would have been improved for haswell already. I don't seem to get or ask the correct questions to get this answered from Google. Mostly because these are 5 years old already.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31150/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31150/3//COMMIT_MSG@8 PS3, Line 8: : Fail builds
How is this achieved?
The region declared here with ABSOLUTE() cannot move. Therefore, if relocatable_data region grows, its end would go past this ABSOLUTE() addresss. Linker script then fails with 'cannot move pointer backwards' or some error like that.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
kmalkki: so test on HSW, BDW, BYT then? Assuming IVB is covered by this already
Well I am hoping this would have been improved for haswell already. I don't seem to get or ask the correct questions to get this answered from Google. Mostly because these are 5 years old already.
I can have a look at the output data pointer on haswell to see what it's doing, but a quick look at the binary did not find weird pointers outside the DCACHE_RAM_MRC_VAR region.
The blobs repo has multiple binaries for sandy/ivy but we generally end up using systemagent-r6.bin. Do we know the other ones have the same heap? (also do we care?)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
kmalkki: so test on HSW, BDW, BYT then? Assuming IVB is covered by this already
Well I am hoping this would have been improved for haswell already. I don't seem to get or ask the correct questions to get this answered from Google. Mostly because these are 5 years old already.
I can have a look at the output data pointer on haswell to see what it's doing, but a quick look at the binary did not find weird pointers outside the DCACHE_RAM_MRC_VAR region.
The blobs repo has multiple binaries for sandy/ivy but we generally end up using systemagent-r6.bin. Do we know the other ones have the same heap? (also do we care?
AFAIR, the other two blobs were built with previous revision of headers, most notably struct pei_data. I don't know why we did not just remove them.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Looks good enough to me. Is it ready?
The blobs repo has multiple binaries for sandy/ivy but we generally end up using systemagent-r6.bin. Do we know the other ones have the same heap? (also do we care?
AFAIR, the other two blobs were built with previous revision of headers, most notably struct pei_data. I don't know why we did not just remove them.
We should. They are just old binaries not compatible with the current tree anyway.
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/31150/2/src/northbridge/intel/sandybridge/ra... PS2, Line 301: BIOS_DEBUG
Good question. […]
any update on this?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 4:
AFAIR, the other two blobs were built with previous revision of headers, most notably struct pei_data. I don't know why we did not just remove them.
We should. They are just old binaries not compatible with the current tree anyway.
CB:31763
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
Patch Set 4: Code-Review+1
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31150 )
Change subject: nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT ......................................................................
nb/intel/sandybridge: Reserve CAR region with !NATIVE_RAMINIT
Fail builds if MRC blobs pool heap would get corrupted by CAR relocatable data from coreboot proper.
Add runtime logging how much pool was required.
Change-Id: Ibc771b592b35d77be81fce87769314fe6bb84c87 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31150 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/car.ld M src/northbridge/intel/sandybridge/raminit_mrc.c 2 files changed, 34 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 2d835a3..4360830 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -85,6 +85,14 @@ _car_global_end = .; _car_relocatable_data_end = .;
+#if IS_ENABLED(CONFIG_NORTHBRIDGE_INTEL_SANDYBRIDGE) && \ + !IS_ENABLED(CONFIG_USE_NATIVE_RAMINIT) + . = ABSOLUTE(0xff7e1000); + _mrc_pool = .; + . += 0x5000; + _emrc_pool = .; +#endif + #if !IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) _car_stack_start = .; _car_stack_end = _car_region_end; diff --git a/src/northbridge/intel/sandybridge/raminit_mrc.c b/src/northbridge/intel/sandybridge/raminit_mrc.c index 6142388..853fdb8 100644 --- a/src/northbridge/intel/sandybridge/raminit_mrc.c +++ b/src/northbridge/intel/sandybridge/raminit_mrc.c @@ -25,6 +25,7 @@ #include <ip_checksum.h> #include <pc80/mc146818rtc.h> #include <device/pci_def.h> +#include <lib.h> #include <mrc_cache.h> #include <halt.h> #include <timestamp.h> @@ -262,10 +263,23 @@ report_memory_config(); }
+/* These are the location and structure of MRC_VAR data in CAR. */ +#define DCACHE_RAM_MRC_VAR_BASE \ + (CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE) + +struct mrc_var_data { + u32 acpi_timer_flag; + u32 pool_used; + u32 pool_base; + u32 tx_byte; + u32 reserved[4]; +} __packed; + void perform_raminit(int s3resume) { int cbmem_was_initted; struct pei_data pei_data; + struct mrc_var_data *mrc_var;
/* Prepare USB controller early in S3 resume */ if (!mainboard_should_reset_usb(s3resume)) @@ -277,6 +291,18 @@ pei_data.boot_mode = s3resume ? 2 : 0; timestamp_add_now(TS_BEFORE_INITRAM); sdram_initialize(&pei_data); + + mrc_var = (void *)DCACHE_RAM_MRC_VAR_BASE; + /* Sanity check mrc_var location by verifying a known field. */ + if (mrc_var->tx_byte == (uintptr_t)pei_data.tx_byte) { + printk(BIOS_DEBUG, "MRC_VAR pool occupied [%08x,%08x]\n", + mrc_var->pool_base, + mrc_var->pool_base + mrc_var->pool_used); + } else { + printk(BIOS_ERR, "Could not parse MRC_VAR data\n"); + hexdump32(BIOS_ERR, mrc_var, sizeof(*mrc_var)/sizeof(u32)); + } + cbmem_was_initted = !cbmem_recovery(s3resume); if (!s3resume) save_mrc_data(&pei_data);
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)