Change in coreboot[master]: soc/intel/common/block/systemagent/memmap.c: Align cached region
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
soc/intel/common/block/systemagent/memmap.c: Align cached region
When asked to place cbmem_top(), FSP does not seem to care about alignment. It can return an address that is MTRR poison, which will exhaust all variable MTRRs when trying to set up caching for CBMEM. This will make memory-mapped flash and TSEG caching fail as well.
Safeguard against this by aligning the region to cache to half of its size, and move it upwards to compensate. It is assumed that caching memory above the provided bootloader TOLUM address is inconsequential.
TEST=Boot out-of-tree Skylake board, observe no MTRR exhaustion error messages in console. The boot process also feels more fluid.
Change-Id: Ic64fd6d3d9e8ab4c78d68b910a476f9c4eb2d353 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/systemagent/memmap.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/45930/1
diff --git a/src/soc/intel/common/block/systemagent/memmap.c b/src/soc/intel/common/block/systemagent/memmap.c index 985f2c4..c59d93e 100644 --- a/src/soc/intel/common/block/systemagent/memmap.c +++ b/src/soc/intel/common/block/systemagent/memmap.c @@ -6,6 +6,7 @@ #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> #include <intelblocks/systemagent.h> +#include <types.h>
/* * Expected Host Memory Map (we don't know 100% and not all regions are present on all SoCs): @@ -55,18 +56,18 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; + /* FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top() */ + uintptr_t top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8 * MiB);
/* * We need to make sure ramstage will be run cached. At this * point exact location of ramstage in cbmem is not known. - * Instruct postcar to cache 16 megs under cbmem top which is + * Instruct postcar to cache 16 megs around cbmem top which is * a safe bet to cover ramstage. */ - top_of_ram = (uintptr_t) cbmem_top(); printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); - top_of_ram -= 16*MiB; - postcar_frame_add_mtrr(pcf, top_of_ram, 16*MiB, MTRR_TYPE_WRBACK); + + postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... PS2, Line 70: postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK); Compared to older versions, we don't care about the space above `cbmem_top` as TSEG is cached separately below. So how about aligning up to 8MiB and then subtracting 16MiB? It should give better results (more DRAM covered that we care about) in case `cbmem_top` is already aligned.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... PS2, Line 70: postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
Compared to older versions, we don't care about the space above `cbmem_top` […]
Hrm, good point. Aligning upwards would work better for this.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45930
to look at the new patch set (#3).
Change subject: [NEED RETEST] soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
[NEED RETEST] soc/intel/common/block/systemagent/memmap.c: Align cached region
When asked to place cbmem_top(), FSP does not seem to care about alignment. It can return an address that is MTRR poison, which will exhaust all variable MTRRs when trying to set up caching for CBMEM. This will make memory-mapped flash and TSEG caching fail as well.
Safeguard against this by aligning the region to cache to half of its size, and move it upwards to compensate. It is assumed that caching memory above the provided bootloader TOLUM address is inconsequential.
TEST=Boot out-of-tree Skylake board, observe no MTRR exhaustion error messages in console. The boot process also feels more fluid.
Change-Id: Ic64fd6d3d9e8ab4c78d68b910a476f9c4eb2d353 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/systemagent/memmap.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/45930/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: [NEED RETEST] soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... PS2, Line 70: postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
Hrm, good point. Aligning upwards would work better for this.
Done but needs to be re-tested.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45930
to look at the new patch set (#4).
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
soc/intel/common/block/systemagent/memmap.c: Align cached region
When asked to place cbmem_top(), FSP does not seem to care about alignment. It can return an address that is MTRR poison, which will exhaust all variable MTRRs when trying to set up caching for CBMEM. This will make memory-mapped flash and TSEG caching fail as well.
Safeguard against this by aligning the region to cache to half of its size, and move it upwards to compensate. It is assumed that caching memory above the provided bootloader TOLUM address is inconsequential.
TEST=Boot Purism Librem Mini WHL, observe no MTRR exhaustion error messages in console. The boot process also feels more fluid.
Change-Id: Ic64fd6d3d9e8ab4c78d68b910a476f9c4eb2d353 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/systemagent/memmap.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/45930/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/2/src/soc/intel/common/block/... PS2, Line 70: postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
Done but needs to be re-tested.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top() Just curious, what platforms have you seen the issue and how bad is it?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top()
Just curious, what platforms have you seen the issue and how bad is it?
I've seen it happen with the Kaby and Coffee Lake FSPs. I don't have any other platforms to test on. On Kaby Lake, I saw that it tries to use 0x79fff000 as top_of_ram which fails miserably, and then `postcar_enable_tseg_cache` fails miserably as well. On Whiskey Lake, this more or less halves the time for a fast boot (800-ish milliseconds down to 400-ish milliseconds).
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top()
I've seen it happen with the Kaby and Coffee Lake FSPs. I don't have any other platforms to test on. […]
I think FSP-M places some data below the real TOLUD and reports the top below that, which explains the weird alignment.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top()
I think FSP-M places some data below the real TOLUD and reports the top below that, which explains t […]
Yes, that's why caching this region is fine
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top()
Yes, that's why caching this region is fine
Wow, yeah that is pretty bad, thanks.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
soc/intel/common/block/systemagent/memmap.c: Align cached region
When asked to place cbmem_top(), FSP does not seem to care about alignment. It can return an address that is MTRR poison, which will exhaust all variable MTRRs when trying to set up caching for CBMEM. This will make memory-mapped flash and TSEG caching fail as well.
Safeguard against this by aligning the region to cache to half of its size, and move it upwards to compensate. It is assumed that caching memory above the provided bootloader TOLUM address is inconsequential.
TEST=Boot Purism Librem Mini WHL, observe no MTRR exhaustion error messages in console. The boot process also feels more fluid.
Change-Id: Ic64fd6d3d9e8ab4c78d68b910a476f9c4eb2d353 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45930 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/common/block/systemagent/memmap.c 1 file changed, 6 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Arthur Heymans: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/common/block/systemagent/memmap.c b/src/soc/intel/common/block/systemagent/memmap.c index 985f2c4..27870b0 100644 --- a/src/soc/intel/common/block/systemagent/memmap.c +++ b/src/soc/intel/common/block/systemagent/memmap.c @@ -6,6 +6,7 @@ #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> #include <intelblocks/systemagent.h> +#include <types.h>
/* * Expected Host Memory Map (we don't know 100% and not all regions are present on all SoCs): @@ -55,18 +56,18 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; + /* FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top() */ + uintptr_t top_of_ram = ALIGN_UP((uintptr_t)cbmem_top(), 8 * MiB);
/* * We need to make sure ramstage will be run cached. At this * point exact location of ramstage in cbmem is not known. - * Instruct postcar to cache 16 megs under cbmem top which is + * Instruct postcar to cache 16 megs below cbmem top which is * a safe bet to cover ramstage. */ - top_of_ram = (uintptr_t) cbmem_top(); printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); - top_of_ram -= 16*MiB; - postcar_frame_add_mtrr(pcf, top_of_ram, 16*MiB, MTRR_TYPE_WRBACK); + + postcar_frame_add_mtrr(pcf, top_of_ram - 16 * MiB, 16 * MiB, MTRR_TYPE_WRBACK);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45930 )
Change subject: soc/intel/common/block/systemagent/memmap.c: Align cached region ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/memmap.c:
https://review.coreboot.org/c/coreboot/+/45930/4/src/soc/intel/common/block/... PS4, Line 59: FSP does not seem to bother w.r.t. alignment when asked to place cbmem_top()
I think FSP-M places some data below the real TOLUD and reports the top below that, which explains the weird alignment.
Basically what the picture above lists between TSEG and TOLUM. The list should be accurate, we acted on that information for a while and it worked out mostly. Until we decided to just use FSP's HOP entry.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)
-
Tim Wawrzynczak (Code Review)