Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align
This patch makes top_of_ram aligned in order to meet MTRR alignment requirments.
Change-Id: I62d89cb35d8b5082d49c80aea55ac34dbb3b10ff Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 27 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35029/1
diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index 9e2f2f8..d83da7f 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -133,6 +133,7 @@ bool s3wake; struct postcar_frame pcf; uintptr_t top_of_ram; + const size_t top_of_ram_size = 16*MiB; struct chipset_power_state *ps = pmc_get_power_state();
console_init(); @@ -155,12 +156,14 @@ * 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 - * a safe bet to cover ramstage. + * a safe bet to cover ramstage. This satisfies MTRR alignment + * requirements as well. */ - top_of_ram = (uintptr_t) cbmem_top(); + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size); 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); + top_of_ram -= top_of_ram_size; + postcar_frame_add_mtrr(&pcf, top_of_ram, top_of_ram_size, + MTRR_TYPE_WRBACK);
/* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/soc/intel/denverton_ns/romstage.c b/src/soc/intel/denverton_ns/romstage.c index af65d38..0d8d4c8 100644 --- a/src/soc/intel/denverton_ns/romstage.c +++ b/src/soc/intel/denverton_ns/romstage.c @@ -142,6 +142,7 @@
struct postcar_frame pcf; uintptr_t top_of_ram; + const size_t top_of_ram_size = 16*MiB;
console_init();
@@ -164,10 +165,12 @@ * 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 a safe bet to cover ramstage. + * This satisfies MTRR alignment requirements as well. */ - top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(&pcf, top_of_ram - 16 * MiB, 16 * MiB, - MTRR_TYPE_WRBACK); + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size); + top_of_ram -= top_of_ram_size; + postcar_frame_add_mtrr(&pcf, top_of_ram, top_of_ram_size, + MTRR_TYPE_WRBACK);
/* Cache the memory-mapped boot media. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index 4d0cc17..a083788 100644 --- a/src/soc/intel/icelake/romstage/romstage.c +++ b/src/soc/intel/icelake/romstage/romstage.c @@ -117,6 +117,7 @@ bool s3wake; struct postcar_frame pcf; uintptr_t top_of_ram; + const size_t top_of_ram_size = 16*MiB; struct chipset_power_state *ps = pmc_get_power_state();
console_init(); @@ -139,12 +140,14 @@ * 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 - * a safe bet to cover ramstage. + * a safe bet to cover ramstage. This satisfies MTRR alignment + * requirements as well. */ - top_of_ram = (uintptr_t) cbmem_top(); + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size); 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); + top_of_ram -= top_of_ram_size; + postcar_frame_add_mtrr(&pcf, top_of_ram, top_of_ram_size, + MTRR_TYPE_WRBACK);
/* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 1d925b3..0977614 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -145,6 +145,7 @@ bool s3wake; struct postcar_frame pcf; uintptr_t top_of_ram; + const size_t top_of_ram_size = 16*MiB; struct chipset_power_state *ps;
console_init(); @@ -166,12 +167,14 @@ * 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 - * a safe bet to cover ramstage. + * a safe bet to cover ramstage. This satisfies MTRR alignment + * requirements as well. */ - top_of_ram = (uintptr_t) cbmem_top(); + top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size); 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); + top_of_ram -= top_of_ram_size; + postcar_frame_add_mtrr(&pcf, top_of_ram, top_of_ram_size, + MTRR_TYPE_WRBACK);
if (CONFIG(HAVE_SMI_HANDLER)) { /* Cache the TSEG region. */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size); So.. if cbmem_top() was at 31 MiB, your WB cache region now covers 0 MiB to 16MiB ?
So you need to align cbmem_top(). One more reason to put everything about postcar_frame into memmap.c files instead. CB:34894
I was hoping we could evolve from CB:34897 and have the platform code just tell the absolute upper boundary for WB (often end of TSEG with alignment no smaller than 8 MiB) and a floating lower boundary that is guaranteed to cover all of CBMEM. But I believe Aaron commented UC holes would be required. I just don't see why anyone would access those (claimed UC-only) regions before MP init where reprogramming of all the MTRRs happens.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
So.. if cbmem_top() was at 31 MiB, your WB cache region now covers 0 MiB to 16MiB ?
Shouldn't we have the same concern here as well ? https://review.coreboot.org/c/coreboot/+/33927
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
So.. if cbmem_top() was at 31 MiB, your WB cache region now covers 0 MiB to 16MiB ? […]
It does what the comment says, at least 8 MiB below, at most 8 MiB above cbmem_top() will be WB. The alignment is to 8 MiB but size is 16 MiB. And this approach is only allowed because postcar_frame_add_mtrr() will split this to multiple MTRRs when necessary.
That fsp1_1/car.c is once again a good sample where figuring out whether alignment requirements are met is difficult, because cbmem_top() implementations live in completely different directories. And it is the same platform_enter_postcar() for multiple different implementations of cbmem_top() too. That's why the approach of just having CBMEM and TSEG covered *somewhere* within the WB region would be so nice and easy.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
It does what the comment says, at least 8 MiB below, at most 8 MiB above cbmem_top() will be WB. The alignment is to 8 MiB but size is 16 MiB. And this approach is only allowed because postcar_frame_add_mtrr() will split this to multiple MTRRs when necessary.
So we can add the similar comments (as you have done there) to make our assumptions clear ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments. What is the further motivation here? Reducing variable MTRR usage? The algorithms take into account natural alignment. However, if the memory map is not aligned well then cbmem_top() should be the entity providing the alignment.
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
It does what the comment says, at least 8 MiB below, at most 8 MiB above cbmem_top() will be WB. […]
I really don't see what this change provides. It doesn't guarantee the memory we're actually using is covered. It's optimizing for MTRR usage while not guaranteeing the region we're using is covered.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments.
What is the further motivation here? Reducing variable MTRR usage?
Yes.
The algorithms take into account natural alignment. However, if the memory map is not aligned well then cbmem_top() should be the entity providing the alignment.
Do you mean memmap.c logic will take care of alignment part?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
I really don't see what this change provides. […]
yes, if top_of_ram is not aligned then above logic might block a region which might be out of our interest.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments.
What is the further motivation here? Reducing variable MTRR usage?
Yes.
The algorithms take into account natural alignment. However, if the memory map is not aligned well then cbmem_top() should be the entity providing the alignment.
Do you mean memmap.c logic will take care of alignment part?
All the algorithms in question already try to fit the requested address and size to meet mtrr natural alignment requirements. I'm suggesting that if there is an address which isn't aligned to high enough granularity we should just have cbmem_top() enforce a better alignment to optimize for MTRR usage at the expense of stranded DRAM. That said, it's easier said than done when it comes to FSP deciding memory maps and needing to deal w/ the reserved space.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments.
What is the further motivation here? Reducing variable MTRR usage?
Yes.
The algorithms take into account natural alignment. However, if the memory map is not aligned well then cbmem_top() should be the entity providing the alignment.
Do you mean memmap.c logic will take care of alignment part?
All the algorithms in question already try to fit the requested address and size to meet mtrr natural alignment requirements. I'm suggesting that if there is an address which isn't aligned to high enough granularity we should just have cbmem_top() enforce a better alignment to optimize for MTRR usage at the expense of stranded DRAM. That said, it's easier said than done when it comes to FSP deciding memory maps and needing to deal w/ the reserved space.
I believe from KBL time, we have enforce memory map alignment in CB space as well so its not true that FSP is deciding everything on reserving some random memory that CB is not aware of. design of memmap.c is taking care of transparent memory layout between FSP and CB. I'm quite sure we will get aligned memory in cbmem_top but the question was raised here that set_var_mtrr is wokring because cbmem_top is aligned, what if cbmem_top() is not align and we are still using set_var_mtrr hence i have ensured alignment again in this file. But now as we have lost romstage.c and its been using from common location, i believe we don't have any mean to apply these changes.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments.
All the algorithms in question already try to fit the requested address and size to meet mtrr natural alignment requirements. I'm suggesting that if there is an address which isn't aligned to high enough granularity we should just have cbmem_top() enforce a better alignment to optimize for MTRR usage at the expense of stranded DRAM. That said, it's easier said than done when it comes to FSP deciding memory maps and needing to deal w/ the reserved space.
I believe from KBL time, we have enforce memory map alignment in CB space as well so its not true that FSP is deciding everything on reserving some random memory that CB is not aware of. design of memmap.c is taking care of transparent memory layout between FSP and CB. I'm quite sure we will get aligned memory in cbmem_top but the question was raised here that set_var_mtrr is wokring because cbmem_top is aligned, what if cbmem_top() is not align and we are still using set_var_mtrr hence i have ensured alignment again in this file. But now as we have lost romstage.c and its been using from common location, i believe we don't have any mean to apply these changes.
I wasn't aware we were talking about using set_var_mtrr() directly and only once. That's a completely different problem. Once can just as easily use the same looping flow to call set_var_mtrr() more often. I think it's important to be specific in what one is trying to optimize for. The terse commit description didn't indicate what assumptions in the goal it was trying to achieve: one call to set_var_mtrr() which I don't think is a great goal on its own, fwiw.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35029?usp=email )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)